Merge pull request #6640 from dotCooCoo/fix/ids-null-takes-selection

Commands: ids null acts on the selection instead of being refused

crates/engine/src/cmd.rs

@@ -519,9 +519,13 @@ pub(crate) fn checked_ids_param(s: &Session, p: &Value, key: &str, command: &str
 }
 
 /// The objects a command acts on: `ids` when given (every one checked, and those inside another
-/// given one dropped), else the selected objects.
+/// given one dropped), else (`ids` left out or null) the selected objects.
 pub(crate) fn checked_ids_or_selection(s: &Session, p: &Value, command: &str) -> Result<Vec<NodeId>> {
-    if p.get("ids").is_some() { Ok(edit::outermost(&s.doc()?.doc, checked_ids_param(s, p, "ids", command)?)) } else { edit::selected_roots(s) }
+    if p.get("ids").is_some_and(|v| !v.is_null()) {
+        Ok(edit::outermost(&s.doc()?.doc, checked_ids_param(s, p, "ids", command)?))
+    } else {
+        edit::selected_roots(s)
+    }
 }
 
 pub(crate) fn checked_id(s: &Session, value: &Value, command: &str) -> Result<NodeId> {

crates/engine/src/cmd/addons/cleanup/text.rs

@@ -806,7 +806,7 @@ fn remove_breaks(s: &mut Session, p: &Value) -> Result<Value> {
     };
     // The characters a Type tool has selected (after Select All in threaded text, all the text of
     // every frame of the story), else all the text of the selected type.
-    let editing = if p.get("ids").is_some() { None } else { editing_range(s)?.filter(|(_, a, b)| a != b) };
+    let editing = if p.get("ids").is_some_and(|v| !v.is_null()) { None } else { editing_range(s)?.filter(|(_, a, b)| a != b) };
     let story = if editing.is_some() { editing_story(s) } else { vec![] };
     let d = &s.doc()?.doc;
     // Each type object with the breaks that go from it.

crates/engine/src/cmd/addons/cleanup/text/tests.rs

@@ -288,6 +288,18 @@ fn remove_breaks_in_the_characters_a_type_tool_selected() {
     assert_eq!(obj(&s, id).plain_text(), "a b\nc");
 }
 
+/// `ids: null` is `ids` left out: the characters the Type tool selected lose their breaks, and
+/// the rest of the text keeps them.
+#[test]
+fn remove_breaks_with_ids_null_in_the_characters_a_type_tool_selected() {
+    let mut s = session();
+    let id = text(&mut s, json!({"x": 40, "y": 60, "text": "a\nb\nc", "size": 12}));
+    select(&mut s, &[id]);
+    edit_text(&mut s, id, 0, 2);
+    s.execute("addon.cleanup.removeBreaks", &json!({"kind": "paragraph", "ids": null})).unwrap();
+    assert_eq!(obj(&s, id).plain_text(), "a b\nc");
+}
+
 #[test]
 fn split_at_the_cursor_and_around_a_selection() {
     for (anchor, caret, want) in [(6, 6, &["Split", "here please"][..]), (6, 10, &["Split", "here", "please"][..])] {

crates/engine/src/tests/ids_without_selection.rs

@@ -436,9 +436,7 @@ fn an_ids_entry_that_is_not_an_id_is_refused_and_changes_nothing() {
 /// With the rectangle apart (`d`) selected, the same commands with `ids` (or `slices`) left out and
 /// with it null do the same: what the selection's list does for the commands that take the
 /// selection, and the command's own default for the others (`object.slice.select` deselects the
-/// slices). `addon.color.filter` and `addon.color.applyNearestSwatch` read `ids` through
-/// [`cmd::checked_ids_or_selection`], which refuses null as a value that is not a list and changes
-/// nothing.
+/// slices).
 #[test]
 fn ids_left_out_or_null_take_the_selection_or_the_default() {
     let (_, [.., d], _) = doc_with_type();
@@ -450,15 +448,7 @@ fn ids_left_out_or_null_take_the_selection_or_the_default() {
         let mut null = params.clone();
         null[key] = serde_json::Value::Null;
         let with_null = outcome(cmd, &null, &[d], slice);
-        if matches!(cmd, "addon.color.filter" | "addon.color.applyNearestSwatch") {
-            let (mut s, ..) = doc_with_type();
-            s.execute("select.set", &json!({ "ids": [d] })).unwrap();
-            let st = s.doc().unwrap();
-            let refused = with_null.0.as_ref().is_err_and(|e| e.ends_with("ids must be an array of non-negative integer object ids"));
-            if !refused || (&with_null.1, &with_null.2, with_null.3) != (&*st.doc, &st.selection, st.history.undo.len()) {
-                failures.push(format!("{cmd} {key}: null is not refused with nothing changed ({:.160?})", with_null.0));
-            }
-        } else if with_null != left_out {
+        if with_null != left_out {
             failures.push(format!("{cmd} {key}: null does not do what leaving it out does ({:.160?})", left_out.0));
         }
         let mut listed = params.clone();
@@ -477,6 +467,65 @@ fn ids_left_out_or_null_take_the_selection_or_the_default() {
     assert!(failures.is_empty(), "{} fail:\n{}", failures.len(), failures.join("\n"));
 }
 
+/// The commands that read `ids` through [`cmd::checked_ids_or_selection`] take `ids: null` as left
+/// out: with the rectangle apart (`d`) or the type object (`t`) selected, null gives the same
+/// result or error, document, selection and undo history as leaving `ids` out. Print Ink Fixes
+/// runs in a CMYK document.
+#[test]
+fn ids_null_takes_the_selection_in_every_command_that_falls_back_to_it() {
+    let (_, [.., d], t) = doc_with_type();
+    let mut failures = vec![];
+    let (rect, both, text) = (&[d][..], &[d, t][..], &[t][..]);
+    let cases = [
+        ("addon.arrange.randomizeOpacity", json!({"seed": 1}), rect),
+        ("addon.arrange.sort", json!({"by": "area", "apply": "x"}), both),
+        ("addon.cleanup.simplify", json!({}), rect),
+        ("addon.cleanup.deepUngroup", json!({}), rect),
+        ("addon.cleanup.rejoin", json!({}), rect),
+        ("addon.color.nearestSwatch", json!({}), rect),
+        ("addon.pathtools.cornerSet", json!({"radius": 2}), rect),
+        ("addon.pathtools.cornerRemove", json!({}), rect),
+        ("addon.pathtools.shape.convert", json!({}), rect),
+        ("addon.pathtools.erase", json!({"points": [[0, 20], [200, 20]]}), rect),
+        ("addon.pathtools.crossings", json!({}), rect),
+        ("addon.pathtools.expandDashes", json!({}), rect),
+        ("object.path.smooth", json!({}), rect),
+        ("object.textWrap.make", json!({}), rect),
+        ("object.textWrap.release", json!({}), rect),
+        ("object.textWrap.options", json!({}), rect),
+        ("variable.bind", json!({"variable": "v"}), rect),
+        ("variable.unbind", json!({}), rect),
+        ("addon.cleanup.breakText", json!({}), text),
+        ("addon.cleanup.combineText", json!({}), text),
+        ("addon.cleanup.joinText", json!({}), text),
+        ("addon.cleanup.removeBreaks", json!({"kind": "line"}), text),
+        ("addon.cleanup.resetTextTransform", json!({}), text),
+        ("addon.cleanup.alignInPlace", json!({"align": "left"}), text),
+    ];
+    for (cmd, params, selected) in cases {
+        let left_out = outcome(cmd, &params, selected, false);
+        let mut null = params.clone();
+        null["ids"] = serde_json::Value::Null;
+        let with_null = outcome(cmd, &null, selected, false);
+        if with_null != left_out {
+            failures.push(format!("{cmd}: null gives {:.160?}; left out gives {:.160?}", with_null.0, left_out.0));
+        }
+    }
+    let print_fixes = |p: &serde_json::Value| {
+        let mut s = Session::new();
+        s.execute("file.new", &json!({"width": 200, "height": 200, "colorMode": "cmyk", "created": null})).unwrap();
+        s.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 50, "height": 40})).unwrap();
+        let r = s.execute("addon.color.printFixes", p).map_err(|e| e.to_string());
+        let st = s.doc().unwrap();
+        (r, (*st.doc).clone(), st.selection.clone(), st.history.undo.len())
+    };
+    let (left_out, with_null) = (print_fixes(&json!({"inkLimit": 200})), print_fixes(&json!({"inkLimit": 200, "ids": null})));
+    if with_null != left_out {
+        failures.push(format!("addon.color.printFixes: null gives {:.160?}; left out gives {:.160?}", with_null.0, left_out.0));
+    }
+    assert!(failures.is_empty(), "{} fail:\n{}", failures.len(), failures.join("\n"));
+}
+
 /// With no document open, every command that takes `ids` refuses a list with an entry that is not
 /// a non-negative integer, and a value that is not a list, with the same error as with a document
 /// open (`effect.list` and `links.findFiles` run without a document).