Merge pull request #1674 from turtwig/fix/delete-selected-anchors

Delete removes every selected anchor and keeps the remainder selected

crates/engine/src/cmd/edit.rs

@@ -128,7 +128,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Clear",
             ["Edit"],
             Some("Delete"),
-            "{ids?} delete the objects `ids`, else the selection (direct-selected anchors, else objects, else ruler guides)",
+            "{ids?} delete the objects `ids`, else the selection (fully selected objects and direct-selected anchors together, else ruler guides)",
             has_selection_or_guides,
             clear
         ),
@@ -368,7 +368,7 @@ fn cut(s: &mut Session, p: &Value) -> Result<Value> {
 
 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 or segments: remove those components instead of whole objects.
+    // Direct-selected anchors or segments: remove those, and any objects selected whole with them.
     if explicit.is_none() && !s.doc()?.selection.anchors.is_empty() {
         return super::path::delete_anchors(s, p);
     }

crates/engine/src/cmd/path.rs

@@ -91,7 +91,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Delete Anchor Points",
             [],
             None,
-            "{} delete the direct-selected anchors and their segments, opening closed paths there (the Delete key)",
+            "{} delete the direct-selected anchors and their segments, opening closed paths there, and delete objects selected whole alongside them; what remains of a path stays selected, so a second Delete removes it (the Delete key)",
             has_anchors,
             delete_anchors
         ),
@@ -521,10 +521,27 @@ pub(crate) fn delete_anchors(s: &mut Session, _: &Value) -> Result<Value> {
     let st = s.doc()?;
     let sel = st.selection.anchors.clone();
     let segments = st.selection.segments.clone();
+    // Every anchor selected stores the path as an object, with no `anchors` entry. A marquee that
+    // also catches part of another path must still delete those whole objects.
+    let whole = st.selection.whole();
     s.edit("Clear", |d, selection| {
+        let targets: BTreeSet<NodeId> = whole.into_iter().filter(|id| d.node(*id).is_some_and(|n| !n.is_layer())).collect();
+        let whole_ids: Vec<NodeId> = targets
+            .iter()
+            .copied()
+            .filter(|id| !d.ancestry(*id).unwrap_or_default().iter().rev().skip(1).any(|ancestor| targets.contains(ancestor)))
+            .collect();
+        if !whole_ids.is_empty() {
+            d.remove_many(&whole_ids)?;
+        }
         let mut survivors = vec![];
-        let mut emptied = Beside::replacing(d, &sel.keys().copied().collect::<Vec<_>>());
+        let remaining: Vec<NodeId> = sel.keys().copied().filter(|id| d.node(*id).is_some()).collect();
+        let mut emptied = Beside::replacing(d, &remaining);
         for (id, set) in &sel {
+            // The path went with a fully selected group around it.
+            if d.node(*id).is_none() {
+                continue;
+            }
             let remove_node = {
                 let path = path_mut(d, *id)?;
                 if let Some(refs) = segments.get(id) {
@@ -538,39 +555,73 @@ pub(crate) fn delete_anchors(s: &mut Session, _: &Value) -> Result<Value> {
                         })
                         .collect();
                 } else {
-                    // Deleting an anchor from a closed path opens it there (Illustrator behaviour on Delete).
-                    for &(si, ai) in set.iter().rev() {
-                        if let Some(sp) = path.subpaths.get_mut(si)
-                            && ai < sp.anchors.len()
-                        {
-                            if sp.closed {
-                                sp.anchors.rotate_left(ai + 1);
-                                sp.anchors.pop();
-                                sp.closed = false;
-                            } else {
-                                sp.anchors.remove(ai);
-                            }
-                        }
-                    }
+                    // Remove every selected anchor against the original indexes. A closed path opens
+                    // across each deleted run and keeps the surviving stretches in their old order.
+                    path.subpaths = std::mem::take(&mut path.subpaths)
+                        .into_iter()
+                        .enumerate()
+                        .flat_map(|(si, sp)| {
+                            let gone = set.iter().filter_map(|&(s, ai)| (s == si).then_some(ai)).collect();
+                            without_anchors(sp, &gone)
+                        })
+                        .collect();
                 }
                 path.subpaths.retain(|s| s.anchors.len() >= 2);
                 path.subpaths.is_empty()
             };
             if remove_node {
                 emptied.put(d, *id, vec![])?;
-            } else if segments.contains_key(id) {
+            } else {
+                // The remainder is selected whole, so the next Delete removes it.
                 survivors.push(*id);
             }
         }
         emptied.finish(d)?;
-        // Another Delete removes the remaining path after a segment cut. Anchor Delete keeps
-        // its existing deselection behavior.
         selection.set(survivors);
         Ok(())
     })?;
     ok()
 }
 
