Merge pull request #1972 from scks1/fix/duplicate-group-member

engine: edit.duplicate copies a shape inside a group into its group

crates/engine/src/cmd/edit.rs

@@ -598,6 +598,16 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
     let (new_ids, made) = s.edit(|doc, sel| {
         let list = super::current_shapes(doc, sel).cloned().unwrap_or_default();
         let mut copies: Vec<Shape> = list.iter().filter(|x| ids.contains(&x.id)).cloned().collect();
+        // A shape inside a group is copied into that group, with the map from the group's space onto
+        // the slide so that its copy is offset on the slide as a top-level copy is.
+        let top = copies.len();
+        let mut to_slide = vec![None; top];
+        for id in grouped_targets(&list, &ids) {
+            if let Some((m, a)) = crate::connect::locate(doc, sel, &list, id) {
+                copies.push(m.clone());
+                to_slide.push(Some(a));
+            }
+        }
         let sources: Vec<ShapeId> = copies.iter().map(|c| c.id).collect();
         let boxes: Vec<_> = copies.iter().map(|c| xfrm_of(doc, sel, c)).collect();
         detach_on_slide(doc, sel, &mut copies);
@@ -609,10 +619,12 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
             })
             .and_then(|(_, _, pairs)| {
                 let (src, copy) = pairs.first()?;
-                let a = xfrm_of(doc, sel, list.iter().find(|x| x.id == *src)?);
-                let b = xfrm_of(doc, sel, list.iter().find(|x| x.id == *copy)?);
-                let d = (b.x - a.x, b.y - a.y);
-                (d.0.is_finite() && d.1.is_finite()).then_some(d)
+                // Measured on the slide, so the copy of a shape in a group repeats its drawn move.
+                let (a, to_a) = crate::connect::locate(doc, sel, &list, *src)?;
+                let (b, to_b) = crate::connect::locate(doc, sel, &list, *copy)?;
+                let (a, b) = (xfrm_of(doc, sel, a), xfrm_of(doc, sel, b));
+                let d = to_b * deckcraft_geom::Point::new(b.x, b.y) - to_a * deckcraft_geom::Point::new(a.x, a.y);
+                (d.x.is_finite() && d.y.is_finite()).then_some((d.x, d.y))
             })
             .unwrap_or((off, off));
         reid("edit.duplicate", doc, &mut copies)?;
@@ -627,7 +639,8 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
             rename_automatic(c, &mut taken, count);
             count += 1;
         });
-        for (c, b) in copies.iter_mut().zip(boxes) {
+        for ((c, b), to) in copies.iter_mut().zip(boxes).zip(&to_slide) {
+            let (dx, dy) = to.map_or((dx, dy), |a| offset_in(a, dx, dy));
             let mut x = b;
             x.x += dx;
             x.y += dy;
@@ -638,7 +651,12 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
         }
         let new_ids: Vec<ShapeId> = copies.iter().map(|c| c.id).collect();
         let shapes = crate::shapes_mut(doc, sel).ok_or_else(|| bad("edit.duplicate", "no slide"))?;
-        shapes.extend(copies);
+        let mut copies = copies.into_iter();
+        shapes.extend(copies.by_ref().take(top));
+        // A member's copy goes last among its group's children, as a top-level copy goes last on the slide.
+        for (c, src) in copies.zip(sources.iter().skip(top)) {
+            deckcraft_model::sibling_list_mut(shapes, *src).ok_or_else(|| bad("edit.duplicate", "no such shape"))?.push(c);
+        }
         sel.shapes = new_ids.clone();
         let made = (sel.slide, sel.target, sources.into_iter().zip(new_ids.iter().copied()).collect());
         Ok((new_ids, made))
@@ -647,6 +665,34 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({"ids": new_ids}))
 }
 
