Place: a DCS file that regains its clipping path clips the group that stayed

When a linked file lost its clipping path, the clip group holding the
image stayed as a plain group of the image. When the file had a clipping
path again, the image went into a new clip group inside that plain group,
so each round nested one more group. The plain group holding just the
image now becomes its clip group again, in the layers and in symbol,
pattern, graph design and mask art.

crates/engine/src/cmd/links.rs

@@ -738,12 +738,16 @@ impl Nested<'_> {
         let mut slots: Vec<(usize, Arc<Node>)> = vec![];
         // The object a clip group that loses its clipping path holds alone.
         let mut declipped = None;
+        // The clipping path and object of a plain group holding just the object, which becomes a
+        // clip group again.
+        let mut reclipped = None;
         if let Some(children) = n.children() {
             // A clip group holding just the object: its clipping path changes.
             let clip_path = match (&n.kind, &children[..]) {
                 (NodeKind::Group { clip: true, .. }, [c, _]) if matches!(c.kind, NodeKind::Path { clipping: true, .. }) => Some(c),
                 _ => None,
             };
+            let plain = matches!((&n.kind, &children[..]), (NodeKind::Group { clip: false, .. }, [_]));
             for (i, c) in children.iter().enumerate() {
                 if !(nested && (self.pick)(c)) {
                     let mut new = Arc::clone(c);
@@ -769,6 +773,7 @@ impl Nested<'_> {
                         }
                         slots.extend([(0, Arc::new(path)), (1, new)]);
                     }
+                    (ClipTo::Path(clip), None) if plain => reclipped = Some((clip, new)),
                     (ClipTo::Path(clip) | ClipTo::Nest(clip), _) => {
                         let group = self.clip_group(new, clip);
                         slots.push((i, Arc::new(group)));
@@ -778,13 +783,21 @@ impl Nested<'_> {
                 }
             }
         }