+/// Remove anchors `removed`. An open path stays one path and joins across the gap. A closed path
+/// opens there: each stretch of anchors that were not deleted becomes its own open path, in the
+/// original order, including a stretch that wraps past the last anchor.
+fn without_anchors(sp: SubPath, removed: &BTreeSet<usize>) -> Vec<SubPath> {
+    let n = sp.anchors.len();
+    let gone: BTreeSet<usize> = removed.iter().copied().filter(|&i| i < n).collect();
+    if gone.is_empty() {
+        return vec![sp];
+    }
+    let keep = |anchors: Vec<Anchor>| (anchors.len() >= 2).then(|| SubPath::new(anchors, false));
+    if !sp.closed {
+        return keep(sp.anchors.into_iter().enumerate().filter(|(i, _)| !gone.contains(i)).map(|(_, a)| a).collect()).into_iter().collect();
+    }
+    let mut runs: Vec<Vec<Anchor>> = vec![];
+    let mut run = vec![];
+    for (i, anchor) in sp.anchors.into_iter().enumerate() {
+        if gone.contains(&i) {
+            if !run.is_empty() {
+                runs.push(std::mem::take(&mut run));
+            }
+        } else {
+            run.push(anchor);
+        }
+    }
+    // The anchors before the first deletion and after the last are one stretch.
+    if gone.contains(&0) {
+        if !run.is_empty() {
+            runs.push(run);
+        }
+    } else if !runs.is_empty() {
+        let head = runs.remove(0);
+        run.extend(head);
+        runs.insert(0, run);
+    } else if !run.is_empty() {
+        runs.push(run);
+    }
+    runs.into_iter().filter_map(keep).collect()
+}
+
 /// Remove connections, splitting open/closed contours into their surviving open runs.
 fn without_segments(sp: SubPath, removed: &BTreeSet<usize>) -> Vec<SubPath> {
     let cuts: BTreeSet<usize> = removed.iter().copied().filter(|&i| i < sp.segment_count()).collect();

crates/engine/tests/clear_commands.rs

@@ -17,6 +17,66 @@ fn compound() -> (Session, NodeId, NodeId, NodeId) {
     (s, a, b, c)
 }
 
+/// The two right-hand corners of a square are indexes 1 and 2. Delete must remove those corners
+/// and leave the left side, not a diagonal from shifting indexes as the path opens.
+#[test]
+fn clear_deletes_every_selected_anchor_of_a_closed_path() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    let id = NodeId(s.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 30, "height": 30})).unwrap()["id"].as_u64().unwrap());
+    let points = |s: &Session| {
+        s.active().unwrap().doc.node(id).unwrap().path_data().unwrap().subpaths[0].anchors.iter().map(|a| (a.p.x, a.p.y)).collect::<Vec<_>>()
+    };
+    let before = points(&s);
+    s.execute("select.anchors", &json!({"id": id.0, "anchors": [[0, 1], [0, 2]]})).unwrap();
+    s.execute("edit.clear", &json!({})).unwrap();
+    let sp = &s.active().unwrap().doc.node(id).unwrap().path_data().unwrap().subpaths[0];
+    assert!(!sp.closed);
+    assert_eq!(points(&s), vec![before[3], before[0]]);
+    assert_eq!(s.active().unwrap().selection.objects, vec![id]);
+    assert!(s.active().unwrap().selection.anchors.is_empty());
+    s.execute("edit.clear", &json!({})).unwrap();
+    assert!(s.active().unwrap().doc.node(id).is_none(), "a second Delete removes the remainder");
+    s.execute("edit.undo", &json!({})).unwrap();
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert_eq!(points(&s), before);
+    // One corner still opens the square into the other three, in order.
+    s.execute("select.anchors", &json!({"id": id.0, "anchors": [[0, 1]]})).unwrap();
+    s.execute("edit.clear", &json!({})).unwrap();
+    assert_eq!(points(&s), vec![before[2], before[3], before[0]]);
+}
+
+/// A marquee that covers one path completely and only some anchors of another stores the first as
+/// a whole object. Delete removes that object and the selected anchors of the other.
+#[test]
+fn clear_deletes_fully_selected_objects_along_with_partial_anchors() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    let mut rectangle =
+        |x| NodeId(s.execute("shape.rectangle", &json!({"x": x, "y": 10, "width": 30, "height": 30})).unwrap()["id"].as_u64().unwrap());
+    let whole = rectangle(10);
+    let partial = rectangle(80);
+    let refs = |s: &Session, id| {
+        s.active().unwrap().doc.node(id).unwrap().path_data().unwrap().anchors().map(|(si, ai, _)| vec![si, ai]).collect::<Vec<_>>()
+    };
+    let all = refs(&s, whole);
+    let one = refs(&s, partial)[0].clone();
+    let before = refs(&s, partial).len();
+    s.execute("select.anchorsMany", &json!({"items": [{"id": whole.0, "anchors": all}, {"id": partial.0, "anchors": [one]}]})).unwrap();
+    assert!(s.active().unwrap().selection.contains(whole) && s.active().unwrap().selection.partial(whole).is_none());
+    s.execute("edit.clear", &json!({})).unwrap();
+    assert!(s.active().unwrap().doc.node(whole).is_none());
+    assert_eq!(refs(&s, partial).len(), before - 1);
+    assert_eq!(s.active().unwrap().selection.objects, vec![partial]);
+    assert!(s.active().unwrap().selection.anchors.is_empty());
+    s.execute("edit.clear", &json!({})).unwrap();
+    assert!(s.active().unwrap().doc.node(partial).is_none(), "a second Delete removes what the first left");
+    s.execute("edit.undo", &json!({})).unwrap();
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert!(s.active().unwrap().doc.node(whole).is_some());
+    assert_eq!(refs(&s, partial).len(), before);
+}
+
 #[test]
 fn clear_selected_compound_member_preserves_its_sibling_and_is_undoable() {
     let (mut s, a, b, c) = compound();