Merge pull request #795 from DWAA1660/fix/selection-validation-and-clear

Validate selection IDs and clear exact compound members

crates/engine/src/cmd/edit.rs

@@ -1,6 +1,6 @@
 //! File (document-level) and Edit commands, plus document queries.
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, BTreeSet};
 use std::sync::Arc;
 
 use serde_json::{Value, json};
@@ -250,29 +250,39 @@ fn cut(s: &mut Session, p: &Value) -> Result<Value> {
     if s.tool_id() == "artboard" {
         return super::panelcmds::artboard_copy(s, &json!({}), true);
     }
+    // Copy promotes a compound member to its compound. Cut must remove the same objects it
+    // copied, while Clear on its own deletes exactly the selected members.
+    let ids = selected_roots(s)?;
     copy(s, p)?;
-    clear(s, &json!({}))
+    if s.doc()?.selection.anchors.is_empty() { clear(s, &json!({ "ids": ids })) } else { clear(s, &json!({})) }
 }
 
 fn clear(s: &mut Session, p: &Value) -> Result<Value> {
+    let explicit = if p.get("ids").is_some() { Some(checked_ids_param(s, p, "ids", "edit.clear")?) } else { None };
     // Direct-selected anchors: delete those anchors instead of whole objects.
-    if ids_param(p, "ids").is_none() && !s.doc()?.selection.anchors.is_empty() {
+    if explicit.is_none() && !s.doc()?.selection.anchors.is_empty() {
         return super::path::delete_anchors(s, p);
     }
     // Selected ruler guides (selected on their own).
-    if ids_param(p, "ids").is_none() && s.doc()?.selection.is_empty() {
+    if explicit.is_none() && s.doc()?.selection.is_empty() {
         return super::docmenu::guide_remove(s, &json!({}));
     }
-    let ids = match ids_param(p, "ids") {
-        Some(v) => v,
-        None => selected_roots(s)?,
-    };
+    let st = s.doc()?;
+    let targets = explicit.unwrap_or_else(|| st.selection.in_paint_order(&st.doc));
+    // Keep the selected ids themselves, without promoting compound members. Delete a selected
+    // ancestor just once; its selected descendants leave with it. Layers themselves are kept.
+    let targets: BTreeSet<_> = targets.into_iter().filter(|id| st.doc.node(*id).is_some_and(|n| !n.is_layer())).collect();
+    let ids: Vec<_> = targets
+        .iter()
+        .copied()
+        .filter(|id| !st.doc.ancestry(*id).unwrap_or_default().iter().rev().skip(1).any(|ancestor| targets.contains(ancestor)))
+        .collect();
+    if ids.is_empty() {
+        return ok();
+    }
     s.edit("Clear", |d, sel| {
         for id in &ids {
-            if d.node(*id).is_some_and(|n| n.is_layer()) {
-                continue;
-            }
-            let _ = d.remove(*id);
+            d.remove(*id)?;
         }
         sel.clear();
         Ok(())

crates/engine/src/cmd/mod.rs

@@ -304,6 +304,21 @@ 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())
 }
+/// Validate all explicit targets before a command changes the document or selection.
+pub(crate) fn checked_ids_param(s: &Session, p: &Value, key: &str, command: &str) -> Result<Vec<NodeId>> {
+    let values =
+        p.get(key).and_then(Value::as_array).ok_or_else(|| bad(command, format!("{key} must be an array of non-negative integer object ids")))?;
+    values.iter().map(|value| checked_id(s, value, command)).collect()
+}
+
+pub(crate) fn checked_id(s: &Session, value: &Value, command: &str) -> Result<NodeId> {
+    let id = value.as_u64().map(NodeId).ok_or_else(|| bad(command, format!("invalid object id {value}: expected a non-negative integer")))?;
+    if s.doc()?.doc.node(id).is_none() {
+        return Err(bad(command, format!("no such object id {value}")));
+    }
+    Ok(id)
+}
+
 pub(crate) fn point_param(p: &Value, key: &str) -> Option<Point> {
     let a = p.get(key)?.as_array()?;
     Some(Point::new(a.first()?.as_f64()?, a.get(1)?.as_f64()?))

crates/engine/src/cmd/select.rs

@@ -25,14 +25,30 @@ pub fn specs() -> Vec<CommandSpec> {
         cmd!("select.inverse", "Inverse", ["Select"], None, "{}", has_doc, inverse),
         cmd!("select.nextAbove", "Next Object Above", ["Select"], Some("Cmd+Alt+]"), "{}", has_selection, |s, _| step(s, 1)),
         cmd!("select.nextBelow", "Next Object Below", ["Select"], Some("Cmd+Alt+["), "{}", has_selection, |s, _| step(s, -1)),
-        cmd!("select.set", "Select Objects", [], None, "{ids: [id…]}", has_doc, set),
-        cmd!("select.add", "Add to Selection", [], None, "{ids: [id…]}", has_doc, add),
+        cmd!(
+            "select.set",
+            "Select Objects",
+            [],
+            None,
+            "{ids: [id…]} all ids must be non-negative integers naming existing objects → {count, ids} (resulting selection)",
+            has_doc,
+            set
+        ),
+        cmd!(
+            "select.add",
+            "Add to Selection",
+            [],
+            None,
+            "{ids: [id…]} all ids must be non-negative integers naming existing objects → {count, ids} (resulting selection)",
+            has_doc,
+            add
+        ),
         cmd!(
             "select.toggle",
             "Toggle Selection",
             [],
             None,
-            "{id} or {ids: [id…]}: each selected one leaves the selection, the others join it",
+            "{id} or {ids: [id…]}: each selected one leaves the selection, the others join it; all ids must be non-negative integers naming existing objects → {count, ids} (resulting selection)",
             has_doc,
             toggle
         ),
@@ -225,29 +241,39 @@ fn step(s: &mut Session, dir: i64) -> Result<Value> {
 }
 
 fn set(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").ok_or_else(|| bad("select.set", "missing ids"))?;
-    s.select(|d, sel| sel.set(ids.iter().copied().filter(|i| d.node(*i).is_some())))?;
-    ok()
+    let ids = checked_ids_param(s, p, "ids", "select.set")?;
+    s.select(|_, sel| sel.set(ids.iter().copied()))?;
+    selection_result(s)
 }
 
 fn add(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").ok_or_else(|| bad("select.add", "missing ids"))?;
+    let ids = checked_ids_param(s, p, "ids", "select.add")?;
     s.select(|_, sel| {
         for i in ids {
             sel.add(i);
         }
     })?;
-    ok()
+    selection_result(s)
 }
 
 fn toggle(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").or_else(|| id_param(p, "id").map(|id| vec![id])).ok_or_else(|| bad("select.toggle", "missing id or ids"))?;
+    let ids = if p.get("ids").is_some() {
+        checked_ids_param(s, p, "ids", "select.toggle")?
+    } else {
+        let value = p.get("id").ok_or_else(|| bad("select.toggle", "missing id or ids"))?;
+        vec![checked_id(s, value, "select.toggle")?]
+    };
     s.select(|_, sel| {
         for id in ids {
             sel.toggle(id);
         }
     })?;
-    ok()
+    selection_result(s)
+}
+
+fn selection_result(s: &Session) -> Result<Value> {
+    let selection = &s.doc()?.selection;
+    Ok(json!({ "count": selection.len(), "ids": selection.objects }))
 }
 
 fn key(s: &mut Session, p: &Value) -> Result<Value> {