Merge pull request #1577 from switch87/fix/ids-without-selection

Commands: objects given as ids stand in for the selection

crates/engine/src/cmd/params.rs

@@ -150,6 +150,12 @@ fn accepted(doc: &'static str, depth: usize) -> Option<Vec<&'static str>> {
     Some(keys)
 }
 
+/// Does the description `doc` name `key` as a parameter (its own keys, and those of the commands
+/// its `…` entries name)?
+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))
+}
+
 /// 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

@@ -1300,6 +1300,17 @@ impl Session {
         true
     }
 
+    /// `spec`'s `enabled` with `ids` selected instead of the selection, which is then put back.
+    fn enabled_with_selection(&mut self, spec: &CommandSpec, ids: Vec<NodeId>) -> std::result::Result<(), String> {
+        let Some(d) = self.active_mut() else { return (spec.enabled)(self) };
+        let old = std::mem::replace(&mut d.selection, Selection { objects: ids, ..Selection::default() });
+        let r = (spec.enabled)(self);
+        if let Some(d) = self.active_mut() {
+            d.selection = old;
+        }
+        r
+    }
+
     /// Execute a command by id. This is THE entry point for every frontend.
     pub fn execute(&mut self, id: &str, params: &Value) -> Result<Value> {
         let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?;
@@ -1309,7 +1320,15 @@ impl Session {
         cmd::params::check_params(id, params)?;
         // 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 enabled = if id == "edit.clear" && params.get("ids").is_some() { cmd::has_doc(self) } else { (spec.enabled)(self) };
+        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).
+            self.enabled_with_selection(spec, ids)
+        } else {
+            (spec.enabled)(self)
+        };
         if let Err(why) = enabled {
             return Err(EngineError::Disabled(id.to_string(), why));
         }
@@ -1770,6 +1789,8 @@ mod tests_halftone;
 #[cfg(test)]
 mod tests_history_mem;
 #[cfg(test)]
+mod tests_ids_without_selection;
+#[cfg(test)]
 mod tests_inline;
 #[cfg(test)]
 mod tests_journal;
@@ -1955,3 +1976,10 @@ mod tests_widthprofiles;
 mod tests_widthtool;
 #[cfg(test)]
 mod tests_xform;
+
+/// 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`]).
+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)
+}

crates/engine/src/tests_ids_without_selection.rs

@@ -0,0 +1,83 @@
+//! #1575: commands that take `ids` work on the objects given, with nothing selected; the
+//! selection is left as it was.
+
+use serde_json::json;
+
+use super::*;
+
+fn doc_with_rect() -> (Session, u64) {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"width": 200, "height": 200})).unwrap();
+    let id = s.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 50, "height": 40})).unwrap()["id"].as_u64().unwrap();
+    s.execute("select.none", &json!({})).unwrap();
+    (s, id)
+}
+
+#[test]
+fn ids_stand_in_for_the_selection() {
+    for (cmd, params) in [
+        ("object.repeat.radial", json!({})),
+        ("edit.colors.invert", json!({})),
+        ("object.pattern.make", json!({"name": "P"})),
+        ("symbol.new", json!({"name": "S"})),
+        ("object.mesh.create", json!({})),
+        ("object.makePixelPerfect", json!({})),
+    ] {
+        let (mut s, id) = doc_with_rect();
+        let mut p = params;
+        p["ids"] = json!([id]);
+        let r = s.execute(cmd, &p);
+        assert!(r.is_ok(), "{cmd}: {r:?}");
+    }
+}
+
+#[test]
+fn the_selection_is_put_back_and_the_check_still_applies() {
+    let (mut s, id) = doc_with_rect();
+    // Disabled even with the object given: Make with Top Object needs two.
+    let r = s.execute("object.envelope.makeWithTopObject", &json!({"ids": [id]}));
+    assert!(matches!(r, Err(EngineError::Disabled(..))), "{r:?}");
+    assert!(s.doc().unwrap().selection.objects.is_empty(), "the stand-in selection is put back");
+    // Without ids, nothing selected is still nothing selected.
+    let r = s.execute("object.repeat.radial", &json!({}));
+    assert!(matches!(r, Err(EngineError::Disabled(..))), "{r:?}");
+}
+
+#[test]
+fn only_an_ids_key_counts() {
+    let p = json!({"ids": [3]});
+    assert_eq!(given_ids("{ids?: [id]}", &p), Some(vec![NodeId(3)]));
+    assert_eq!(given_ids("{id?|ids?, name}", &p), Some(vec![NodeId(3)]));
+    assert_eq!(given_ids("{grids: n, guides: bool}", &p), None);
+    // 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);
+}
+
+/// Every command that names `ids` and is refused with nothing selected: given the object as `ids`
+/// it does what it does with the object selected (the same result or the same error, and the same
+/// document), so no command is let through to run on an empty selection.
+#[test]
+fn ids_do_what_the_selection_does_in_every_command_that_takes_them() {
+    let mut checked = vec![];
+    for spec in command_specs() {
+        let (mut given, id) = doc_with_rect();
+        if !cmd::params::takes_key(spec.params, "ids") || (spec.enabled)(&given).is_ok() {
+            continue;
+        }
+        let (mut selected, _) = doc_with_rect();
+        selected.execute("select.set", &json!({"ids": [id]})).unwrap();
+        let (a, b) = (given.execute(spec.id, &json!({"ids": [id]})), selected.execute(spec.id, &json!({})));
+        match (&a, &b) {
+            (Ok(_), Ok(_)) => {}
+            (Err(x), Err(y)) => assert_eq!(x.to_string(), y.to_string(), "{}", spec.id),
+            _ => panic!("{}: with ids {a:?}, with the selection {b:?}", spec.id),
+        }
+        match (given.doc(), selected.doc()) {
+            (Ok(g), Ok(s)) => assert!(g.doc == s.doc, "{}: the documents differ", spec.id),
+            (g, s) => assert_eq!(g.is_ok(), s.is_ok(), "{}", spec.id),
+        }
+        checked.push((spec.id, a.is_ok()));
+    }
+    assert!(checked.iter().filter(|c| c.1).count() >= 20, "{checked:?}");
+}