Merge pull request #3967 from Taha-Mahmoodi/fix/commands-accept-id

Fix #3965: id works as ids of one with nothing selected wherever params take ids?|id?

crates/engine/src/cmd/docinfo.rs

@@ -436,8 +436,11 @@ fn snap_xf(b: Rect, half: bool) -> Affine {
 }
 
 fn pixel_perfect(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids: Vec<NodeId> =
-        if p.get("ids").is_some() { super::edit::outermost(&s.doc()?.doc, targets(s, p)?) } else { super::edit::selected_roots(s)? };
+    let ids: Vec<NodeId> = if p.get("ids").is_some() || p.get("id").is_some() {
+        super::edit::outermost(&s.doc()?.doc, targets(s, p)?)
+    } else {
+        super::edit::selected_roots(s)?
+    };
     let n = ids.len();
     s.edit("Make Pixel Perfect", |d, _| {
         for id in &ids {

crates/engine/src/cmd/gradient.rs

@@ -586,7 +586,9 @@ fn set_gradient_geom(s: &mut Session, p: &Value) -> Result<Value> {
     let stroke = edits_stroke(s, p, item, !s.fill_active, C)?;
     let item = item.of_kind(s, !stroke);
     let targets = match item {
-        ItemTarget::Top => leaf_targets(s, &ids_param(p, "ids").map_or_else(|| selected_roots(s), |ids| Ok(outermost(&s.doc()?.doc, ids)))?)?,
+        ItemTarget::Top => {
+            leaf_targets(s, &super::given_objects(s, p, C)?.map_or_else(|| selected_roots(s), |ids| Ok(outermost(&s.doc()?.doc, ids)))?)?
+        }
         _ => item.targets(s, p)?,
     };
     if targets.is_empty() {

crates/engine/src/cmd/mod.rs

@@ -367,6 +367,22 @@ pub(crate) fn id_param(p: &Value, key: &str) -> Option<NodeId> {
 pub(crate) fn ids_param(p: &Value, key: &str) -> Option<Vec<NodeId>> {
     p.get(key).and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(NodeId).collect())
 }
+/// The objects `p` gives `command`: its `ids`, else its `id` as `ids` of one (as [`targets`] reads
+/// them), each checked ([`checked_id`]): a malformed one, or one naming no object, is an error,
+/// never the selection. `None` when it gives neither (or gives them as `null`).
+pub(crate) fn given_objects(s: &Session, p: &Value, command: &str) -> Result<Option<Vec<NodeId>>> {
+    if p.get("ids").is_some_and(|v| !v.is_null()) {
+        return checked_ids_param(s, p, "ids", command).map(Some);
+    }
+    match p.get("id").filter(|v| !v.is_null()) {
+        None => Ok(None),
+        // A step reference a batch couldn't resolve, as `targets` reports it, under the command.
+        Some(Value::String(r)) if r.starts_with('$') => {
+            Err(bad(command, format!("`id` is the step reference {r:?}, which only `run` and `command_batch` resolve")))
+        }
+        Some(v) => Ok(Some(vec![checked_id(s, v, command)?])),
+    }
+}
 /// `v` as an index, a count or a byte offset: past `usize::MAX` reads as `usize::MAX`, past the end of any list.
 pub(crate) fn index(v: u64) -> usize {
     usize::try_from(v).unwrap_or(usize::MAX)

crates/engine/src/cmd/paint.rs

@@ -684,7 +684,7 @@ fn default_paint(s: &mut Session, p: &Value) -> Result<Value> {
 /// active item of the proxy's kind is recoloured in each selected object's own stack instead.
 fn proxy_recolor(s: &mut Session, p: &Value, cmd: &str, label: &str, f: &dyn Fn(Color) -> Color) -> Result<Value> {
     let stroke = stroke_param(s, p, cmd)?;
-    let ids = match ids_param(p, "ids") {
+    let ids = match super::given_objects(s, p, cmd)? {
         Some(ids) => ids,
         None => super::edit::selected_roots(s)?,
     };

crates/engine/src/cmd/params.rs

@@ -161,6 +161,32 @@ pub(crate) fn takes_key(doc: &'static str, key: &str) -> bool {
     doc_keys(doc).is_ok_and(|parsed| parsed.keys.contains(&key)) || accepted(doc, 0).is_some_and(|keys| keys.contains(&key))
 }
 
+/// Does the description `doc` take `id` as `ids` of one object (`{ids?|id?}`, `{id?, ids?: …
+/// (instead of id)}`, `{id} or {ids: […]}`): it names both keys, and no params object of it names
+/// `ids` beside a required `id`, which is something else (`plugin.run {id, ids?}`: the plug-in's)?
+pub(crate) fn id_stands_for_ids(doc: &'static str) -> bool {
+    fn head(alternative: &str) -> &str {
+        alternative.split([':', '=', '(']).next().unwrap_or_default().trim()
+    }
+    if !takes_key(doc, "ids") || !takes_key(doc, "id") {
+        return false;
+    }
+    let mut rest = doc;
+    while let Ok((body, after)) = object_body(rest) {
+        let entries = split_top(body, ',');
+        let required_id = entries.iter().any(|e| split_top(e, '|').len() == 1 && head(e) == "id");
+        let names_ids = entries.iter().any(|e| split_top(e, '|').iter().any(|a| head(a).trim_end_matches('?').trim_end() == "ids"));
+        if required_id && names_ids {
+            return false;
+        }
+        match after.trim_start().strip_prefix("or ").or_else(|| after.trim_start().strip_prefix("| ")).filter(|r| r.starts_with('{')) {
+            Some(next) => rest = next,
+            None => break,
+        }
+    }
+    true
+}
+
 /// The edits (insert, delete, replace, swap of neighbours) that turn `a` into `b`, ignoring case.
 fn distance(a: &str, b: &str) -> usize {
     let (a, b): (Vec<char>, Vec<char>) = (a.chars().map(|c| c.to_ascii_lowercase()).collect(), b.chars().map(|c| c.to_ascii_lowercase()).collect());

crates/engine/src/lib.rs

@@ -1760,13 +1760,25 @@ impl Session {
         // passes that its command's description doesn't name fails the test.
         #[cfg(test)]
         self.check_params(id, params)?;
+        // Objects given as `id` (where it stands for `ids` of one) or as `ids` beside it are checked
+        // before anything else: a malformed one, or one naming no object, is an error under this
+        // command's id, never the selection in its place. `effect.apply {id: "stylize.dropShadow"}`
+        // without `effect` names the effect (its older key), not an object.
+        let effect_named = id == "effect.apply"
+            && params.get("effect").is_none()
+            && params.get("ids").is_none_or(Value::is_null)
+            && params.get("id").and_then(Value::as_str).is_some_and(|e| !e.starts_with('$'));
+        if cmd::params::id_stands_for_ids(spec.params) && !effect_named && self.active().is_some() {
+            cmd::given_objects(self, params, id)?;
+        }
         // Clear's menu needs a selection; an agent supplying explicit targets only needs a
         // document. The command validates those targets before making any edit.
+        let given = given_ids(spec.params, params);
         let enabled = if id == "edit.clear" && params.get("ids").is_some() {
             cmd::has_doc(self)
-        } else if let Some(ids) = given_ids(spec.params, params) {
-            // Objects given as `ids` stand in for the selection: the command is enabled as it is
-            // with them selected (#1575).
+        } else if let Some(ids) = given.clone() {
+            // Objects given as `ids` (or `id`) stand in for the selection: the command is enabled
+            // as it is with them selected (#1575).
             self.enabled_with_selection(spec, ids)
         } else {
             (spec.enabled)(self)
@@ -1782,13 +1794,18 @@ impl Session {
             self.journal_query = false;
             self.batch_stash = None;
         }
-        // A selection-only command given `ids` selects them first, in the same undo step (#2130);
-        // when the command then fails, the selection (and the current layer selecting them made
-        // current) goes back to how it was, as when the ids are refused.
-        let select_first = SELECTS_GIVEN_IDS.contains(&id).then(|| params.get("ids")).flatten();
-        let selected_before = select_first.and(self.active()).map(|d| (d.uid, d.selection.clone(), d.active_layer));
+        // A selection-only command given `ids` (or `id`) selects them first, in the same undo step
+        // (#2130); when the command then fails, the selection (and the current layer selecting
+        // them made current) goes back to how it was, as when the ids are refused.
+        let select_first = SELECTS_GIVEN_IDS
+            .contains(&id)
+            .then(|| {
+                params.get("ids").filter(|v| !v.is_null()).cloned().or_else(|| given.map(|ids| ids.iter().map(|i| i.0).collect::<Vec<_>>().into()))
+            })
+            .flatten();
+        let selected_before = select_first.as_ref().and(self.active()).map(|d| (d.uid, d.selection.clone(), d.active_layer));
         let run = |s: &mut Session| {
-            if let Some(ids) = select_first {
+            if let Some(ids) = &select_first {
                 s.execute("select.set", &serde_json::json!({ "ids": ids }))?;
             }
             let r = (spec.run)(s, params);
@@ -2784,8 +2801,16 @@ pub(crate) const SELECTS_GIVEN_IDS: &[&str] = &[
 ];
 
 /// The non-empty `ids` given to a command whose params description names `ids` as a key (the
-/// key callers outside the app may pass it, [`cmd::params::takes_key`]).
+/// key callers outside the app may pass it, [`cmd::params::takes_key`]), or its `id` as `ids` of
+/// one when no `ids` are given and the description takes `id` so
+/// ([`cmd::params::id_stands_for_ids`]), as [`cmd::targets`] reads them.
 fn given_ids(description: &'static str, params: &Value) -> Option<Vec<NodeId>> {
-    let ids = cmd::ids_param(params, "ids").filter(|ids| !ids.is_empty())?;
-    cmd::params::takes_key(description, "ids").then_some(ids)
+    if !cmd::params::takes_key(description, "ids") {
+        return None;
+    }
+    match params.get("ids").filter(|v| !v.is_null()) {
+        Some(_) => cmd::ids_param(params, "ids"),
+        None => cmd::id_param(params, "id").filter(|_| cmd::params::id_stands_for_ids(description)).map(|id| vec![id]),
+    }
+    .filter(|ids| !ids.is_empty())
 }

crates/engine/src/tests_ids_without_selection.rs

@@ -53,6 +53,139 @@ fn only_an_ids_key_counts() {
     // A word in what the description says about another key isn't the key.
     assert_eq!(given_ids("{name: of the swatch the ids of the selection take}", &p), None);
     assert_eq!(given_ids("{ids?: [id]}", &json!({"ids": []})), None);
+    // `id` is `ids` of one where the description names both keys, and `ids` aren't given.
+    let one = json!({"id": 3});
+    assert_eq!(given_ids("{ids?|id?}", &one), Some(vec![NodeId(3)]));
+    assert_eq!(given_ids("{id?, ids?: [id…] (instead of id)}", &one), Some(vec![NodeId(3)]));
+    assert_eq!(given_ids("{ids?: [id…]}", &one), None, "a description without `id`");
+    assert_eq!(given_ids("{id?: dimension id}", &one), None, "a description without `ids`");
+    assert_eq!(given_ids("{ids?|id?}", &json!({"id": "a1"})), None, "not an object id");
+    assert_eq!(given_ids("{ids?|id?}", &json!({"id": 3, "ids": [4]})), Some(vec![NodeId(4)]), "`ids` first");
+    assert_eq!(given_ids("{ids?|id?}", &json!({"id": 3, "ids": null})), Some(vec![NodeId(3)]));
+    // A required `id` beside `ids` is something else (`plugin.run`'s plug-in); `{id} or {ids}` is
+    // one object or several.
+    assert_eq!(given_ids("{id, params?: {…}, ids?: [..]}", &one), None, "a plug-in's id");
+    assert!(!cmd::params::id_stands_for_ids("{id, params?: {…}, ids?: [..]}"));
+    assert!(cmd::params::id_stands_for_ids("{id} or {ids: [id…]}: toggles them"));
+    assert!(cmd::params::id_stands_for_ids("{ids|id, target}"));
+}
+
+/// `{"id": x}` with params besides it, and the same as `{"ids": [x]}`.
+fn id_and_ids(cmd: &str, x: u64, d: u64) -> (serde_json::Value, serde_json::Value) {
+    let (mut one, mut list) = (params_for(cmd, d), params_for(cmd, d));
+    one["id"] = json!(x);
+    list["ids"] = json!([x]);
+    (one, list)
+}
+
+/// [`doc_with_nested_groups`] with point type `Ab` added, nothing selected → its session and the
+/// type's id.
+fn doc_with_type() -> (Session, [u64; 4], u64) {
+    let (mut s, ids) = doc_with_nested_groups();
+    let t = s.execute("text.create", &json!({"x": 10, "y": 120, "text": "Ab", "size": 20})).unwrap()["id"].as_u64().unwrap();
+    s.execute("select.none", &json!({})).unwrap();
+    (s, ids, t)
+}
+
+/// What `cmd` gives in a fresh [`doc_with_type`] (nothing selected) with `params`: its result or
+/// error, and the document after it.
+fn run_fresh(
+    cmd: &str,
+    params: &serde_json::Value,
+) -> (std::result::Result<serde_json::Value, String>, Option<std::sync::Arc<vectorcraft_doc::Document>>) {
+    let (mut s, ..) = doc_with_type();
+    let r = s.execute(cmd, params).map_err(|e| e.to_string());
+    (r, s.doc().ok().map(|d| d.doc.clone()))
+}
+
+/// Every command whose params description names both `id` and `ids` (`{ids?|id?}`, `{id?, ids?:
+/// … (instead of id)}`), with nothing selected: `id` does what `ids` of that one object does (the
+/// same result or the same error, and the same document), for objects of the document and for an
+/// id the document doesn't have. Before, the commands that need a selection refused `id` as
+/// "nothing selected" and took `ids`.
+#[test]
+fn id_does_what_ids_of_one_does_with_nothing_selected() {
+    let (fresh, [a, inner, _, d], t) = doc_with_type();
+    let (mut checked, mut failures, mut refused, mut refused_ran) = (vec![], vec![], vec![], vec![]);
+    for spec in command_specs() {
+        if !cmd::params::id_stands_for_ids(spec.params) {
+            continue;
+        }
+        // Those the menus refuse with nothing selected are the ones that refused `id`.
+        if (spec.enabled)(&fresh).is_err() {
+            refused.push(spec.id);
+        }
+        // A rectangle, a group, type, and an id the document doesn't have.
+        for x in [a, inner, t, 999_999] {
+            let (one, list) = id_and_ids(spec.id, x, d);
+            let (with_id, with_ids) = (run_fresh(spec.id, &one), run_fresh(spec.id, &list));
+            if with_id != with_ids {
+                failures.push(format!("{} {x}: with id {:.160}; with ids {:.160}", spec.id, format!("{:?}", with_id.0), format!("{:?}", with_ids.0)));
+            } else if refused.last() == Some(&spec.id) && x != 999_999 && with_id.0.is_ok() && !refused_ran.contains(&spec.id) {
+                refused_ran.push(spec.id);
+            }
+        }
+        checked.push(spec.id);
+    }
+    assert!(failures.is_empty(), "{} differ:\n{}", failures.len(), failures.join("\n"));
+    // The commands that refused `id` with nothing selected (29), and those of them that run on
+    // these objects (14: the rest need other art, as with `ids`).
+    assert!(refused.len() >= 26 && refused_ran.len() >= 14, "{refused_ran:?} ran of {refused:?}; checked {}", checked.len());
+}
+
+/// Every command that takes `id` as `ids` of one ([`cmd::params::id_stands_for_ids`]), with an
+/// object selected: an `id` that is malformed (a string, negative, a fraction, an object) or names
+/// no object is refused under the command's id, as the same value in `ids` is, and the document
+/// and the selection stay as they were. It never stands for the selection instead. (A string `id`
+/// given to `effect.apply` without `effect` names the effect, which "bad" isn't: refused too.)
+#[test]
+fn a_bad_id_is_refused_as_the_same_ids_are_and_changes_nothing() {
+    let (_, [.., d], _) = doc_with_type();
+    let (mut failures, mut checked) = (vec![], 0);
+    for spec in command_specs() {
+        if !cmd::params::id_stands_for_ids(spec.params) {
+            continue;
+        }
+        checked += 1;
+        for bad in [json!("bad"), json!(-1), json!(1.5), json!({"id": 1}), json!(999_999)] {
+            let run = |key: &str, value: serde_json::Value| {
+                let (mut s, ..) = doc_with_type();
+                s.execute("select.set", &json!({ "ids": [d] })).unwrap();
+                let before = s.doc().unwrap().doc.clone();
+                let mut p = params_for(spec.id, d);
+                p[key] = value;
+                let r = s.execute(spec.id, &p).map_err(|e| e.to_string());
+                let st = s.doc().unwrap();
+                (r, st.doc == before && st.selection.objects == vec![NodeId(d)])
+            };
+            let (with_id, with_ids) = (run("id", bad.clone()), run("ids", json!([bad.clone()])));
+            let effect_named = spec.id == "effect.apply" && bad.is_string();
+            if with_id.0.is_ok() || !with_id.1 || (with_id != with_ids && !effect_named) {
+                failures.push(format!(
+                    "{} {bad}: with id {:.160} (unchanged: {}); with ids {:.160}",
+                    spec.id,
+                    format!("{:?}", with_id.0),
+                    with_id.1,
+                    format!("{:?}", with_ids.0)
+                ));
+            }
+        }
+        // A step reference no batch resolved is refused under this command's id too.
+        let (mut s, ..) = doc_with_type();
+        s.execute("select.set", &json!({ "ids": [d] })).unwrap();
+        let before = s.doc().unwrap().doc.clone();
+        let mut p = params_for(spec.id, d);
+        p["id"] = json!("$missing");
+        match s.execute(spec.id, &p) {
+            Err(EngineError::BadParams { cmd, msg }) if cmd == spec.id && msg.contains("step reference") => {}
+            r => failures.push(format!("{} \"$missing\": {r:?}", spec.id)),
+        }
+        if s.doc().unwrap().doc != before {
+            failures.push(format!("{} \"$missing\": the document changed", spec.id));
+        }
+    }
+    assert!(failures.is_empty(), "{} fail:\n{}", failures.len(), failures.join("\n"));
+    assert!(checked >= 100, "{checked} commands");
 }
 
 /// Every command that names `ids` and is refused with nothing selected: given the object as `ids`

docs/mcp.md

@@ -347,8 +347,12 @@ currently describes them in prose, so MCP does not guess schemas or silently rem
 
 Coordinates are points in document space: y points down, the origin is the first artboard's top-left, and a new
 document is 612 × 792 (US Letter). New objects become the selection. Most commands act on the selection or on
-explicit `ids`. A command that leaves out an object selected inside a selected group also leaves it out when `ids`
-list it together with a group that holds it.
+explicit `ids`, which stand in for the selection, so they work with nothing selected. Where a command's params say
+`ids?|id?` (or `id?, ids?: … (instead of id)`), `id` is `ids` of one object: `{"id": 7}` does what `{"ids": [7]}`
+does, with or without a selection, and an `id` or `ids` that is malformed (not a non-negative integer) or names no
+object is refused under the command's name, never replaced by the selection (`effect.apply {"id": "warp.arc"}`
+without `effect` names the effect). A command that leaves out an object selected inside a selected group also
+leaves it out when `ids` list it together with a group that holds it.
 
 Paint values (`fill`, `stroke`) accept `"#rrggbb"`, `"none"`, `[r,g,b]` (0..1), `{"c","m","y","k"}`, `{"gray"}`,
 or a full `paint.setFill` params object (`{"gradient": …}`, `{"swatch": "name"}`).