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

@@ -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<Value> {
-    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<deckcraft_model::Shape> =
-                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<ShapeId, Xfrm> = 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;

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();

crates/engine/src/cmd/shape.rs

@@ -374,6 +374,25 @@ fn visible(s: &mut Session, p: &Value) -> Result<Value> {
 /// (a child can still be unlocked on its own afterwards), as PowerPoint does.
 fn lock(s: &mut Session, p: &Value) -> Result<Value> {
     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);

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.