+/// The shapes in `ids` that are inside groups, in drawing order, without those in a group that is
+/// in `ids` too: they are copied along with it.
+fn grouped_targets(list: &[Shape], ids: &[ShapeId]) -> Vec<ShapeId> {
+    fn rec(shapes: &[Shape], ids: &[ShapeId], nested: bool, out: &mut Vec<ShapeId>, depth: usize) {
+        if depth > 64 {
+            return;
+        }
+        for sh in shapes {
+            if !ids.contains(&sh.id) {
+                rec(sh.children(), ids, true, out, depth + 1);
+            } else if nested {
+                out.push(sh.id);
+            }
+        }
+    }
+    let mut out = vec![];
+    rec(list, ids, false, &mut out, 0);
+    out
+}
+
+/// The offset in the space `to_slide` maps onto the slide that moves a shape by (`dx`, `dy`) on
+/// the slide. A group with no width or height draws nothing, and the offset stays as it is.
+fn offset_in(to_slide: deckcraft_geom::Affine, dx: f64, dy: f64) -> (f64, f64) {
+    let back = to_slide.inverse();
+    let v = back * deckcraft_geom::Point::new(dx, dy) - back * deckcraft_geom::Point::ZERO;
+    if v.x.is_finite() && v.y.is_finite() { (v.x, v.y) } else { (dx, dy) }
+}
+
 fn delete(s: &mut Session, p: &Value) -> Result<Value> {
     if s.doc()?.selection.text.is_some() && ids_param(p, "ids").is_none() {
         return super::text::delete_dir(s, "forward");

crates/engine/src/tests.rs

@@ -6686,6 +6686,66 @@ fn duplicating_a_moved_copy_repeats_the_move() {
     assert_eq!(s.doc().unwrap().doc.validate(), Vec::<String>::new());
 }
 
+/// Duplicating a shape inside a group puts the copy in that group, last among its children, and
+/// selects it. The copy is offset on the slide as a top-level copy is, however the group is moved,
+/// stretched or turned, and duplicating a moved copy repeats the move (#966). A shape whose group is
+/// duplicated as well goes along with the group's copy. Before, Duplicate looked only at the slide's
+/// top level: it answered `{ids: []}` and changed nothing.
+#[test]
+fn duplicating_a_shape_in_a_group_copies_it_into_the_group() {
+    fn rect(s: &mut Session, x: f64) -> u64 {
+        s.execute("shape.insert", &json!({"preset": "rect", "rect": [x, 100, 100, 100]})).unwrap()["id"].as_u64().unwrap()
+    }
+    let sid = |id: u64| ShapeId(u32::try_from(id).unwrap());
+    // How far shape `to` is drawn from shape `from` on the slide, the same for each of its sites.
+    let offset = |s: &Session, from: u64, to: u64| {
+        let st = s.doc().unwrap();
+        let sites = |id| connect::sites_of(&st.doc, &st.selection, st.shapes(), sid(id)).unwrap();
+        let (p, q) = (sites(from), sites(to));
+        assert_eq!(p.len(), q.len(), "{p:?} {q:?}");
+        let d = q[0] - p[0];
+        assert!(p.iter().zip(&q).all(|(p, q)| (*q - *p - d).hypot() < 1e-6), "{p:?} {q:?}");
+        (d.x, d.y)
+    };
+    let grouped = |edit: &str, p: Value| {
+        let mut s = session();
+        s.execute("slide.new", &json!({"layout": "blank"})).unwrap();
+        let (a, b) = (rect(&mut s, 100.0), rect(&mut s, 300.0));
+        let g = s.execute("arrange.group", &json!({"ids": [a, b]})).unwrap()["id"].as_u64().unwrap();
+        let mut p = p;
+        p["ids"] = json!([g]);
+        s.execute(edit, &p).unwrap();
+        (s, a, b, g)
+    };
+    for (edit, p) in [("shape.move", json!({"dx": 200})), ("shape.resize", json!({"w": 600})), ("shape.rotate", json!({"deg": 90}))] {
+        let (mut s, a, b, g) = grouped(edit, p);
+        let off = s.prefs.duplicate_offset;
+        let r = s.execute("edit.duplicate", &json!({"ids": [a]})).unwrap();
+        assert_eq!(r["ids"].as_array().map(Vec::len), Some(1), "{edit}: {r}");
+        let c = r["ids"][0].as_u64().unwrap();
+        assert_eq!(s.doc().unwrap().selection.shapes, [sid(c)]);
+        let st = s.doc().unwrap();
+        let order: Vec<ShapeId> = deckcraft_model::find_shape(st.shapes(), sid(g)).unwrap().children().iter().map(|x| x.id).collect();
+        assert_eq!(order, [sid(a), sid(b), sid(c)], "{edit}");
+        let (dx, dy) = offset(&s, a, c);
+        assert!((dx - off).abs() < 1e-6 && (dy - off).abs() < 1e-6, "{edit}: the copy is drawn ({dx}, {dy}) from the original");
+        // Moved, then duplicated again: the next copy is drawn as far from it again.
+        s.execute("shape.move", &json!({"ids": [c], "dx": 30})).unwrap();
+        let (mx, my) = offset(&s, a, c);
+        let d = s.execute("edit.duplicate", &json!({"ids": [c]})).unwrap()["ids"][0].as_u64().unwrap();
+        let (nx, ny) = offset(&s, c, d);
+        assert!((nx - mx).abs() < 1e-6 && (ny - my).abs() < 1e-6, "{edit}: moved ({mx}, {my}), then ({nx}, {ny})");
+        assert_eq!(s.doc().unwrap().doc.validate(), Vec::<String>::new());
+    }
+    let (mut s, a, _, g) = grouped("shape.move", json!({"dx": 200}));
+    let r = s.execute("edit.duplicate", &json!({"ids": [a, g]})).unwrap();
+    assert_eq!(r["ids"].as_array().map(Vec::len), Some(1), "{r}");
+    let st = s.doc().unwrap();
+    let count = |id: u64| deckcraft_model::find_shape(st.shapes(), sid(id)).unwrap().children().len();
+    assert_eq!((count(g), count(r["ids"][0].as_u64().unwrap())), (2, 2));
+    assert_eq!(st.shapes().len(), 2);
+}
+
 fn chart_of(s: &Session, id: u64) -> deckcraft_model::Chart {
     match &s.doc().unwrap().shape(deckcraft_model::ShapeId(id as u32)).unwrap().kind {
         deckcraft_model::ShapeKind::Chart(c) => (**c).clone(),