Merge pull request #7249 from dotCooCoo/fix/cleanup-selection-one-pass

Artwork Cleanup: update the selected objects in one pass after each fix

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

@@ -369,11 +369,7 @@ fn restructure(d: &mut Document, scope: &mut Scope, kind: Kind) -> Result<Outcom
             break;
         }
         if let Scope::Objects(roots) = scope {
-            for (id, nodes) in &found {
-                if let Some(at) = roots.iter().position(|r| r == id) {
-                    roots.splice(at..=at, nodes.iter().map(|n| n.id));
-                }
-            }
+            *roots = replace_roots(roots, &found);
         }
         out.ids.extend(found.iter().map(|(id, _)| *id));
         d.insert_beside(found, true)?;
@@ -382,6 +378,36 @@ fn restructure(d: &mut Document, scope: &mut Scope, kind: Kind) -> Result<Outcom
     Ok(out)
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: how many scope roots [`replace_roots`] looked at on this thread.
+    pub(super) static ROOTS_SEEN: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
+}
+
+/// The scope's objects after a pass replaced `found`: each found object gives way to the ids of
+/// what replaces it, in its place, and the others keep their order. An id that `roots` lists more
+/// than once is replaced at its first places, once for each time `found` lists it. One walk of
+/// `roots`.
+pub(super) fn replace_roots(roots: &[NodeId], found: &[(NodeId, Vec<Node>)]) -> Vec<NodeId> {
+    let mut with: HashMap<NodeId, (&[Node], usize)> = HashMap::with_capacity(found.len());
+    for (id, nodes) in found {
+        with.entry(*id).or_insert((nodes.as_slice(), 0)).1 += 1;
+    }
+    let mut out = Vec::with_capacity(roots.len());
+    for r in roots {
+        #[cfg(test)]
+        ROOTS_SEEN.set(ROOTS_SEEN.get() + 1);
+        match with.get_mut(r) {
+            Some((nodes, left)) if *left > 0 => {
+                *left -= 1;
+                out.extend(nodes.iter().map(|n| n.id));
+            }
+            _ => out.push(*r),
+        }
+    }
+    out
+}
+
 /// Run `f` on a copy of each path in scope (not guides), and keep the copies it changed: `f`
 /// returns how many issues it fixed.
 fn reshape(d: &mut Document, scope: &Scope, f: impl Fn(&Node, &mut PathData) -> usize) -> Result<Outcome> {
@@ -479,13 +505,38 @@ fn near_points(d: &mut Document, scope: &Scope) -> Result<Outcome> {
 fn artboards(d: &mut Document, sel: &mut Selection, scope: &Scope) -> Result<Outcome> {
     let Scope::Document = scope else { return Ok(Outcome::default()) };
     let empty = empty_artboards(d);
-    // The last first, so the others keep their places.
-    for &i in empty.iter().rev() {
-        if i < d.artboards.len() {
-            let id = d.artboards.remove(i).id;
-            d.retire_artboard_id(id);
-            d.retain_guides(sel, |g| g.artboard != Some(id));
-        }
+    if empty.is_empty() {
+        return Ok(Outcome::default());
     }
+    // `empty` is in artboard order: one walk drops them and the others keep their order.
+    let mut gone: HashSet<u32> = HashSet::with_capacity(empty.len());
+    let mut i = 0;
+    d.artboards.retain(|a| {
+        let keep = empty.binary_search(&i).is_err();
+        i += 1;
+        if !keep {
+            gone.insert(a.id);
+        }
+        keep
+    });
+    count_artboard_pass();
+    for &id in &gone {
+        d.retire_artboard_id(id);
+    }
+    d.retain_guides(sel, |g| !g.artboard.is_some_and(|a| gone.contains(&a)));
+    count_artboard_pass();
     Ok(Outcome { count: empty.len(), ids: vec![], artboards: empty })
 }
+
+#[cfg(test)]
+thread_local! {
+    /// Tests: how many walks [`artboards`] made over the artboards and the ruler guides on this
+    /// thread.
+    pub(super) static ARTBOARD_PASSES: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
+}
+
+/// Tests: count one walk over the artboards or the ruler guides.
+fn count_artboard_pass() {
+    #[cfg(test)]
+    ARTBOARD_PASSES.set(ARTBOARD_PASSES.get() + 1);
+}

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

@@ -389,6 +389,150 @@ fn the_scope_is_the_selection_or_the_document() {
     }
 }
 
+/// The ids of the first layer's objects, bottom to top.
+fn layer_ids(s: &Session) -> Vec<NodeId> {
+    match &s.doc().unwrap().doc.layers[0].kind {
+        NodeKind::Layer { children, .. } => children.iter().map(|c| c.id).collect(),
+        other => panic!("not a layer: {other:?}"),
+    }
+}
+
+/// A plain group of the one object `id`.
+fn group(s: &mut Session, id: NodeId) -> NodeId {
+    wrap(s, id, |children| NodeKind::Group { children, clip: false })
+}
+
+#[test]
+fn unwrapping_selected_groups_keeps_their_places_and_order() {
+    let mut s = session();
+    let c1 = rect(&mut s, 10.0, 10.0, 20.0, 20.0);
+    let a = rect(&mut s, 40.0, 10.0, 20.0, 20.0);
+    let g1 = group(&mut s, a);
+    let c2 = rect(&mut s, 70.0, 10.0, 20.0, 20.0);
+    let empty = s
+        .edit("t", |d, _| {
+            let (parent, i, _) = d.position(c2).unwrap();
+            let g = d.alloc_id();
+            Ok(d.insert(parent, i + 1, Node::new(g, NodeKind::Group { children: vec![], clip: false }))?)
+        })
+        .unwrap();
+    let c3 = rect(&mut s, 100.0, 10.0, 20.0, 20.0);
+    let b = rect(&mut s, 130.0, 10.0, 20.0, 20.0);
+    let inner = group(&mut s, b);
+    let outer = group(&mut s, inner);
+    let c4 = rect(&mut s, 160.0, 10.0, 20.0, 20.0);
+    let x = rect(&mut s, 190.0, 10.0, 20.0, 20.0);
+    let unselected = group(&mut s, x);
+    assert_eq!(layer_ids(&s), [c1, g1, c2, empty, c3, outer, c4, unselected]);
+    select(&mut s, &[c1, g1, c2, empty, c3, outer, c4]);
+    let check = s.execute("addon.cleanup.check", &json!({"kinds": ["needlessGroups"], "scope": "selection"})).unwrap();
+    let r = s.execute("addon.cleanup.fix", &json!({"kinds": ["needlessGroups"], "scope": "selection"})).unwrap();
+    // The inner group is found only once its outer group is gone: on a later pass, as a root.
+    assert_eq!(r["fixed"]["needlessGroups"], 4, "{r}");
+    assert_eq!(check["total"], 4, "the check finds what Fix fixes: {check}");
+    let mut listed: Vec<u64> = check["findings"][0]["ids"].as_array().unwrap().iter().map(|v| v.as_u64().unwrap()).collect();
+    listed.sort_unstable();
+    let mut gone: Vec<u64> = [g1, empty, outer, inner].iter().map(|i| i.0).collect();
+    gone.sort_unstable();
+    assert_eq!(listed, gone);
+    assert_eq!(layer_ids(&s), [c1, a, c2, c3, b, c4, unselected], "each member takes its group's place");
+    for id in [g1, empty, outer, inner] {
+        assert!(node(&s, id).is_none(), "{id:?} unwrapped");
+    }
+}
+
+#[test]
+fn fix_all_reaches_the_members_of_unwrapped_selected_groups() {
+    let mut s = session();
+    rect(&mut s, 200.0, 20.0, 80.0, 60.0);
+    let below = rect(&mut s, 20.0, 150.0, 30.0, 30.0);
+    let copy = s
+        .edit("t", |d, _| {
+            let mut copy = d.node(below).unwrap().clone();
+            copy.id = d.alloc_id();
+            let (parent, i, _) = d.position(below).unwrap();
+            Ok(d.insert(parent, i + 1, copy)?)
+        })
+        .unwrap();
+    let g = group(&mut s, copy);
+    select(&mut s, &[below, g]);
+    // The copy sits apart from the object it doubles until its group is unwrapped.
+    assert_eq!(found(&mut s, "duplicateObjects", "selection"), 0);
+    assert_eq!(found(&mut s, "needlessGroups", "selection"), 1);
+    let r = s.execute("addon.cleanup.fix", &json!({"scope": "selection"})).unwrap();
+    assert_eq!((r["fixed"]["needlessGroups"].as_u64(), r["fixed"]["duplicateObjects"].as_u64()), (Some(1), Some(1)), "{r}");
+    assert!(node(&s, g).is_none() && node(&s, copy).is_none());
+    assert!(node(&s, below).is_some());
+}
+
+#[test]
+fn replacing_roots_takes_each_found_object_at_its_first_place() {
+    use super::checks::replace_roots;
+    let n = |id: u64| Node::new(NodeId(id), NodeKind::Group { children: vec![], clip: false });
+    let ids = |v: &[u64]| v.iter().map(|i| NodeId(*i)).collect::<Vec<_>>();
+    let roots = ids(&[1, 2, 1, 3]);
+    assert_eq!(replace_roots(&roots, &[(NodeId(1), vec![n(10), n(11)])]), ids(&[10, 11, 2, 1, 3]));
+    assert_eq!(replace_roots(&roots, &[(NodeId(1), vec![]), (NodeId(1), vec![])]), ids(&[2, 3]), "listed twice: both places");
+    assert_eq!(replace_roots(&roots, &[(NodeId(3), vec![]), (NodeId(2), vec![n(20)])]), ids(&[1, 20, 1]));
+    // A found object inside a root leaves the roots as they are.
+    assert_eq!(replace_roots(&roots, &[(NodeId(7), vec![n(70)])]), roots);
+    assert_eq!(replace_roots(&[], &[(NodeId(7), vec![])]), ids(&[]));
+}
+
+/// How many scope roots fixing `n` selected stray points above `n` selected clean objects looks at.
+fn roots_seen_removing(n: usize) -> usize {
+    use super::checks::ROOTS_SEEN;
+    let mut s = session();
+    let mut ids: Vec<NodeId> = (0..n).map(|i| rect(&mut s, (i % 20) as f64 * 15.0, (i / 20) as f64 * 15.0, 10.0, 10.0)).collect();
+    ids.extend((0..n).map(|i| path(&mut s, json!([{"x": i % 300, "y": i / 300}]), false)));
+    select(&mut s, &ids);
+    ROOTS_SEEN.set(0);
+    let r = s.execute("addon.cleanup.fix", &json!({"kinds": ["strayPoints"], "scope": "selection"})).unwrap();
+    assert_eq!(r["total"].as_u64(), Some(n as u64));
+    assert_eq!(layer_ids(&s), ids[..n]);
+    ROOTS_SEEN.get()
+}
+
+#[test]
+fn removing_many_selected_objects_looks_at_each_root_once_a_pass() {
+    let (small, large) = (roots_seen_removing(150), roots_seen_removing(300));
+    assert_eq!(small, 300, "one pass over the 300 roots");
+    assert!(large <= 2 * small, "{small} roots looked at for 150 removals, {large} for 300");
+}
+
+#[test]
+fn deleting_many_empty_artboards_walks_the_artboards_and_guides_once() {
+    use super::checks::ARTBOARD_PASSES;
+    let mut s = session();
+    rect(&mut s, 200.0, 20.0, 80.0, 60.0);
+    let (keep, gone) = s
+        .edit("t", |d, _| {
+            let first = d.artboards[0].id;
+            let keep = d.add_ruler_guide(&vectorcraft_doc::Guide { artboard: Some(first), ..vectorcraft_doc::Guide::new(true, 100.0) });
+            let mut gone = vec![];
+            for i in 0..40 {
+                let mut a = d.artboards[0].clone();
+                a.id = d.next_artboard_id();
+                let x = 1000.0 + i as f64 * 200.0;
+                a.rect = Rect::new(x, 1000.0, x + 100.0, 1100.0);
+                gone.push(d.add_ruler_guide(&vectorcraft_doc::Guide { artboard: Some(a.id), ..vectorcraft_doc::Guide::new(true, x + 50.0) }));
+                d.artboards.push(a);
+            }
+            Ok((keep, gone))
+        })
+        .unwrap();
+    let first = s.doc().unwrap().doc.artboards[0].clone();
+    ARTBOARD_PASSES.set(0);
+    let r = s.execute("addon.cleanup.fix", &json!({"kinds": ["emptyArtboards"], "scope": "document"})).unwrap();
+    assert_eq!(r["total"].as_u64(), Some(40), "{r}");
+    assert_eq!(ARTBOARD_PASSES.get(), 2, "one walk over the artboards and one over the guides");
+    let d = &s.doc().unwrap().doc;
+    assert_eq!(d.artboards, vec![first]);
+    let left: Vec<Option<NodeId>> = d.ruler_guides().into_iter().map(|g| g.id).collect();
+    assert_eq!(left, vec![Some(keep)]);
+    assert!(gone.iter().all(|id| !left.contains(&Some(*id))));
+}
+
 #[test]
 fn show_selects_the_objects_with_the_issue() {
     let mut s = session();