From 05500f7ddbe1027d8c9d7c75acecb6ef132c3f2a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Fri, 13 Nov 2026 11:20:05 -0800 Subject: [PATCH] Artwork Cleanup: update the selected objects in one pass after each fix With the selection as its scope, a fix that removes or unwraps objects (Needless Groups, Stray Points, Empty Text and the others) kept the list of selected objects current by searching it and splicing it once for each object it changed. Removing many selected objects above a long run of others took time that grew with the square of their number. The list is now rebuilt in one walk after each pass: each changed object gives way to what replaces it, in its place, and the others keep their order. The objects found, the counts, the document and the undo step are the same as before. Empty Artboards deleted each empty artboard with its own Vec::remove and walked every ruler guide again for each one. It now drops the empty artboards in one walk and deletes their guides in one more. The same artboards and guides are deleted as before. Tests cover the order after unwrapping nested and empty groups, Fix All reaching an unwrapped member on a later round, the rebuild itself, the number of objects looked at for 150 and 300 removals, and the number of walks over the artboards and guides when 40 empty artboards are deleted. --- .../engine/src/cmd/addons/cleanup/checks.rs | 75 +++++++-- crates/engine/src/cmd/addons/cleanup/tests.rs | 144 ++++++++++++++++++ 2 files changed, 207 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/cmd/addons/cleanup/checks.rs b/crates/engine/src/cmd/addons/cleanup/checks.rs index 4fd1f1ce96..52b20cdddc 100644 --- a/crates/engine/src/cmd/addons/cleanup/checks.rs +++ b/crates/engine/src/cmd/addons/cleanup/checks.rs @@ -369,11 +369,7 @@ fn restructure(d: &mut Document, scope: &mut Scope, kind: Kind) -> Result Result = 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)]) -> Vec { + let mut with: HashMap = 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 { @@ -479,13 +505,38 @@ fn near_points(d: &mut Document, scope: &Scope) -> Result { fn artboards(d: &mut Document, sel: &mut Selection, scope: &Scope) -> Result { 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 = 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 = 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); +} diff --git a/crates/engine/src/cmd/addons/cleanup/tests.rs b/crates/engine/src/cmd/addons/cleanup/tests.rs index 1184687366..2ccdce9dad 100644 --- a/crates/engine/src/cmd/addons/cleanup/tests.rs +++ b/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 { + 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 = check["findings"][0]["ids"].as_array().unwrap().iter().map(|v| v.as_u64().unwrap()).collect(); + listed.sort_unstable(); + let mut gone: Vec = [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::>(); + 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 = (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> = 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();