From 154767077e49cb90cbeebc4cfda7c77b724ab8ec Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 14 Nov 2026 18:43:12 -0800 Subject: [PATCH 1/2] Effects: keep ruler guides below guide-free sublayers and in groups out of effects A layer's Pathfinder or geometry effect now moves every ruler guide in its sublayers and groups out of the art it replaces or copies, including a guide in a layer inside a group inside a sublayer that holds no guide of its own, and a guide directly in a group. A group's Pathfinder or geometry effect does the same for the guides among its members. Each guide stays once, last in the container, hidden or locked as the containers it came from were. Every member is looked at once. --- crates/effects/src/group.rs | 71 +++++++----------- crates/effects/src/group/tests.rs | 120 ++++++++++++++++++++++++++++-- 2 files changed, 140 insertions(+), 51 deletions(-) diff --git a/crates/effects/src/group.rs b/crates/effects/src/group.rs index 1b64971a3a..0e37be9ded 100644 --- a/crates/effects/src/group.rs +++ b/crates/effects/src/group.rs @@ -742,45 +742,28 @@ fn item_art(id: NodeId, item: &AppearanceItem, shapes: &[(PathData, FillRule)], } /// The members of a container that [`take_held_guides`] visits, by index, each with the same for -/// its own members. A ruler guide's entry is empty; a container's is not. +/// its own members. A ruler guide's entry is empty, whether its container is a layer or a group; a +/// container's is not. #[derive(Default)] struct HeldGuides(Vec<(usize, HeldGuides)>); -/// The members of `n` holding or leading to the ruler guides of sublayers, found by looking at each -/// member at every depth once: the ruler guides (`is_guide`) of `n` when it is a layer, each -/// sublayer holding one directly or in its sublayers, and each member of another kind with such a -/// sublayer among its members or its groups' members. With them, whether `n` is a layer holding a -/// ruler guide directly or in its sublayers. The other members of a layer holding none are not -/// looked into, unless the layer is the one whose sublayers are gathered (`top`). -fn held_guides(n: &Node, top: bool, is_guide: &impl Fn(&Node) -> bool) -> (bool, HeldGuides) { - let layer = n.is_layer(); - let (mut holds, mut held, mut other) = (false, vec![], vec![]); +/// The members of `n` that are ruler guides (`is_guide`), and those holding one at any depth, in +/// order. Every member at every depth is looked at once: a sublayer or group with no guide of its +/// own is still looked into, and a guide directly in a group counts as one in a layer does. A +/// member holding no guide gets no entry. +fn held_guides(n: &Node, is_guide: &impl Fn(&Node) -> bool) -> HeldGuides { + let mut held = vec![]; for (i, c) in n.children().into_iter().flatten().enumerate() { if is_guide(c) { - if layer { - holds = true; - held.push((i, HeldGuides::default())); - } - } else if c.is_layer() { - let (sub_holds, inner) = held_guides(c, false, is_guide); - if sub_holds { - holds = true; - held.push((i, inner)); - } - } else { - other.push((i, c)); + held.push((i, HeldGuides::default())); + continue; + } + let inner = held_guides(c, is_guide); + if !inner.0.is_empty() { + held.push((i, inner)); } } - if !layer || holds || top { - for (i, c) in other { - let (_, inner) = held_guides(c, false, is_guide); - if !inner.0.is_empty() { - held.push((i, inner)); - } - } - held.sort_unstable_by_key(|(i, _)| *i); - } - (layer && holds, HeldGuides(held)) + HeldGuides(held) } /// Move the ruler guides of `n` and of its members that [`held_guides`] found (`held`) to `out`: @@ -818,15 +801,14 @@ fn take_held_guides(n: &mut Node, held: HeldGuides, out: &mut Vec>, hi out.append(&mut deeper); } -/// Move the ruler guides of the sublayers among the members of `n`, and among its groups' -/// members, to `out`, in order: a sublayer's own guides, then those of its members. A guide taken -/// from a hidden or locked sublayer, or from one inside such a sublayer or group, is hidden or -/// locked itself. Each member at every depth is looked at once at most (`is_guide`), and only -/// containers holding one are copied. +/// Move the ruler guides of `n` and those held at any depth by the sublayers and groups among its +/// members to `out`, in order: a container's own guides, then those of its members. A sublayer or +/// group with no guide of its own still gives up those of its members. A guide taken from a hidden +/// or locked sublayer or group, or from one inside such a container, is hidden or locked itself. +/// Each member at every depth is looked at once (`is_guide`), and only containers holding a guide +/// are copied. fn take_sublayer_guides(n: &mut Node, out: &mut Vec>, is_guide: &impl Fn(&Node) -> bool) { - let (_, mut held) = held_guides(n, true, is_guide); - // The ruler guides of `n` itself stay where they are. - held.0.retain(|(_, inner)| !inner.0.is_empty()); + let held = held_guides(n, is_guide); take_held_guides(n, held, out, false, false); } @@ -850,11 +832,12 @@ pub fn evaluate_container(doc: Option<&Document>, n: &Node) -> Option { return None; } let mut m = n.clone(); - // A layer's ruler guides take no part: they go back above the evaluated art at the end. + // Its ruler guides take no part: they go back above the evaluated art at the end. let mut guides = m.children_mut().map(|ch| ch.split_off(Node::guide_run_start(ch))).unwrap_or_default(); - // So do its sublayers' guides, which would otherwise go with the art the effects replace or - // copy: they stay on this layer, hidden and locked as their sublayers were. - if m.is_layer() && (pathfinder || !fx.is_empty()) { + // So do its other guides and those in its sublayers and groups, which would otherwise go with + // the art the effects replace or copy: they stay on `n`, hidden and locked as their containers + // were. + if pathfinder || !fx.is_empty() { take_sublayer_guides(&mut m, &mut guides, &Node::is_ruler_guide); } if pathfinder { diff --git a/crates/effects/src/group/tests.rs b/crates/effects/src/group/tests.rs index 519323e5f0..b2b802cc44 100644 --- a/crates/effects/src/group/tests.rs +++ b/crates/effects/src/group/tests.rs @@ -1808,7 +1808,7 @@ fn a_sublayers_ruler_guide_in_a_hidden_or_locked_group_stays_hidden_or_locked() } /// Gathering the ruler guides of nested sublayers, and of a sublayer inside nested groups, looks -/// at each member at every depth once at most, and takes the same guides, in the same order and +/// at each member at every depth once, and takes the same guides, in the same order and /// state, as a layer's effects keep. #[test] fn gathering_sublayer_ruler_guides_looks_at_each_member_once() { @@ -1840,7 +1840,7 @@ fn gathering_sublayer_ruler_guides_looks_at_each_member_once() { groups.locked = depth == 3; } let free = Arc::new(Node::group(NodeId(500), vec![sq(501, 0.0, Color::BLACK), sq(502, 5.0, Color::BLACK)])); - // A sublayer with no guide: the art in its group is not looked at. + // A sublayer with no guide, its group holding none either. let quiet = Node::group(NodeId(601), vec![sq(602, 0.0, Color::BLACK), sq(603, 5.0, Color::BLACK)]); let quiet = Arc::new(layer(600, vec![Arc::new(quiet)])); let mut root = layer(10, vec![sq(1, 0.0, Color::rgb(1.0, 0.0, 0.0)), Arc::new(chain), Arc::new(groups), free.clone(), quiet.clone(), guide(20)]); @@ -1858,15 +1858,16 @@ fn gathering_sublayer_ruler_guides_looks_at_each_member_once() { }; let mut taken = vec![]; take_sublayer_guides(&mut m, &mut taken, &is_guide); - assert_eq!(looked.get(), members(&root) - 2, "each member is looked at once, none in the quiet sublayer's group"); - assert_eq!(state(&taken), expected, "a sublayer's own guides, then its sublayers', each once"); - assert!(Arc::ptr_eq(&taken[0], &first_guide), "a guide with nothing to change is not copied"); - assert!(!Arc::ptr_eq(&taken[1], &second_guide), "a guide made hidden is copied"); + assert_eq!(looked.get(), members(&root), "each member is looked at once, those of guide-free containers included"); + assert_eq!(taken[0].id, NodeId(20), "the layer's own guide comes first"); + assert_eq!(state(&taken[1..]), expected, "a sublayer's own guides, then its sublayers', each once"); + assert!(Arc::ptr_eq(&taken[1], &first_guide), "a guide with nothing to change is not copied"); + assert!(!Arc::ptr_eq(&taken[2], &second_guide), "a guide made hidden is copied"); assert!(second_guide.visible, "the guide as it was stays as it was"); let ch = m.children().unwrap(); + assert_eq!(ch.len(), 5, "the layer's own guide is taken"); assert!(Arc::ptr_eq(&ch[3], &free), "a member with no sublayer guide is not copied"); assert!(Arc::ptr_eq(&ch[4], &quiet), "a sublayer with no guide is not copied"); - assert!(ch[5].is_ruler_guide(), "the layer's own guide stays where it is"); let ids = |n: &Node| n.children().into_iter().flatten().map(|c| c.id).collect::>(); assert_eq!(ids(&ch[1]), [NodeId(102), NodeId(301)], "the sublayer's art stays, in order"); assert_eq!(ids(&root.children().unwrap()[1]), [NodeId(102), NodeId(301), NodeId(201)], "the source keeps its guides"); @@ -1892,6 +1893,111 @@ fn gathering_sublayer_ruler_guides_looks_at_each_member_once() { } } +/// Where each ruler guide in `n` is, at every depth, with its position, visibility and lock. +fn ruler_guides_in(n: &Node, out: &mut Vec<(NodeId, NodeKind, bool, bool)>) { + for c in n.children().into_iter().flatten() { + if c.is_ruler_guide() { + out.push((c.id, c.kind.clone(), c.visible, c.locked)); + } + ruler_guides_in(c, out); + } +} + +/// A ruler guide in a layer inside a group inside a sublayer that holds no guide of its own, at +/// any depth of that pattern, survives its layer's Pathfinder effect or Transform copies once, last +/// in the layer, hidden or locked when any container on its way was. +#[test] +fn a_ruler_guide_below_a_guide_free_sublayer_and_a_group_survives_its_layers_effects() { + use vectorcraft_doc::LayerColor; + let layer = |id, children: Vec>, (hidden, locked): (bool, bool)| { + let mut l = Node::layer(NodeId(id), "Sub", LayerColor::Preset(1)); + if let Some(ch) = l.children_mut() { + *ch = children; + } + (l.visible, l.locked) = (!hidden, locked); + Arc::new(l) + }; + let group = |id, children: Vec>, (hidden, locked): (bool, bool)| { + let mut g = Node::group(NodeId(id), children); + (g.visible, g.locked) = (!hidden, locked); + Arc::new(g) + }; + let kind = NodeKind::RulerGuide { vertical: false, pos: 7.0, artboard: None }; + let states = [(false, false), (true, false), (false, true)]; + for (effect, params) in [("pathfinder.add", serde_json::json!({})), ("distort.transform", serde_json::json!({"moveH": 30, "copies": 2}))] { + for depth in 1..=3_u64 { + for sub in states { + for grp in states { + for inner in states { + // Layer 10 → sublayer → group → … → layer holding the guide, the pattern + // repeated `depth` times; only the innermost layer holds a guide. + let mut n = layer(900, vec![sq(901, 0.0, Color::BLACK), Arc::new(Node::new(NodeId(20), kind.clone()))], inner); + for d in (0..depth).rev() { + let (s, g) = if d == 0 { (sub, grp) } else { ((false, false), (false, false)) }; + n = group(700 + d, vec![sq(710 + d, 5.0, Color::BLACK), n], g); + n = layer(800 + d, vec![n, sq(810 + d, 2.0, Color::BLACK)], s); + } + let mut root = Node::layer(NodeId(10), "Layer 1", LayerColor::Preset(0)); + if let Some(ch) = root.children_mut() { + *ch = vec![sq(1, 0.0, Color::rgb(1.0, 0.0, 0.0)), n]; + } + root.appearance.effects.push(Effect { id: effect.into(), params: params.clone(), visible: true }); + let art = evaluate_container(None, &root).unwrap(); + let (hidden, locked) = (sub.0 || grp.0 || inner.0, sub.1 || grp.1 || inner.1); + let mut all = vec![]; + ruler_guides_in(&art, &mut all); + let case = format!("{effect}, depth {depth}, sublayer {sub:?}, group {grp:?}, layer {inner:?}"); + assert_eq!(all, [(NodeId(20), kind.clone(), !hidden, locked)], "{case}: the guide, once, unchanged"); + let last = art.children().and_then(|c| c.last()).unwrap(); + assert_eq!(last.id, NodeId(20), "{case}: last in the layer"); + } + } + } + } + } +} + +/// A ruler guide directly in a group, in a layer or in a group with a Pathfinder effect or +/// Transform copies, survives that effect once, last in the container, hidden or locked as its +/// groups were. So does one among a group's art rather than after it. +#[test] +fn a_ruler_guide_in_a_group_survives_its_containers_effects() { + use vectorcraft_doc::LayerColor; + let kind = NodeKind::RulerGuide { vertical: true, pos: 4.0, artboard: None }; + let guide = || Arc::new(Node::new(NodeId(20), kind.clone())); + for (effect, params) in [("pathfinder.add", serde_json::json!({})), ("distort.transform", serde_json::json!({"moveH": 30, "copies": 2}))] { + for (hidden, locked) in [(false, false), (true, false), (false, true)] { + let mut inner = Node::group(NodeId(12), vec![sq(2, 5.0, Color::BLACK), guide(), sq(3, 8.0, Color::BLACK)]); + (inner.visible, inner.locked) = (!hidden, locked); + let outer = Arc::new(Node::group(NodeId(11), vec![Arc::new(inner)])); + let fx = Effect { id: effect.into(), params: params.clone(), visible: true }; + let mut l = Node::layer(NodeId(10), "Layer 1", LayerColor::Preset(0)); + if let Some(ch) = l.children_mut() { + *ch = vec![sq(1, 0.0, Color::rgb(1.0, 0.0, 0.0)), outer.clone()]; + } + l.appearance.effects.push(fx.clone()); + let mut g = Node::group(NodeId(13), vec![sq(1, 0.0, Color::rgb(1.0, 0.0, 0.0)), outer]); + g.appearance.effects.push(fx.clone()); + for (what, n) in [("layer", l), ("group", g)] { + let art = evaluate_container(None, &n).unwrap(); + let mut all = vec![]; + ruler_guides_in(&art, &mut all); + let case = format!("{effect} on a {what}, hidden {hidden}, locked {locked}"); + assert_eq!(all, [(NodeId(20), kind.clone(), !hidden, locked)], "{case}: the guide, once, unchanged"); + assert_eq!(art.children().and_then(|c| c.last()).map(|c| c.id), Some(NodeId(20)), "{case}: last"); + } + } + // A group's guide placed before its art. + let mut g = Node::group(NodeId(13), vec![guide(), sq(1, 0.0, Color::BLACK), sq(2, 5.0, Color::BLACK)]); + g.appearance.effects.push(Effect { id: effect.into(), params: params.clone(), visible: true }); + let art = evaluate_container(None, &g).unwrap(); + let mut all = vec![]; + ruler_guides_in(&art, &mut all); + assert_eq!(all, [(NodeId(20), kind.clone(), true, false)], "{effect}: a group's leading guide, once"); + assert_eq!(art.children().and_then(|c| c.last()).map(|c| c.id), Some(NodeId(20)), "{effect}: moved last"); + } +} + /// A group's own Fill + Offset Path below its contents follows the outline of a clip group among /// its members, its clipping path, not the clipped art it hides outside that path (#7289). #[test] From a11bd64909190f36bfab40d076b95c690d54265c Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 14 Nov 2026 23:04:26 -0800 Subject: [PATCH 2/2] Guides: a container's ruler guides keep their order through its effects A layer or group with a Pathfinder effect or geometry effects moves its ruler guides above the evaluated art. Its trailing guides were split off first and its guides among the art were added after them, so [guide A, art, guide B] came out as B, A. All of its direct guides are now gathered in one pass, in document order. --- crates/effects/src/group.rs | 12 +++++++----- crates/effects/src/group/tests.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/crates/effects/src/group.rs b/crates/effects/src/group.rs index 0e37be9ded..d2a6687dd7 100644 --- a/crates/effects/src/group.rs +++ b/crates/effects/src/group.rs @@ -832,13 +832,15 @@ pub fn evaluate_container(doc: Option<&Document>, n: &Node) -> Option { return None; } let mut m = n.clone(); - // Its ruler guides take no part: they go back above the evaluated art at the end. - let mut guides = m.children_mut().map(|ch| ch.split_off(Node::guide_run_start(ch))).unwrap_or_default(); - // So do its other guides and those in its sublayers and groups, which would otherwise go with - // the art the effects replace or copy: they stay on `n`, hidden and locked as their containers - // were. + // Its ruler guides take no part: they go back above the evaluated art at the end. With effects + // that replace or copy the art, so do its guides among the art and those in its sublayers and + // groups, which would otherwise go with that art: they stay on `n`, in order, hidden and + // locked as their containers were. + let mut guides = vec![]; if pathfinder || !fx.is_empty() { take_sublayer_guides(&mut m, &mut guides, &Node::is_ruler_guide); + } else if let Some(ch) = m.children_mut() { + guides = ch.split_off(Node::guide_run_start(ch)); } if pathfinder { let hook = |c: &Node| crate::outline_object(doc, c); diff --git a/crates/effects/src/group/tests.rs b/crates/effects/src/group/tests.rs index b2b802cc44..50fbf8f759 100644 --- a/crates/effects/src/group/tests.rs +++ b/crates/effects/src/group/tests.rs @@ -1998,6 +1998,35 @@ fn a_ruler_guide_in_a_group_survives_its_containers_effects() { } } +/// The ruler guides of a layer or group with a Pathfinder effect or Transform copies, one among +/// its art and one after it, go after the evaluated art in the order they were in. +#[test] +fn a_containers_ruler_guides_keep_their_order_through_its_effects() { + use vectorcraft_doc::LayerColor; + let guide = |id| Arc::new(Node::new(NodeId(id), NodeKind::RulerGuide { vertical: false, pos: 3.0, artboard: None })); + for (effect, params) in [("pathfinder.add", serde_json::json!({})), ("distort.transform", serde_json::json!({"moveH": 30, "copies": 2}))] { + let members = || vec![sq(1, 0.0, Color::BLACK), guide(20), sq(2, 5.0, Color::BLACK), guide(21)]; + let fx = Effect { id: effect.into(), params: params.clone(), visible: true }; + let mut l = Node::layer(NodeId(10), "Layer 1", LayerColor::Preset(0)); + if let Some(ch) = l.children_mut() { + *ch = members(); + } + l.appearance.effects.push(fx.clone()); + let mut g = Node::group(NodeId(11), members()); + g.appearance.effects.push(fx); + for (what, n) in [("layer", l), ("group", g)] { + let art = evaluate_container(None, &n).unwrap(); + let mut all = vec![]; + ruler_guides_in(&art, &mut all); + let ids = all.iter().map(|(id, ..)| *id).collect::>(); + assert_eq!(ids, [NodeId(20), NodeId(21)], "{effect} on a {what}: the guides, in order"); + let ch = art.children().unwrap(); + let tail = ch[ch.len() - 2..].iter().map(|c| c.id).collect::>(); + assert_eq!(tail, [NodeId(20), NodeId(21)], "{effect} on a {what}: last, in order"); + } + } +} + /// A group's own Fill + Offset Path below its contents follows the outline of a clip group among /// its members, its clipping path, not the clipped art it hides outside that path (#7289). #[test]