From 1c8b14edadd014b9ef60aff25e19a1cb65f0618f Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:21:58 +0000 Subject: [PATCH 1/5] engine: arrange.ungroup frees a group nested in another group arrange.ungroup looked only at the slide's top-level list, so a group inside another group answered `{"ids": []}`, stayed grouped, and the selection was emptied. Such a group's children now take its place among the outer group's children, with their boxes mapped into the outer group's space by child_to_parent, as for a top-level group, so they are drawn where they were. They are selected and returned. A shape freed from a named group and then from a named group around it is listed once. Test: ungrouping_a_nested_group_frees_its_shapes_into_the_outer_group. --- crates/engine/src/cmd/arrange.rs | 31 ++++++++++++++ crates/engine/src/tests.rs | 71 ++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/crates/engine/src/cmd/arrange.rs b/crates/engine/src/cmd/arrange.rs index 75c9b79d4..47b4110d4 100644 --- a/crates/engine/src/cmd/arrange.rs +++ b/crates/engine/src/cmd/arrange.rs @@ -365,6 +365,37 @@ fn ungroup(s: &mut Session, p: &Value) -> Result { out.push(sh); } *list = out; + // A group inside a group: its children take its place in the outer group, mapped into the + // outer group's child space, where the inner group's own box is. + for id in &ids { + let Some(siblings) = deckcraft_model::sibling_list_mut(list, *id) else { continue }; + let Some(at) = siblings.iter().position(|x| x.id == *id) else { continue }; + let freed: Vec = match siblings.get(at) { + Some(Shape { kind: ShapeKind::Group { children, child }, xfrm: Some(gx), .. }) => children + .iter() + .map(|c| { + let mut c = c.clone(); + if let Some(cx) = c.xfrm { + c.xfrm = Some(child_to_parent(gx, child, &cx)); + } + c + }) + .collect(), + _ => continue, + }; + let freed_ids: Vec = freed.iter().map(|c| c.id).collect(); + siblings.splice(at..=at, freed); + // Ungrouped along with its outer group, it was listed among that group's children. + match new_sel.iter().position(|x| x == id) { + Some(i) => { + new_sel.splice(i..=i, freed_ids); + } + None => new_sel.extend(freed_ids), + } + } + // A shape freed from a named group and then from a named group around it is listed once. + let mut listed = std::collections::HashSet::new(); + new_sel.retain(|x| listed.insert(*x)); sel.shapes = new_sel.clone(); Ok(json!({"ids": new_sel})) }) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5d2310328..771268ae4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -885,6 +885,77 @@ fn group_ungroup_roundtrip_positions() { assert!((v["box"]["x"].as_f64().unwrap() - 310.0).abs() < 1e-6, "{v}"); } +/// Ungrouping a group inside another group frees its shapes into the outer group, in its place +/// in the outer group's order, drawn where they were, and selects them. Before, arrange.ungroup +/// looked only at the slide's top level: it answered `{ids: []}` and changed nothing. +#[test] +fn ungrouping_a_nested_group_frees_its_shapes_into_the_outer_group() { + fn rect(s: &mut Session, x: f64, y: f64) -> u64 { + s.execute("shape.insert", &json!({"preset": "rect", "rect": [x, y, 100, 100]})).unwrap()["id"].as_u64().unwrap() + } + let sid = |id: u64| ShapeId(u32::try_from(id).unwrap()); + let sites = |s: &Session, id: u64| { + let st = s.doc().unwrap(); + connect::sites_of(&st.doc, &st.selection, st.shapes(), sid(id)).unwrap() + }; + // The outer group moved and stretched; then an inner group turned and flipped, inside an outer + // group that is turned and moved. + type Edits<'a> = &'a [(&'a str, Value)]; + let cases: [(Edits<'_>, Edits<'_>); 2] = [ + (&[], &[("shape.move", json!({"dx": 200})), ("shape.resize", json!({"w": 450}))]), + ( + &[("shape.rotate", json!({"deg": 30})), ("shape.flip", json!({"axis": "horizontal"}))], + &[("shape.rotate", json!({"deg": 45})), ("shape.move", json!({"dx": 120, "dy": 40}))], + ), + ]; + let edit = |s: &mut Session, id: u64, edits: Edits<'_>| { + for (cmd, p) in edits { + let mut p = p.clone(); + p["ids"] = json!([id]); + s.execute(cmd, &p).unwrap(); + } + }; + for (n, (inner_edits, outer_edits)) in cases.into_iter().enumerate() { + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + let (a, b, c) = (rect(&mut s, 100.0, 100.0), rect(&mut s, 300.0, 100.0), rect(&mut s, 100.0, 300.0)); + let inner = s.execute("arrange.group", &json!({"ids": [a, b]})).unwrap()["id"].as_u64().unwrap(); + edit(&mut s, inner, inner_edits); + let outer = s.execute("arrange.group", &json!({"ids": [inner, c]})).unwrap()["id"].as_u64().unwrap(); + edit(&mut s, outer, outer_edits); + let before = [sites(&s, a), sites(&s, b), sites(&s, c)]; + let r = s.execute("arrange.ungroup", &json!({"ids": [inner]})).unwrap(); + assert_eq!(r["ids"], json!([a, b]), "case {n}"); + assert_eq!(s.doc().unwrap().selection.shapes, [sid(a), sid(b)]); + let st = s.doc().unwrap(); + let order: Vec = deckcraft_model::find_shape(st.shapes(), sid(outer)).unwrap().children().iter().map(|x| x.id).collect(); + assert_eq!(order, [sid(a), sid(b), sid(c)], "case {n}"); + for (id, was) in [a, b, c].into_iter().zip(before) { + let now = sites(&s, id); + assert!(was.iter().zip(&now).all(|(p, q)| (*p - *q).hypot() < 1e-6), "case {n}: shape {id} moved from {was:?} to {now:?}"); + } + } + // Two named groups, one inside the other, inside a third: the shapes of both go to the third, + // each listed once. + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + let (a, b, c, d) = (rect(&mut s, 100.0, 100.0), rect(&mut s, 300.0, 100.0), rect(&mut s, 100.0, 300.0), rect(&mut s, 300.0, 300.0)); + let inner = s.execute("arrange.group", &json!({"ids": [a, b]})).unwrap()["id"].as_u64().unwrap(); + let middle = s.execute("arrange.group", &json!({"ids": [inner, c]})).unwrap()["id"].as_u64().unwrap(); + let outer = s.execute("arrange.group", &json!({"ids": [middle, d]})).unwrap()["id"].as_u64().unwrap(); + s.execute("shape.resize", &json!({"ids": [outer], "w": 600})).unwrap(); + let before = [a, b, c, d].map(|id| sites(&s, id)); + let r = s.execute("arrange.ungroup", &json!({"ids": [inner, middle]})).unwrap(); + assert_eq!(r["ids"], json!([a, b, c])); + let st = s.doc().unwrap(); + let order: Vec = deckcraft_model::find_shape(st.shapes(), sid(outer)).unwrap().children().iter().map(|x| x.id).collect(); + assert_eq!(order, [sid(a), sid(b), sid(c), sid(d)]); + for (id, was) in [a, b, c, d].into_iter().zip(before) { + let now = sites(&s, id); + assert!(was.iter().zip(&now).all(|(p, q)| (*p - *q).hypot() < 1e-6), "shape {id} moved from {was:?} to {now:?}"); + } +} + /// Grouping a group again and again stops at `MAX_GROUP_DEPTH`: past about 39 levels a saved /// .deckcraft file no longer opens, and past 64 commands such as format.languageAll miss the text. #[test] From de5398cfc589683e0ba3fd7cd5db77f11cc2af4f Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:01:24 -0500 Subject: [PATCH 2/5] engine: nested ungroup test compares site counts and checks the selection The test compared each shape's connection sites before and after through zip, which stops at the shorter list, so a shape that lost sites would still pass. It now checks the counts first. The case with three nested groups also checks that the freed shapes are selected. --- crates/engine/src/tests.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 771268ae4..6a4882839 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -932,6 +932,7 @@ fn ungrouping_a_nested_group_frees_its_shapes_into_the_outer_group() { assert_eq!(order, [sid(a), sid(b), sid(c)], "case {n}"); for (id, was) in [a, b, c].into_iter().zip(before) { let now = sites(&s, id); + assert_eq!(was.len(), now.len(), "case {n}: shape {id}"); assert!(was.iter().zip(&now).all(|(p, q)| (*p - *q).hypot() < 1e-6), "case {n}: shape {id} moved from {was:?} to {now:?}"); } } @@ -947,11 +948,13 @@ fn ungrouping_a_nested_group_frees_its_shapes_into_the_outer_group() { let before = [a, b, c, d].map(|id| sites(&s, id)); let r = s.execute("arrange.ungroup", &json!({"ids": [inner, middle]})).unwrap(); assert_eq!(r["ids"], json!([a, b, c])); + assert_eq!(s.doc().unwrap().selection.shapes, [sid(a), sid(b), sid(c)]); let st = s.doc().unwrap(); let order: Vec = deckcraft_model::find_shape(st.shapes(), sid(outer)).unwrap().children().iter().map(|x| x.id).collect(); assert_eq!(order, [sid(a), sid(b), sid(c), sid(d)]); for (id, was) in [a, b, c, d].into_iter().zip(before) { let now = sites(&s, id); + assert_eq!(was.len(), now.len(), "shape {id}"); assert!(was.iter().zip(&now).all(|(p, q)| (*p - *q).hypot() < 1e-6), "shape {id} moved from {was:?} to {now:?}"); } } From cdca5bd09de7d5a4c526afdf02f24c23e127348e Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Sun, 8 Nov 2026 14:56:11 +0000 Subject: [PATCH 3/5] Apply review fixes - crates/engine/src/cmd/arrange.rs --- crates/engine/src/cmd/arrange.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/arrange.rs b/crates/engine/src/cmd/arrange.rs index 47b4110d4..2f633501c 100644 --- a/crates/engine/src/cmd/arrange.rs +++ b/crates/engine/src/cmd/arrange.rs @@ -385,10 +385,11 @@ fn ungroup(s: &mut Session, p: &Value) -> Result { }; let freed_ids: Vec = freed.iter().map(|c| c.id).collect(); siblings.splice(at..=at, freed); - // Ungrouped along with its outer group, it was listed among that group's children. + // Ungrouped along with an outer group, it was listed among that group's children (once per expanded ancestor). match new_sel.iter().position(|x| x == id) { Some(i) => { new_sel.splice(i..=i, freed_ids); + new_sel.retain(|x| x != id); } None => new_sel.extend(freed_ids), } From 17db75b74a46c2c55caead33075b1e58d86d4d84 Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 8 Nov 2026 10:22:34 -0500 Subject: [PATCH 4/5] engine: test ungrouping nested groups named out of order Ungrouping a middle group, the group around it and then the innermost group, all inside a fourth group, returns the four freed rectangles once each, selects them, and leaves them in the fourth group in order. Before the previous commit, the innermost group was listed twice and replaced once, so the call also returned that group's id after the group was gone. --- crates/engine/src/tests.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6a4882839..d1ce63f5d 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -959,6 +959,33 @@ fn ungrouping_a_nested_group_frees_its_shapes_into_the_outer_group() { } } +/// Ungrouping nested groups in one call returns each freed shape once, whatever order the groups +/// are named in. Before, a group freed into a group that the same call freed next was listed twice +/// and replaced once, so the call returned the id of a group that no longer existed. +#[test] +fn ungrouping_nested_groups_in_any_order_returns_only_freed_shapes() { + fn rect(s: &mut Session, x: f64, y: f64) -> u64 { + s.execute("shape.insert", &json!({"preset": "rect", "rect": [x, y, 100, 100]})).unwrap()["id"].as_u64().unwrap() + } + fn group(s: &mut Session, ids: [u64; 2]) -> u64 { + s.execute("arrange.group", &json!({"ids": ids})).unwrap()["id"].as_u64().unwrap() + } + let sid = |id: u64| ShapeId(u32::try_from(id).unwrap()); + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + let [a, b, c, d, e] = [(100.0, 100.0), (300.0, 100.0), (100.0, 300.0), (300.0, 300.0), (500.0, 300.0)].map(|(x, y)| rect(&mut s, x, y)); + let inner = group(&mut s, [a, b]); + let middle = group(&mut s, [inner, c]); + let upper = group(&mut s, [middle, d]); + let outer = group(&mut s, [upper, e]); + let r = s.execute("arrange.ungroup", &json!({"ids": [middle, upper, inner]})).unwrap(); + assert_eq!(r["ids"], json!([a, b, c, d])); + assert_eq!(s.doc().unwrap().selection.shapes, [sid(a), sid(b), sid(c), sid(d)]); + let st = s.doc().unwrap(); + let order: Vec = deckcraft_model::find_shape(st.shapes(), sid(outer)).unwrap().children().iter().map(|x| x.id).collect(); + assert_eq!(order, [sid(a), sid(b), sid(c), sid(d), sid(e)]); +} + /// Grouping a group again and again stops at `MAX_GROUP_DEPTH`: past about 39 levels a saved /// .deckcraft file no longer opens, and past 64 commands such as format.languageAll miss the text. #[test] From 70a0d7a2d867cfa68f6dc901235daadc7cf03356 Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan <126534658+scks1@users.noreply.github.com> Date: Sun, 8 Nov 2026 15:36:50 -0500 Subject: [PATCH 5/5] engine: test ungrouping a top-level group with a group inside it Ungrouping a top-level group and a group inside it in one call, in either order, frees the shapes of both onto the slide in the top-level group's place, drawn where they were, returns them once each and selects them. The other ungroup tests leave the outermost group unnamed, so none checked that the inner group is found at the slide's top level after the same call frees the group around it. --- crates/engine/src/tests.rs | 47 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index d1ce63f5d..7a417ab29 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -986,6 +986,53 @@ fn ungrouping_nested_groups_in_any_order_returns_only_freed_shapes() { assert_eq!(order, [sid(a), sid(b), sid(c), sid(d), sid(e)]); } +/// Ungrouping a top-level group and a group inside it in one call frees the shapes of both onto +/// the slide, in the top-level group's place and drawn where they were, whichever is named first. +#[test] +fn ungrouping_a_top_level_group_with_a_group_inside_it_frees_both() { + fn rect(s: &mut Session, x: f64, y: f64) -> u64 { + s.execute("shape.insert", &json!({"preset": "rect", "rect": [x, y, 100, 100]})).unwrap()["id"].as_u64().unwrap() + } + fn group(s: &mut Session, ids: [u64; 2]) -> u64 { + s.execute("arrange.group", &json!({"ids": ids})).unwrap()["id"].as_u64().unwrap() + } + let sid = |id: u64| ShapeId(u32::try_from(id).unwrap()); + let sites = |s: &Session, id: u64| { + let st = s.doc().unwrap(); + connect::sites_of(&st.doc, &st.selection, st.shapes(), sid(id)).unwrap() + }; + let roots = |s: &Session| -> Vec { s.doc().unwrap().shapes().iter().map(|x| x.id).collect() }; + for outer_first in [true, false] { + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + let [a, b, c, z] = [(100.0, 100.0), (300.0, 100.0), (100.0, 300.0), (500.0, 500.0)].map(|(x, y)| rect(&mut s, x, y)); + let inner = group(&mut s, [a, b]); + let outer = group(&mut s, [inner, c]); + s.execute("shape.resize", &json!({"ids": [outer], "w": 600})).unwrap(); + let before = [a, b, c].map(|id| sites(&s, id)); + let freed = [sid(a), sid(b), sid(c)]; + let mut expected = vec![]; + for x in roots(&s) { + if x == sid(outer) { + expected.extend(freed); + } else { + expected.push(x); + } + } + assert!(expected.contains(&sid(z))); + let ids = if outer_first { [outer, inner] } else { [inner, outer] }; + let r = s.execute("arrange.ungroup", &json!({"ids": ids})).unwrap(); + assert_eq!(r["ids"], json!([a, b, c]), "{ids:?}"); + assert_eq!(s.doc().unwrap().selection.shapes, freed, "{ids:?}"); + assert_eq!(roots(&s), expected, "{ids:?}"); + for (id, was) in [a, b, c].into_iter().zip(before) { + let now = sites(&s, id); + assert_eq!(was.len(), now.len(), "{ids:?}: shape {id}"); + assert!(was.iter().zip(&now).all(|(p, q)| (*p - *q).hypot() < 1e-6), "{ids:?}: shape {id} moved from {was:?} to {now:?}"); + } + } +} + /// Grouping a group again and again stops at `MAX_GROUP_DEPTH`: past about 39 levels a saved /// .deckcraft file no longer opens, and past 64 commands such as format.languageAll miss the text. #[test]