From e3e218a8fc9835c2c88c5f9cf48025d64d5caa1d Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 04:00:58 -0500 Subject: [PATCH] Shape edits: edit each target once and read its box without copying it (#1769) * engine: edit each target shape once and read its box without copying it * engine: check the fill colour field in the repeated-ids test * Apply review fixes - crates/engine/src/tests.rs * address review: shape.lock skips targets inside a targeted group * address review: dedupe shape.lock targets before walking groups --------- Co-authored-by: aiongg <67543387+aiongg@users.noreply.github.com> --- crates/engine/src/cmd.rs | 18 +++++++++++------- crates/engine/src/cmd/selection_tests.rs | 8 ++++++++ crates/engine/src/cmd/shape.rs | 19 +++++++++++++++++++ crates/engine/src/tests.rs | 12 ++++++++++++ 4 files changed, 50 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 6f20c4143..e3a50f689 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -466,22 +466,26 @@ fn ctx_xfrm(doc: &deckcraft_model::Presentation, sel: &crate::Selection, shape: /// Run `f` on each target shape (copy-on-write), materialising inherited boxes first. pub(crate) fn edit_shapes(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut deckcraft_model::Shape) -> Result<()>) -> Result { - let ids = targets(s, p)?; + let mut ids = targets(s, p)?; if ids.is_empty() { return Err(bad(cmd, "no shapes selected")); } + // A shape named twice is edited once. + let mut seen = std::collections::HashSet::new(); + ids.retain(|id| seen.insert(*id)); s.edit(|doc, sel| { - let boxes: Vec<(ShapeId, Xfrm)> = { - let st_shapes: Vec = - ids.iter().filter_map(|id| current_shapes(doc, sel).and_then(|v| deckcraft_model::find_shape(v, *id)).cloned()).collect(); - st_shapes.iter().map(|sh| (sh.id, xfrm_of(doc, sel, sh))).collect() - }; + // The boxes are read from the shapes in place: no copy of a shape (and its text) per id. + let boxes: std::collections::HashMap = ids + .iter() + .filter_map(|id| current_shapes(doc, sel).and_then(|v| deckcraft_model::find_shape(v, *id))) + .map(|sh| (sh.id, xfrm_of(doc, sel, sh))) + .collect(); let shapes = crate::shapes_mut(doc, sel).ok_or_else(|| bad(cmd, "no slide"))?; let mut n = 0; for id in &ids { if let Some(sh) = deckcraft_model::find_shape_mut(shapes, *id) { if sh.xfrm.is_none() { - sh.xfrm = boxes.iter().find(|(i, _)| i == id).map(|(_, x)| *x); + sh.xfrm = boxes.get(id).copied(); } f(sh)?; n += 1; diff --git a/crates/engine/src/cmd/selection_tests.rs b/crates/engine/src/cmd/selection_tests.rs index e4e2494b4..07bed2d97 100644 --- a/crates/engine/src/cmd/selection_tests.rs +++ b/crates/engine/src/cmd/selection_tests.rs @@ -135,6 +135,14 @@ fn eye_show_all_and_hide_all() { assert!(!flags(&s, a).0 && flags(&s, g).0 && flags(&s, e).0); } +#[test] +fn lock_group_with_its_child_targeted_leaves_child_locked() { + let (mut s, ids) = deck(); + let (d, g) = (ids[3], ids[7]); + s.execute("shape.lock", &json!({"ids": [g, d, d]})).unwrap(); + assert!(flags(&s, g).1 && flags(&s, d).1); +} + #[test] fn lock_cascades_through_groups_and_undoes() { let (mut s, ids) = deck(); diff --git a/crates/engine/src/cmd/shape.rs b/crates/engine/src/cmd/shape.rs index 78b6204bb..b7b3d3f05 100644 --- a/crates/engine/src/cmd/shape.rs +++ b/crates/engine/src/cmd/shape.rs @@ -374,6 +374,25 @@ fn visible(s: &mut Session, p: &Value) -> Result { /// (a child can still be unlocked on its own afterwards), as PowerPoint does. fn lock(s: &mut Session, p: &Value) -> Result { let v = bool_param(p, "locked"); + // A targeted shape inside another targeted group is reached through the group: editing it again + // would toggle it back. + let mut ids = super::targets(s, p)?; + if ids.len() > 1 { + let doc = s.doc()?; + // Walk each distinct group once, however often `ids` repeats it. + let mut once = std::collections::HashSet::new(); + ids.retain(|id| once.insert(*id)); + let mut inside = std::collections::HashSet::new(); + for id in &ids { + if let Some(g) = doc.shape(*id) { + deckcraft_model::walk(g.children(), &mut |c, _| { + inside.insert(c.id); + }); + } + } + ids.retain(|id| !inside.contains(id)); + } + let p = &json!({"ids": ids, "locked": p.get("locked")}); edit_shapes(s, p, "shape.lock", |sh| { let to = v.unwrap_or(!sh.locked); set_locked(sh, to, 0); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 4cdff41ff..4f40b2dfb 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6921,6 +6921,18 @@ fn master_body_placeholder_format_keeps_per_level_formatting() { assert_eq!(shown(&s), vec![Some(28.0), Some(24.0)]); } +/// A shape named many times in one edit is edited once and not copied per mention. +#[test] +fn repeated_target_ids_edit_the_shape_once() { + let mut s = session(); + let r = s.execute("shape.insert", &json!({"preset": "rect", "rect": [100, 100, 300, 100], "text": "x".repeat(100_000)})).unwrap(); + let id = r["id"].as_u64().unwrap(); + let edited = s.execute("shape.fill", &json!({"ids": vec![id; 50_000], "color": "#ff0000"})).unwrap(); + assert_eq!(edited, json!(1)); + let v = s.execute("shape.inspect", &json!({"id": id})).unwrap(); + assert_eq!(v["shape"]["fill"]["color"]["base"]["rgb"], json!({"a": 255, "b": 0, "g": 0, "r": 255})); +} + #[test] fn many_distinct_fonts_are_listed_once_each_without_a_quadratic_scan() { // 30,000 runs in 15,000 families, each written twice in different case.