Merge pull request #6676 from dotCooCoo/fix/sublayer-ruler-guides

Guides: sublayer guides survive layer effects and the canvas check

crates/doc/src/canvas_range.rs

@@ -15,7 +15,8 @@ pub struct BeyondCanvas {
     /// Objects removed whole (a layer or group keeps its children within the canvas).
     pub objects: usize,
     pub artboards: usize,
-    /// Guides beyond the canvas, and the artboard guides of the artboards removed.
+    /// Guides beyond the canvas, the artboard guides of the artboards removed, and the layer
+    /// guides of the layers removed.
     pub guides: usize,
     /// The ruler origin was beyond the canvas and is back at the origin.
     pub ruler_origin: bool,
@@ -100,11 +101,20 @@ fn layers_in(n: &Node, layers: &mut Vec<NodeId>) {
     }
 }
 
+/// How many ruler guides the layers in `n` hold, if it is a layer or group.
+fn guides_in(n: &Node) -> usize {
+    match clipping(n) {
+        None => 0,
+        Some(_) => n.children().into_iter().flatten().map(|c| if c.is_ruler_guide() { 1 } else { guides_in(c) }).sum(),
+    }
+}
+
 /// Remove the objects in `node` that reach beyond the canvas: a layer or group keeps those of its
 /// objects within it, unless its clipping path reaches beyond (see [`clips_beyond`]); anything
 /// else goes whole. Only containers holding such an object are copied. Recursion follows the
-/// nesting, which [`Document::limit_depth`] bounds. The layers removed are added to `layers`.
-fn drop_children_beyond(node: &mut Arc<Node>, dropped: &mut usize, layers: &mut Vec<NodeId>) {
+/// nesting, which [`Document::limit_depth`] bounds. The layers removed are added to `layers`, and
+/// the ruler guides they held to `guides`.
+fn drop_children_beyond(node: &mut Arc<Node>, dropped: &mut usize, layers: &mut Vec<NodeId>, guides: &mut usize) {
     if !node.children().is_some_and(|c| c.iter().any(|c| reaches_beyond(c))) {
         return;
     }
@@ -114,10 +124,11 @@ fn drop_children_beyond(node: &mut Arc<Node>, dropped: &mut usize, layers: &mut
             return true;
         }
         if clipping(c).is_some() && !clips_beyond(c) {
-            drop_children_beyond(c, dropped, layers);
+            drop_children_beyond(c, dropped, layers, guides);
             return true;
         }
         layers_in(c, layers);
+        *guides += guides_in(c);
         *dropped += 1;
         false
     });
