From af9f522c6a9d6be58017a55136422d0ee3512b93 Mon Sep 17 00:00:00 2001 From: Zach Berry <1112351+zberry@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:48:16 -0600 Subject: [PATCH 1/3] shape.connect glues to a shape inside a group, and the connector follows the group An agent run found that connecting to a grouped shape answered ok but left the connector's end unset: the sites of a group member were computed in the group's child space, and rerouting only looked at the slide's top-level shapes, so the glue was dropped at once. Both now locate the shape anywhere in the tree with the affine onto the slide, as PowerPoint glues to grouped shapes. shape.sites answers in the same slide space. A line inside a group is refused as `id`: its ends would land in the group's space, and rerouting does not follow grouped connectors. Split out of the connector default sites PR at review. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/shape/freeform.rs | 19 ++++++-- crates/engine/src/connect.rs | 49 ++++++++++++++++++-- crates/engine/src/tests.rs | 61 +++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/shape/freeform.rs b/crates/engine/src/cmd/shape/freeform.rs index f3be4f4bb..c0430c85f 100644 --- a/crates/engine/src/cmd/shape/freeform.rs +++ b/crates/engine/src/cmd/shape/freeform.rs @@ -3,8 +3,9 @@ use super::{ColorRef, Fill, Geom, Line, NO_ID_SPACE, Result, Session, Value, bad pub(super) fn sites(s: &mut Session, p: &Value) -> Result { let id = id_param(p, "id", "shape.sites")?.ok_or_else(|| bad("shape.sites", "missing `id`"))?; let st = s.doc()?; - let sh = st.shape(id).ok_or_else(|| bad("shape.sites", "no such shape"))?; - Ok(json!(crate::connect::sites(&st.doc, &st.selection, sh).iter().map(|q| [q.x, q.y]).collect::>())) + // In slide space, as shape.connect uses them, for a shape inside a group too. + let v = crate::connect::sites_of(&st.doc, &st.selection, st.shapes(), id).ok_or_else(|| bad("shape.sites", "no such shape"))?; + Ok(json!(v.iter().map(|q| [q.x, q.y]).collect::>())) } pub(super) fn connect(s: &mut Session, p: &Value) -> Result { @@ -12,8 +13,13 @@ pub(super) fn connect(s: &mut Session, p: &Value) -> Result { return Err(bad("shape.connect", "missing `from` / `to`")); }; let st = s.doc()?; - let site_list = |id| st.shape(id).map(|sh| crate::connect::sites(&st.doc, &st.selection, sh)).unwrap_or_default(); - let (a, b) = (site_list(from), site_list(to)); + // Anywhere in the slide's tree: a shape inside a group takes a connector too (PowerPoint + // glues to grouped shapes), its sites where the group draws them. + let site_list = |id| { + crate::connect::sites_of(&st.doc, &st.selection, st.shapes(), id) + .ok_or_else(|| bad("shape.connect", format!("no shape {} on the current slide", id.0))) + }; + let (a, b) = (site_list(from)?, site_list(to)?); if a.is_empty() || b.is_empty() { return Err(bad("shape.connect", "both shapes need connection sites (lines and groups have none)")); } @@ -35,6 +41,11 @@ pub(super) fn connect(s: &mut Session, p: &Value) -> Result { }; let (p0, p1) = (a[ia], b[ib]); let id = match id_param(p, "id", "shape.connect")? { + // The ends are in slide space and rerouting follows the slide's own connectors, so a line + // inside a group would be drawn in the group's space and never follow its shapes. + Some(id) if !st.shapes().iter().any(|sh| sh.id == id) && st.shape(id).is_some() => { + return Err(bad("shape.connect", "`id` names a line inside a group; ungroup it first, or leave `id` out for a new connector")); + } Some(id) => id, None => { let preset = str_param(p, "preset").unwrap_or("straightConnector1"); diff --git a/crates/engine/src/connect.rs b/crates/engine/src/connect.rs index 8cdbecda8..5991ba619 100644 --- a/crates/engine/src/connect.rs +++ b/crates/engine/src/connect.rs @@ -1,14 +1,21 @@ //! Connectors glued to shapes: connection sites, snapping line ends to them, and rerouting attached //! connectors whenever the shapes they connect move, resize or rotate. -use deckcraft_geom::{Point, Xfrm}; +use deckcraft_geom::{Affine, Point, Rect, Xfrm}; use deckcraft_model::{Presentation, Shape, ShapeId, ShapeKind}; use crate::Selection; use crate::cmd::xfrm_of; -/// Connection sites of `s` in slide coordinates (index = site index stored on connectors). +/// Connection sites of a top-level shape `s` in slide coordinates (index = site index stored on +/// connectors). For a shape inside a group use [`sites_of`] or [`sites_in`]. pub fn sites(doc: &Presentation, sel: &Selection, s: &Shape) -> Vec { + sites_in(doc, sel, s, Affine::IDENTITY) +} + +/// Connection sites of `s` in slide coordinates, `to_abs` mapping the space its box lives in (the +/// slide, or the child space of the groups around it) onto the slide. +pub fn sites_in(doc: &Presentation, sel: &Selection, s: &Shape, to_abs: Affine) -> Vec { if s.is_line() || matches!(s.kind, ShapeKind::Group { .. } | ShapeKind::Ink { .. }) { return vec![]; } @@ -17,10 +24,42 @@ pub fn sites(doc: &Presentation, sel: &Selection, s: &Shape) -> Vec { if local.is_empty() { local = vec![Point::new(x.w / 2.0, 0.0), Point::new(0.0, x.h / 2.0), Point::new(x.w / 2.0, x.h), Point::new(x.w, x.h / 2.0)]; } - let a = x.affine(); + let a = to_abs * x.affine(); local.into_iter().map(|p| a * p).collect() } +/// The shape `id` anywhere in `shapes` (a group's member included) with the affine that maps the +/// space its box lives in onto the slide: a connector glues to a grouped shape, as in PowerPoint, +/// and its sites are where the group draws them. +pub fn locate<'a>(doc: &Presentation, sel: &Selection, shapes: &'a [Shape], id: ShapeId) -> Option<(&'a Shape, Affine)> { + fn rec<'a>(doc: &Presentation, sel: &Selection, shapes: &'a [Shape], id: ShapeId, to_abs: Affine, depth: usize) -> Option<(&'a Shape, Affine)> { + if depth > 64 { + return None; + } + for s in shapes { + if s.id == id { + return Some((s, to_abs)); + } + if let ShapeKind::Group { children, child } = &s.kind { + let x = xfrm_of(doc, sel, s); + let ch = Rect::new(child.x, child.y, child.x + child.w, child.y + child.h); + let nested = to_abs * deckcraft_geom::group_child_affine(&x, ch); + if let Some(f) = rec(doc, sel, children, id, nested, depth + 1) { + return Some(f); + } + } + } + None + } + rec(doc, sel, shapes, id, Affine::IDENTITY, 0) +} + +/// Connection sites of shape `id` anywhere in `shapes`, in slide coordinates; `None` when there is +/// no such shape. +pub fn sites_of(doc: &Presentation, sel: &Selection, shapes: &[Shape], id: ShapeId) -> Option> { + locate(doc, sel, shapes, id).map(|(s, a)| sites_in(doc, sel, s, a)) +} + /// The nearest connection site within `radius` of `p` on any visible shape except `exclude`. pub fn nearest_site( doc: &Presentation, @@ -68,8 +107,8 @@ pub fn reroute(doc: &mut Presentation, sel: &Selection) { let snapshot: Vec = list.clone(); let view = doc.clone(); let site = |id: ShapeId, idx: u32| -> Option { - let s = snapshot.iter().find(|s| s.id == id)?; - let v = sites(&view, sel, s); + // Anywhere in the tree: a connector glued to a shape that is then grouped keeps following it. + let v = sites_of(&view, sel, &snapshot, id)?; v.get(idx as usize).or(v.first()).copied() }; let mut updates: Vec<(usize, Xfrm, bool, bool)> = vec![]; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 12fac5efc..63b90abb0 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6509,3 +6509,64 @@ fn save_to_image_path_is_an_error_not_zip_bytes() { } let _ = std::fs::remove_dir_all(&dir); } + +/// `shape.connect` glues to a shape inside a group, as PowerPoint does: the connector's +/// end is set to the grouped shape and follows it when the group moves. The glue was dropped at +/// once (the end stayed `None`) because rerouting only looked at the slide's top-level shapes, +/// and the sites of a grouped shape were computed in the group's child space. +#[test] +fn shape_connect_glues_to_a_shape_inside_a_group() { + use deckcraft_model::{ShapeId, ShapeKind}; + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + let a = s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 10, 100, 50]})).unwrap()["id"].as_u64().unwrap(); + let b = s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 200, 100, 50]})).unwrap()["id"].as_u64().unwrap(); + let g = s.execute("arrange.group", &json!({"ids": [a, b]})).unwrap()["id"].as_u64().unwrap(); + let c = s.execute("shape.insert", &json!({"preset": "rect", "rect": [400, 10, 100, 50]})).unwrap()["id"].as_u64().unwrap(); + let glue = |s: &Session, id: u64| { + let st = s.doc().unwrap(); + match st.shape(ShapeId(id as u32)).unwrap().kind { + ShapeKind::Connector { start, end } => (start.map(|(i, _)| i.0 as u64), end.map(|(i, _)| i.0 as u64)), + _ => (None, None), + } + }; + let near = |p: deckcraft_geom::Point, x: f64, y: f64| (p.x - x).abs() < 1e-6 && (p.y - y).abs() < 1e-6; + // A new connector into the group. + let r = s.execute("shape.connect", &json!({"from": c, "to": a})).unwrap(); + let k = r["id"].as_u64().unwrap(); + assert_eq!(glue(&s, k), (Some(c), Some(a)), "{r}"); + let (p0, p1) = line_ends(&s, k); + assert!(near(p0, 400.0, 35.0), "from C's left site: {p0:?}"); + assert!(near(p1, 110.0, 35.0), "to A's right site in slide space: {p1:?}"); + s.execute("shape.move", &json!({"ids": [g], "dy": 100})).unwrap(); + assert_eq!(glue(&s, k), (Some(c), Some(a)), "the glue survives the move"); + let (_, p1) = line_ends(&s, k); + assert!(near(p1, 110.0, 135.0), "the connector follows the grouped shape: {p1:?}"); + // Re-gluing an existing line with `id`. + let l = s.execute("shape.insert", &json!({"preset": "straightConnector1", "rect": [600, 400, 50, 50]})).unwrap()["id"].as_u64().unwrap(); + let r = s.execute("shape.connect", &json!({"id": l, "from": c, "to": b})).unwrap(); + assert_eq!(r["id"], l, "{r}"); + assert_eq!(glue(&s, l), (Some(c), Some(b)), "{r}"); + let (_, p1) = line_ends(&s, l); + assert!(near(p1, 110.0, 325.0), "to B's right site (C is mostly to its right) in slide space, B moved with the group: {p1:?}"); + s.execute("shape.move", &json!({"ids": [g], "dx": 50})).unwrap(); + assert_eq!(glue(&s, l), (Some(c), Some(b))); + let (_, p1) = line_ends(&s, l); + assert!(near(p1, 160.0, 325.0), "{p1:?}"); + // shape.sites answers in slide space, as shape.connect uses them: A's right site, A moved + // with the group. + let v = s.execute("shape.sites", &json!({"id": a})).unwrap(); + assert!( + v.as_array().unwrap().iter().any(|q| near(deckcraft_geom::Point::new(q[0].as_f64().unwrap(), q[1].as_f64().unwrap()), 160.0, 135.0)), + "{v}" + ); + // A line inside a group is refused as `id`: its ends would be written in slide space into + // the group's space, and rerouting does not follow grouped connectors. + let m = s.execute("shape.insert", &json!({"preset": "line", "rect": [600, 300, 50, 0]})).unwrap()["id"].as_u64().unwrap(); + let n = s.execute("shape.insert", &json!({"preset": "rect", "rect": [600, 450, 50, 50]})).unwrap()["id"].as_u64().unwrap(); + s.execute("arrange.group", &json!({"ids": [m, n]})).unwrap(); + let before = s.doc().unwrap().doc.clone(); + let e = s.execute("shape.connect", &json!({"id": m, "from": c, "to": b})).unwrap_err().to_string(); + assert!(e.contains("inside a group"), "{e}"); + assert_eq!(s.doc().unwrap().doc, before, "nothing changed"); +} From 75744dd67d2dbd4963ddf1b2b0d69d03e2b54ca3 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Thu, 5 Nov 2026 04:02:42 +0000 Subject: [PATCH 2/3] Apply review fixes - crates/engine/src/tools/drag.rs --- crates/engine/src/tools/drag.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/tools/drag.rs b/crates/engine/src/tools/drag.rs index dcee9cf7d..a1abcb93c 100644 --- a/crates/engine/src/tools/drag.rs +++ b/crates/engine/src/tools/drag.rs @@ -236,7 +236,12 @@ impl Session { } } // A connector dragged away from shapes that stay put comes unglued at those ends. - let moved: Vec = origs2.iter().map(|o| o.0).collect(); + let mut moved: Vec = vec![]; + for (id, _) in &origs2 { + if let Some(sh) = deckcraft_model::find_shape(list, *id) { + deckcraft_model::walk(std::slice::from_ref(sh), &mut |s, _| moved.push(s.id)); + } + } for sh in list.iter_mut().filter(|s| moved.contains(&s.id)) { if let ShapeKind::Connector { start, end } = &mut sh.kind { for c in [start, end] { From afc7f765c94c49eecd742647f33e4b2376f85e4b Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Thu, 5 Nov 2026 05:18:29 +0100 Subject: [PATCH 3/3] shape.connect: refuse a grouped non-line id as not a line --- crates/engine/src/cmd/shape/freeform.rs | 2 +- crates/engine/src/tests.rs | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/shape/freeform.rs b/crates/engine/src/cmd/shape/freeform.rs index c0430c85f..5fd299c9e 100644 --- a/crates/engine/src/cmd/shape/freeform.rs +++ b/crates/engine/src/cmd/shape/freeform.rs @@ -43,7 +43,7 @@ pub(super) fn connect(s: &mut Session, p: &Value) -> Result { let id = match id_param(p, "id", "shape.connect")? { // The ends are in slide space and rerouting follows the slide's own connectors, so a line // inside a group would be drawn in the group's space and never follow its shapes. - Some(id) if !st.shapes().iter().any(|sh| sh.id == id) && st.shape(id).is_some() => { + Some(id) if !st.shapes().iter().any(|sh| sh.id == id) && st.shape(id).is_some_and(|sh| sh.is_line()) => { return Err(bad("shape.connect", "`id` names a line inside a group; ungroup it first, or leave `id` out for a new connector")); } Some(id) => id, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index db23c803a..7f59ced53 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6569,6 +6569,10 @@ fn shape_connect_glues_to_a_shape_inside_a_group() { let e = s.execute("shape.connect", &json!({"id": m, "from": c, "to": b})).unwrap_err().to_string(); assert!(e.contains("inside a group"), "{e}"); assert_eq!(s.doc().unwrap().doc, before, "nothing changed"); + // A grouped shape that is not a line is refused as not a line, not as a grouped line. + let e = s.execute("shape.connect", &json!({"id": n, "from": c, "to": b})).unwrap_err().to_string(); + assert!(e.contains("not a line"), "{e}"); + assert_eq!(s.doc().unwrap().doc, before, "nothing changed"); } /// The outline exports (PowerPoint's outline view) holds titles and body text only: footer,