From 09548fe55c480a2363b3a7ac60d6f04cdbcd90cb Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:21:57 +0000 Subject: [PATCH 1/2] engine: edit.duplicate copies a shape inside a group into its group edit.duplicate took its copies from the slide's top-level list only, so duplicating a shape inside a group answered `{"ids": []}` and changed nothing. A shape inside a group is now copied into that group, after its other children, and the copy is selected and returned. The duplicate offset is converted into the group's space, so the copy is drawn as far from the original on the slide as a top-level copy is, however the groups around it are moved, stretched or turned. Duplicating a moved copy again repeats the move (#966), measured on the slide. A shape whose group is duplicated in the same call is copied only with the group's copy. Test: duplicating_a_shape_in_a_group_copies_it_into_the_group. --- crates/engine/src/cmd/edit.rs | 58 ++++++++++++++++++++++++++++++---- crates/engine/src/tests.rs | 59 +++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 743aa8362..1e065c53b 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -559,6 +559,16 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { let (new_ids, made) = s.edit(|doc, sel| { let list = super::current_shapes(doc, sel).cloned().unwrap_or_default(); let mut copies: Vec = 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 = 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); @@ -570,10 +580,12 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { }) .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)?; @@ -588,7 +600,8 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { 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; @@ -599,7 +612,12 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { } let new_ids: Vec = 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)) @@ -608,6 +626,34 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { 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 { + fn rec(shapes: &[Shape], ids: &[ShapeId], nested: bool, out: &mut Vec, 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 { if s.doc()?.selection.text.is_some() && ids_param(p, "ids").is_none() { return super::text::delete_dir(s, "forward"); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5d2310328..640c586dc 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6543,6 +6543,65 @@ fn duplicating_a_moved_copy_repeats_the_move() { assert_eq!(s.doc().unwrap().doc.validate(), Vec::::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)); + 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 = 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::::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(), From 2750e65096d98f7229e6ab7ee157df4928c8942b Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:01:23 -0500 Subject: [PATCH 2/2] engine: group duplicate test compares site counts The test compared the copy's connection sites with the original's through zip, which stops at the shorter list. It now checks that both have the same number of sites first. --- crates/engine/src/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 640c586dc..c66500aea 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6559,6 +6559,7 @@ fn duplicating_a_shape_in_a_group_copies_it_into_the_group() { 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)