@@ -159,13 +170,14 @@ impl Document {
                     for c in children.iter() {
                         layers_in(c, &mut layers);
                     }
-                    // The layer's own ruler guides stay; only its art goes.
+                    // The layer's own ruler guides stay; only its art goes, with its sublayers' guides.
                     gone.objects += children.iter().filter(|c| !c.is_ruler_guide()).count();
+                    gone.guides += children.iter().filter(|c| !c.is_ruler_guide()).map(|c| guides_in(c)).sum::<usize>();
                     children.retain(|c| c.is_ruler_guide());
                     *clip = false;
                 }
             } else {
-                drop_children_beyond(layer, &mut gone.objects, &mut layers);
+                drop_children_beyond(layer, &mut gone.objects, &mut layers, &mut gone.guides);
             }
         }
         if gone.objects > 0 {
@@ -184,7 +196,7 @@ impl Document {
         let guides = self.saved_guides.len();
         self.saved_guides
             .retain(|g| in_range(g.pos) && g.artboard.is_none_or(|a| !removed.contains(&a)) && g.layer.is_none_or(|l| !layers.contains(&l)));
-        gone.guides = guides - self.saved_guides.len();
+        gone.guides += guides - self.saved_guides.len();
         let out = |n: &Node| match n.kind {
             NodeKind::RulerGuide { pos, artboard, .. } => !in_range(pos) || artboard.is_some_and(|a| removed.contains(&a)),
             _ => false,
@@ -324,6 +336,21 @@ mod tests {
         let layer = d2.layers[0].id.0;
         assert_eq!(d2.drop_beyond_canvas().objects, 1);
         assert_eq!(ids(&d2), [layer, 10]);
+        // The guides of a sublayer emptied out of a top-level layer go with it, counted.
+        let probe = Document::new(1.0, 1.0);
+        let sub = sublayer(&probe, 40, vec![rect(41, 0.0)], false);
+        let mut d3 = doc(vec![rect(10, 1e17), sub]);
+        if let NodeKind::Layer { clip, .. } = &mut Arc::make_mut(&mut d3.layers[0]).kind {
+            *clip = true;
+        }
+        d3.saved_guides =
+            vec![Guide { layer: Some(NodeId(40)), ..Guide::new(false, 5.0) }, Guide { layer: Some(d3.layers[0].id), ..Guide::new(true, 5.0) }];
+        let mut nodes = d3.clone();
+        assert_eq!(nodes.guides_into_nodes(), 2);
+        let gone = d3.drop_beyond_canvas();
+        assert_eq!((gone.objects, gone.guides), (2, 1), "the clipping path, the sublayer and its guide; the layer's own guide stays");
+        assert_eq!(nodes.drop_beyond_canvas(), gone);
+        assert_eq!(nodes.ruler_guides().len(), 1);
         assert_eq!(d.drop_beyond_canvas(), BeyondCanvas::default());
     }
 
@@ -353,8 +380,13 @@ mod tests {
         ];
         d.variables.bindings.insert(NodeId(11), ["v".to_string()].into());
         d.variables.bindings.insert(NodeId(10), ["w".to_string()].into());
+        // The same guides as nodes, as a document has them once read.
+        let mut nodes = d.clone();
+        assert_eq!(nodes.guides_into_nodes(), 2);
         let gone = d.drop_beyond_canvas();
         assert_eq!((gone.objects, gone.guides), (2, 1), "the far rectangle, the clipping sublayer and its layer guide");
+        assert_eq!(nodes.drop_beyond_canvas(), gone, "a guide node goes with its sublayer, counted as a saved one is");
+        assert_eq!(nodes.ruler_guides().len(), 1);
         assert_eq!(d.saved_guides.len(), 1, "the guide of a layer that stays, stays");
         assert_eq!(d.saved_selections[0].objects, [NodeId(10)]);
         assert_eq!(d.assets.iter().map(|a| (a.id, a.nodes.clone())).collect::<Vec<_>>(), [(2, vec![NodeId(10)])]);

crates/effects/src/group.rs

@@ -719,6 +719,37 @@ fn item_art(id: NodeId, item: &AppearanceItem, shapes: &[(PathData, FillRule)],
     Some(g)
 }
 
+/// Whether a sublayer among the members of `n`, or among its groups' members, holds a ruler guide.
+fn sublayer_guides(n: &Node) -> bool {
+    n.children().is_some_and(|ch| ch.iter().any(|c| c.holds_ruler_guides() || (!c.is_layer() && sublayer_guides(c))))
+}
+
+/// Move the ruler guides of the sublayers among the members of `n`, and among its groups'
+/// members, to `out`, in order. A guide taken from a hidden or locked sublayer, or from one inside
+/// such a sublayer or group, is hidden or locked itself. Only containers holding one are copied.
+fn take_sublayer_guides(n: &mut Node, out: &mut Vec<Arc<Node>>, hidden: bool, locked: bool) {
+    let Some(ch) = n.children_mut() else { return };
+    for c in ch.iter_mut().filter(|c| c.holds_ruler_guides() || (!c.is_layer() && sublayer_guides(c))) {
+        let c = Arc::make_mut(c);
+        let (hidden, locked) = (hidden || !c.visible, locked || c.locked);
+        if c.is_layer()
+            && let Some(lc) = c.children_mut()
+        {
+            let (taken, art): (Vec<_>, Vec<_>) = std::mem::take(lc).into_iter().partition(|g| g.is_ruler_guide());
+            *lc = art;
+            out.extend(taken.into_iter().map(|mut g| {
+                if hidden || locked {
+                    let g = Arc::make_mut(&mut g);
+                    g.visible &= !hidden;
+                    g.locked |= locked;
+                }
+                g
+            }));
+        }
+        take_sublayer_guides(c, out, hidden, locked);
+    }
+}
+
 /// `n`, a group or layer, as plain art: its Pathfinder effects evaluated, its geometry effects
 /// reshaping all its members as one piece (the whole art's bounds as their reference box), and its
 /// own fills and strokes turned into art painting the members' geometry ([`member_shapes`]) below
@@ -739,7 +770,12 @@ pub fn evaluate_container(doc: Option<&Document>, n: &Node) -> Option<Node> {
     }
     let mut m = n.clone();
     // A layer's ruler guides take no part: they go back above the evaluated art at the end.
-    let guides = m.children_mut().map(|ch| ch.split_off(Node::guide_run_start(ch))).unwrap_or_default();
+    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()) {
+        take_sublayer_guides(&mut m, &mut guides, false, false);
+    }
     if pathfinder {
         let hook = |c: &Node| crate::outline_object(doc, c);
         let result = pathfinder_children(doc, &m, Some(&hook)).unwrap_or_default();

crates/effects/src/group/tests.rs

@@ -862,3 +862,70 @@ fn a_layers_ruler_guides_stay_last_through_its_effects() {
     painted.appearance.items = Appearance::basic(Paint::solid(Color::BLACK), Paint::None, 0.0).items;
     assert_eq!(tail(&evaluate_container(None, &painted).unwrap()), guides);
 }
+
+/// The ruler guides of a sublayer in a layer with a Pathfinder effect, or a Transform effect that
+/// copies its art, stay on a layer of the evaluated art, once, hidden and locked as the sublayer is.
+#[test]
+fn a_sublayers_ruler_guides_survive_its_layers_effects() {
+    use vectorcraft_doc::LayerColor;
+    let layer = |effect: &str, params: serde_json::Value, shown: bool| {
+        let mut sub = Node::layer(NodeId(11), "Sub", LayerColor::Preset(1));
+        (sub.visible, sub.locked) = (shown, !shown);
+        if let Some(ch) = sub.children_mut() {
+            *ch = vec![
+                sq(2, 5.0, Color::rgb(0.0, 0.0, 1.0)),
+                Arc::new(Node::new(NodeId(20), NodeKind::RulerGuide { vertical: true, pos: 3.0, artboard: None })),
+            ];
+        }
+        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)), Arc::new(sub)];
+        }
+        l.appearance.effects.push(Effect { id: effect.into(), params, visible: true });
+        l
+    };
+    // The guides found where ruler guides are looked for: directly in a layer or sublayer.
+    fn listed(n: &Node, out: &mut Vec<(NodeId, bool, bool)>) {
+        for c in n.children().into_iter().flatten() {
+            if c.is_ruler_guide() {
+                out.push((c.id, c.visible, c.locked));
+            } else if c.is_layer() {
+                listed(c, out);
+            }
+        }
+    }
+    for (effect, params) in [("pathfinder.add", serde_json::json!({})), ("distort.transform", serde_json::json!({"moveH": 30, "copies": 2}))] {
+        for shown in [true, false] {
+            let art = evaluate_container(None, &layer(effect, params.clone(), shown)).unwrap();
+            let mut found = vec![];
+            listed(&art, &mut found);
+            assert_eq!(found, [(NodeId(20), shown, !shown)], "{effect}: the sublayer's guide, once, on a layer");
+        }
+    }
+}
+
+/// A sublayer's ruler guide inside a hidden or locked group stays hidden or locked when its layer's
+/// Pathfinder effect moves it onto the layer, as the group made it.
+#[test]
+fn a_sublayers_ruler_guide_in_a_hidden_or_locked_group_stays_hidden_or_locked() {
+    use vectorcraft_doc::LayerColor;
+    for (hidden, locked) in [(true, false), (false, true), (false, false)] {
+        let mut sub = Node::layer(NodeId(11), "Sub", LayerColor::Preset(1));
+        if let Some(ch) = sub.children_mut() {
+            *ch = vec![
+                sq(2, 5.0, Color::rgb(0.0, 0.0, 1.0)),
+                Arc::new(Node::new(NodeId(20), NodeKind::RulerGuide { vertical: true, pos: 3.0, artboard: None })),
+            ];
+        }
+        let mut group = Node::group(NodeId(12), vec![Arc::new(sub)]);
+        (group.visible, group.locked) = (!hidden, locked);
+        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)), Arc::new(group)];
+        }
+        l.appearance.effects.push(Effect { id: "pathfinder.add".into(), params: serde_json::json!({}), visible: true });
+        let art = evaluate_container(None, &l).unwrap();
+        let guides: Vec<_> = art.children().into_iter().flatten().filter(|c| c.is_ruler_guide()).map(|c| (c.id, c.visible, c.locked)).collect();
+        assert_eq!(guides, [(NodeId(20), !hidden, locked)], "hidden {hidden}, locked {locked}");
+    }
+}

