Merge pull request #1336 from dotCooCoo/feat/no-op-undo-opacity-masks

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

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();