-        if mask.is_none() && slots.is_empty() && declipped.is_none() {
+        if mask.is_none() && slots.is_empty() && declipped.is_none() && reclipped.is_none() {
             return;
         }
+        let reclipped = reclipped.map(|(clip, object)| (Arc::new(self.clipping_path(clip)), object));
         let n = Arc::make_mut(n);
         if let (Some(art), Some(m)) = (mask, n.extras_if_any().and_then(|e| e.mask.as_mut())) {
             m.art = art;
         }
+        // A plain group of just the object that becomes a clip group again.
+        if let Some((path, object)) = reclipped {
+            if let NodeKind::Group { children, clip } = &mut n.kind {
+                (*children, *clip) = (vec![path, object], true);
+            }
+            return;
+        }
         // A clip group that stays as a plain group of just the object.
         if let Some(object) = declipped {
             if let NodeKind::Group { children, clip } = &mut n.kind {
@@ -803,15 +816,18 @@ impl Nested<'_> {
 
     /// A clip group of `object` clipped to `clip`, as [`clip_image_to`] makes one in the layers.
     fn clip_group(&mut self, object: Arc<Node>, clip: PathData) -> Node {
-        let (path_id, group_id) = (NodeId(self.next), NodeId(self.next + 1));
-        self.next += 2;
-        let none = vectorcraft_color::Paint::None;
-        let mut path = Node::path(path_id, clip, Appearance::basic(none.clone(), none, 0.0));
-        if let NodeKind::Path { clipping, .. } = &mut path.kind {
-            *clipping = true;
-        }
+        let path = self.clipping_path(clip);
+        let group_id = NodeId(self.next);
+        self.next += 1;
         Node::new(group_id, NodeKind::Group { children: vec![Arc::new(path), object], clip: true })
     }
+
+    /// A clipping path of `clip`, with the next node id.
+    fn clipping_path(&mut self, clip: PathData) -> Node {
+        let id = NodeId(self.next);
+        self.next += 1;
+        clipping_path(id, clip)
+    }
 }
 
 /// Placed documents `ids` show `src`, read from the file `link` points at, each placed as its
@@ -936,23 +952,40 @@ fn clip_image(d: &mut Document, id: NodeId, clip: Clip) -> Result<()> {
 }
 
 /// Clip object `id` of `d` to `path`: the clipping path of the clip group holding just it changes,
-/// else it goes into a new clip group.
+/// a plain group holding just it (as [`unclip`] leaves one) becomes a clip group again, else it
+/// goes into a new clip group.
 fn clip_image_to(d: &mut Document, id: NodeId, path: PathData) -> Result<()> {
     if set_clip_path(d, id, path.clone()) {
         return Ok(());
     }
-    new_clip_group(d, id, path)
+    let plain = d.parent_of(id).filter(|g| match d.node(*g).map(|n| &n.kind) {
+        Some(NodeKind::Group { children, clip: false }) => matches!(&children[..], [o] if o.id == id),
+        _ => false,
+    });
+    let Some(g) = plain else { return new_clip_group(d, id, path) };
+    let clip = clipping_path(d.alloc_id(), path);
+    d.insert(Some(g), 0, clip)?;
+    if let Some(Node { kind: NodeKind::Group { clip, .. }, .. }) = d.node_mut(g) {
+        *clip = true;
+    }
+    Ok(())
+}
+
+/// A clipping path `id` of `path`, unpainted.
+fn clipping_path(id: NodeId, path: PathData) -> Node {
+    let none = vectorcraft_color::Paint::None;
+    let mut clip = Node::path(id, path, Appearance::basic(none.clone(), none, 0.0));
+    if let NodeKind::Path { clipping, .. } = &mut clip.kind {
+        *clipping = true;
+    }
+    clip
 }
 
 /// Put object `id` of `d` into a new clip group of `path`, where it is.
 fn new_clip_group(d: &mut Document, id: NodeId, path: PathData) -> Result<()> {
     let (par, index, _) = d.position(id).ok_or(EngineError::NoNode(id))?;
     let image = d.remove(id)?;
-    let none = vectorcraft_color::Paint::None;
-    let mut clip = Node::path(d.alloc_id(), path, Appearance::basic(none.clone(), none, 0.0));
-    if let NodeKind::Path { clipping, .. } = &mut clip.kind {
-        *clipping = true;
-    }
+    let clip = clipping_path(d.alloc_id(), path);
     let group = Node::new(d.alloc_id(), NodeKind::Group { children: vec![std::sync::Arc::new(clip), image], clip: true });
     d.insert(par, index, group)?;
     Ok(())

crates/engine/src/tests/dcs.rs

@@ -357,6 +357,42 @@ fn a_file_that_lost_its_clipping_path_unclips_the_image() {
     );
 }
 
+#[test]
+fn a_file_that_gains_its_clipping_path_again_clips_the_group_that_stayed() {
+    let dir = temp_dir("update-regained-clip");
+    let path = write(&dir, "pic.eps", &two_plates());
+    let mut s = session();
+    let r = s.execute("file.place", &json!({ "path": path, "at": [100, 100] })).unwrap();
+    let placed = NodeId(r["ids"][0].as_u64().unwrap());
+    let id = image_of(&s, &r["ids"][0]).id;
+    let other = s.execute("file.place", &json!({ "path": path, "at": [200, 100] })).unwrap();
+    s.execute("symbol.new", &json!({ "name": "Pic", "ids": [other["ids"][0]] })).unwrap();
+    let unclipped = String::from_utf8(two_plates()).unwrap().replacen("closepath clip newpath", "closepath newpath     ", 1);
+    // The symbol's art: how deep the image is.
+    let depth = |s: &Session| {
+        fn find(n: &Node, at: usize) -> Option<usize> {
+            if matches!(n.kind, NodeKind::Image(_)) {
+                return Some(at);
+            }
+            n.children().and_then(|ch| ch.iter().find_map(|c| find(c, at + 1)))
+        }
+        find(&s.doc().unwrap().doc.symbols.iter().find(|w| w.name == "Pic").expect("the symbol").art, 0)
+    };
+    let clipped_depth = depth(&s);
+    for _ in 0..2 {
+        std::fs::write(&path, unclipped.as_bytes()).unwrap();
+        s.execute("links.update", &json!({ "ids": [id.0] })).unwrap();
+        std::fs::write(&path, two_plates()).unwrap();
+        s.execute("links.update", &json!({ "ids": [id.0] })).unwrap();
+        let doc = &s.doc().unwrap().doc;
+        assert_eq!(doc.parent_of(id), Some(placed), "the placed group holds the image itself");
+        let NodeKind::Group { children, clip: true } = &doc.node(placed).unwrap().kind else { panic!("a clip group again") };
+        assert!(matches!(&children[..], [c, im] if matches!(c.kind, NodeKind::Path { clipping: true, .. }) && im.id == id));
+        assert_eq!(depth(&s), clipped_depth, "the symbol's image is not nested deeper");
+        assert!(symbol_clip_width(&s, "Pic").is_some(), "the symbol's image is clipped again");
+    }
+}
+
 /// The kind of group `g` is, as (clip, the kinds of its children are just one image).
 fn plain_group_of_image(g: &Node) -> Option<(bool, bool)> {
     match &g.kind {

docs/mcp.md

@@ -5965,7 +5965,7 @@ while such an image that the art shows has a plate of its name; deleting the swa
 file's composite is drawn through clips the image (a clipping group, its path under the nonzero fill rule, an even-odd
 clipping path kept as the same area), and `file.place` answers the image's id as
 `image`, since the Links commands take the image and not the group. `links.update` and `links.relink` clip the image
-by the file's clipping path again (in a new clipping group when none holds it, and inside a clipping mask made in the
+by the file's clipping path again (a plain group holding just the image becomes its clipping group, a new clipping group holds it when no group does, and it goes inside a clipping mask made in the
 document, whose path is kept), unless Placement Options clip the image, the same way for the images in symbols, graph designs, patterns and opacity masks; when the new file has no
 clipping path, a clipping group holding just the image whose path is the old file's clipping path loses that path and
 stays as a plain group of the image, with its id, name, visibility and transparency (one whose path was given the