crates/format/src/lib.rs

@@ -594,6 +594,10 @@ fn load_file_within(bytes: &[u8], max_nodes: usize, max_values: usize) -> Result
     doc.raster_effects.add_around = doc.raster_effects.add_around.clamp(0.0, vectorcraft_doc::rastersettings::MAX_ADD_AROUND);
     let mut depth_notes = vec![];
     doc.limit_depth_noting(&mut depth_notes);
+    // Untrusted: ruler guide nodes only directly in layers, above their art (after the depth limit:
+    // the walk recurses; before the canvas check, which takes a layer's first member as its
+    // clipping path).
+    doc.tidy_ruler_guides();
     // Untrusted: nothing beyond the canvas, where no edit may leave anything (after the depth
     // limit: the walk recurses). A file whose artboards all lie there has no page to show (#3575).
     let had_artboards = !doc.artboards.is_empty();
@@ -605,9 +609,6 @@ fn load_file_within(bytes: &[u8], max_nodes: usize, max_values: usize) -> Result
         )));
     }
     depth_notes.extend(beyond.note());
-    // Untrusted: ruler guide nodes only directly in layers, above their art (after the depth limit:
-    // the walk recurses).
-    doc.tidy_ruler_guides();
     // The saved ruler guides become nodes on their layers (after the guides beyond the canvas are
     // dropped, and after `fix_next_id`: new ids start above the file's).
     doc.guides_into_nodes();
