Transparency: opacity mask options that change nothing record no undo step

Disable, Enable, Unlink and Link Opacity Mask and Opacity Mask Options
(transparency.setOpacityMask) edited the document whenever a target
had an opacity mask, even when every mask already had the flags the
call set. Such a call recorded an undo step, emptied Redo and marked
the document modified.

When every mask a call acts on has those flags already, the call now
returns without editing the document, whether it acts on the
selection, on id or ids, or on the object whose mask is being edited.
The reply is still null, and the journal still records the call. A
call that changes a flag on any of the masks is still one undo step.

One line in the evidence list of docs/target-app-parity.md names the
commands.

crates/engine/src/cmd/opacitymask.rs

@@ -250,6 +250,18 @@ fn set_flags(s: &mut Session, target: &Value, label: &str, p: &Value) -> Result<
         return Err(bad("transparency.setOpacityMask", "give at least one of clip, invert, disabled, linked"));
     }
     let ids = masked_targets(s, target)?;
+    // When every mask already has the flags given, the document and its undo history stay as they
+    // are.
+    let has_flags = |m: &OpacityMask| {
+        clip.is_none_or(|v| m.clip == v)
+            && invert.is_none_or(|v| m.invert == v)
+            && disabled.is_none_or(|v| m.disabled == v)
+            && linked.is_none_or(|v| m.linked == v)
+    };
+    let d = &s.doc()?.doc;
+    if ids.iter().all(|id| d.node(*id).and_then(|n| n.mask.as_deref()).is_none_or(has_flags)) {
+        return ok();
+    }
     s.edit(label, |d, _| {
         for id in &ids {
             if let Some(m) = d.node_mut(*id).and_then(|n| n.mask.as_mut()) {

crates/engine/src/tests.rs

@@ -571,13 +571,13 @@ fn selection_tool_drag_is_one_undo_step() {
 }
 
 /// How many steps there are to undo and to redo, and whether the document has unsaved changes.
-fn history(s: &Session) -> (usize, usize, bool) {
+pub(crate) fn history(s: &Session) -> (usize, usize, bool) {
     let st = s.doc().unwrap();
     (st.history.undo.len(), st.history.redo.len(), st.is_dirty())
 }
 
 /// Select `ids` with a move of them left to redo, and mark the document saved.
-fn select_with_a_step_to_redo(s: &mut Session, ids: &[NodeId]) {
+pub(crate) fn select_with_a_step_to_redo(s: &mut Session, ids: &[NodeId]) {
     s.execute("select.set", &json!({"ids": ids.iter().map(|i| i.0).collect::<Vec<_>>()})).unwrap();
     s.execute("object.move", &json!({"dx": 5, "dy": 0})).unwrap();
     s.execute("edit.undo", &json!({})).unwrap();

crates/engine/src/tests_opacitymask.rs

@@ -6,6 +6,7 @@ use serde_json::{Value, json};
 
 use super::*;
 use crate::cmd::maskedit::MASK_EDIT_LAYER;
+use crate::tests::{history, select_with_a_step_to_redo};
 
 fn session() -> Session {
     let mut s = Session::new();
@@ -67,6 +68,70 @@ fn mask_options_act_on_the_masked_object_while_editing() {
     assert_eq!((info["ids"].clone(), info["opacity"].clone(), info["editingMask"].clone()), (json!([obj.0]), json!(40.0), json!(obj.0)));
 }
 
+/// Disable, Enable, Unlink and Link Opacity Mask and Opacity Mask Options with the flags the masks
+/// already have record no undo step, on the selection, with ids and while the mask is being edited:
+/// the step to redo stays, the document stays unmodified and the journal still has the command. A
+/// call that changes a flag on one of the masks is one undo step. A target without a mask and a
+/// call that gives no flag are still errors.
+#[test]
+fn mask_options_already_set_record_no_undo_step() {
+    let mut s = session();
+    let masked = |s: &mut Session, x: f64| {
+        let (obj, m) = (rect(s, x, 100.0), rect(s, x, 50.0));
+        s.execute("select.set", &json!({"ids": [obj.0, m.0]})).unwrap();
+        s.execute("transparency.makeOpacityMask", &json!({})).unwrap();
+        obj
+    };
+    // `a`'s mask is as made (clipping, not inverted, enabled, linked); `b`'s has each flag the other
+    // way.
+    let (a, b) = (masked(&mut s, 0.0), masked(&mut s, 150.0));
+    s.execute("transparency.setOpacityMask", &json!({"id": b.0, "clip": false, "invert": true, "disabled": true, "linked": false})).unwrap();
+    select_with_a_step_to_redo(&mut s, &[a]);
+    let before = history(&s);
+    assert_eq!((before.1, before.2), (1, false), "a step to redo, and nothing unsaved");
+    let calls = [
+        ("transparency.enableOpacityMask", json!({})),
+        ("transparency.linkOpacityMask", json!({})),
+        ("transparency.setOpacityMask", json!({"clip": true, "invert": false, "disabled": false, "linked": true})),
+        ("transparency.disableOpacityMask", json!({"id": b.0})),
+        ("transparency.unlinkOpacityMask", json!({"ids": [b.0]})),
+        ("transparency.setOpacityMask", json!({"ids": [b.0], "clip": false, "invert": true})),
+    ];
+    for (id, p) in calls {
+        assert_eq!(s.execute(id, &p).unwrap(), Value::Null, "{id} {p}");
+        assert_eq!(history(&s), before, "{id} {p}");
+        assert_eq!(s.journal.last().unwrap(), &(id.to_string(), p));
+    }
+    // While `a`'s mask is edited, the selection is its mask art and the commands act on `a`.
+    s.execute("transparency.editOpacityMask", &json!({"id": a.0})).unwrap();
+    let art = s.doc().unwrap().selection.objects.clone();
+    select_with_a_step_to_redo(&mut s, &art);
+    let before = history(&s);
+    assert_eq!((before.1, before.2), (1, false), "a step to redo, and nothing unsaved while editing");
+    let calls = [
+        ("transparency.enableOpacityMask", json!({})),
+        ("transparency.linkOpacityMask", json!({})),
+        ("transparency.setOpacityMask", json!({"clip": true, "linked": true})),
+    ];
+    for (id, p) in calls {
+        assert_eq!(s.execute(id, &p).unwrap(), Value::Null, "{id} {p} while editing");
+        assert_eq!(history(&s), before, "{id} {p} while editing");
+        assert_eq!(s.journal.last().unwrap(), &(id.to_string(), p));
+    }
+    assert!(doc(&s).mask_edit.is_some(), "still editing");
+    // `b`'s mask is disabled and `a`'s is not: disabling both is one undo step, and it clears Redo.
+    s.execute("transparency.stopEditingOpacityMask", &json!({})).unwrap();
+    select_with_a_step_to_redo(&mut s, &[a, b]);
+    let undo = history(&s).0;
+    s.execute("transparency.disableOpacityMask", &json!({})).unwrap();
+    assert_eq!(history(&s), (undo + 1, 0, true));
+    assert!(mask(&s, a).unwrap().disabled && mask(&s, b).unwrap().disabled);
+    let plain = rect(&mut s, 0.0, 20.0);
+    let err = s.execute("transparency.enableOpacityMask", &json!({"id": plain.0})).unwrap_err();
+    assert!(err.to_string().contains("no target object has an opacity mask"), "{err}");
+    assert!(s.execute("transparency.setOpacityMask", &json!({"id": a.0})).is_err(), "no flag given");
+}
+
 #[test]
 fn release_while_editing_leaves_editing_first() {
     let mut s = session();