@@ -1385,4 +1386,35 @@ mod tests {
         assert_eq!(base64_decode("QUI="), Some(vec![0x41, 0x42]));
         assert_eq!(base64_decode("QUJD"), Some(vec![0x41, 0x42, 0x43]));
     }
+
+    /// A layer clipping by a path beyond the canvas is emptied on open and stops clipping, also
+    /// when the file puts a ruler guide before the clipping path: no other object becomes it.
+    #[test]
+    fn a_guide_before_a_clipping_path_beyond_the_canvas_does_not_change_the_clipping_path() {
+        let opened = |guide_first: bool| {
+            let mut d = Document::new(100.0, 100.0);
+            let rect = |d: &mut Document, y: f64| {
+                Arc::new(Node::path(d.alloc_id(), shapes::rectangle(Rect::new(0.0, y, 10.0, y + 10.0)), Appearance::default_art()))
+            };
+            let (far, near) = (rect(&mut d, 1e17), rect(&mut d, 0.0));
+            if let NodeKind::Layer { children, clip, .. } = &mut Arc::make_mut(&mut d.layers[0]).kind {
+                *children = vec![far, near];
+                *clip = true;
+            }
+            // Saving writes ruler guides to the saved list: the guide node is put in the file.
+            let mut v: serde_json::Value = serde_json::from_slice(&save(&d, false)).unwrap();
+            let guide = serde_json::json!({"id": 50, "kind": {"type": "rulerguide", "vertical": true, "pos": 5.0}});
+            let kids = v["document"]["layers"][0]["kind"]["children"].as_array_mut().unwrap();
+            if guide_first {
+                kids.insert(0, guide)
+            } else {
+                kids.push(guide)
+            }
+            let d = load(&serde_json::to_vec(&v).unwrap()).unwrap();
+            let NodeKind::Layer { children, clip, .. } = &d.layers[0].kind else { panic!("a layer") };
+            (*clip, children.iter().map(|c| c.is_ruler_guide()).collect::<Vec<_>>())
+        };
+        assert_eq!(opened(false), (false, vec![true]), "the clipping path and what it clipped go; the guide stays");
+        assert_eq!(opened(true), opened(false));
+    }
 }