Merge pull request #2043 from midasmatterhorn/fix/issue-1732-transform-again-after-transform-each

transform: Transform Again repeats a Transform Each (#1732)

crates/engine/src/cmd/menucmds.rs

@@ -312,6 +312,19 @@ fn about(o: Point, a: Affine) -> Affine {
 }
 
 fn transform_each(s: &mut Session, p: &Value) -> Result<Value> {
+    transform_each_as(s, p, "Transform Each")
+}
+
+/// Object → Transform → Transform Again (⌘D) repeating a Transform Each: the same per-object
+/// transform again (or another copy), run as the command so each object turns about its own
+/// bounds (see [`transform_again`](super::object::transform_again)).
+pub(crate) fn transform_each_again(s: &mut Session, p: &Value) -> Result<Value> {
+    transform_each_as(s, p, "Transform Again")
+}
+
+/// [`transform_each`] under an undo label. Outside an interaction it records what Transform Again
+/// repeats; a dialog's preview records it as it commits (`Session::commit_interaction`).
+fn transform_each_as(s: &mut Session, p: &Value, label: &str) -> Result<Value> {
     let roots = selected_roots(s)?;
     // Bounded so coordinates stay finite (and the document stays serializable).
     let sh = f64_or(p, "scaleH", 100.0).clamp(-100_000.0, 100_000.0);
@@ -337,7 +350,7 @@ fn transform_each(s: &mut Session, p: &Value) -> Result<Value> {
     if roots.is_empty() || (!copy && sh == 100.0 && sv == 100.0 && mh == 0.0 && mv == 0.0 && rot == 0.0 && !rx && !ry) {
         return Ok(ids_json(&roots));
     }
-    let ids = s.edit("Transform Each", |d, sel| {
+    let ids = s.edit(label, |d, sel| {
         let targets = if copy { duplicate_in(d, sel, &roots, Affine::IDENTITY)? } else { roots.clone() };
         for id in &targets {
             let Some(b) = d.node(*id).and_then(|n| n.geometric_bounds()) else { continue };
@@ -359,6 +372,11 @@ fn transform_each(s: &mut Session, p: &Value) -> Result<Value> {
         }
         Ok(targets)
     })?;
+    let st = s.doc_mut()?;
+    if st.interaction.is_none() {
+        st.last_transform = Some(crate::LastTransform::Each(p.clone()));
+        st.last_perspective = None;
+    }
     Ok(ids_json(&ids))
 }
 

crates/engine/src/cmd/object.rs

@@ -14,7 +14,7 @@ use vectorcraft_geom::{Affine, FillRule, PathData, Point, Rect, Vec2};
 use super::edit::{duplicate_in, selected_roots};
 use super::opacitymask::percent;
 use super::*;
-use crate::EngineError;
+use crate::{EngineError, LastTransform};
 
 pub fn specs() -> Vec<CommandSpec> {
     vec![
@@ -342,7 +342,7 @@ pub(crate) fn apply_transform(s: &mut Session, label: &str, ids: Vec<NodeId>, xf
             options["strokes"] = json!(sc.strokes);
             options["corners"] = json!(!sc.keep_corners);
         }
-        st.last_transform = Some((xf, options));
+        st.last_transform = Some(LastTransform::Matrix(xf, options));
         st.last_perspective = None;
     }
     Ok(json!({ "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>() }))
@@ -478,18 +478,27 @@ fn shear(s: &mut Session, p: &Value) -> Result<Value> {
     apply_transform(s, "Shear", ids, about(o, m), p, true)
 }
 
-/// The last transform's matrix on the selection, with the options `last_transform` keeps for it; an
-/// option that `p` sets to true or false (a recorded Transform Again's) takes precedence.
+/// The last transform on the selection, with the options `last_transform` keeps for it; an option
+/// that `p` sets to true or false (a recorded Transform Again's) takes precedence. A Transform
+/// Each is repeated by running it again (its transform is per object, about each one's bounds).
 fn transform_again(s: &mut Session, p: &Value) -> Result<Value> {
     if let Some(again) = s.doc()?.last_perspective.clone() {
         return super::distortcmds::transform_again(s, &again);
     }
-    let (m, mut options) = s.doc()?.last_transform.clone().ok_or_else(|| EngineError::Other("no previous transform".into()))?;
-    if let (Some(o), Some(given)) = (options.as_object_mut(), p.as_object()) {
-        o.extend(given.iter().filter(|(_, v)| v.is_boolean()).map(|(k, v)| (k.clone(), v.clone())));
+    let last = s.doc()?.last_transform.clone().ok_or_else(|| EngineError::Other("no previous transform".into()))?;
+    let merge = |mut options: Value| {
+        if let (Some(o), Some(given)) = (options.as_object_mut(), p.as_object()) {
+            o.extend(given.iter().filter(|(_, v)| v.is_boolean()).map(|(k, v)| (k.clone(), v.clone())));
+        }
+        options
+    };
+    match last {
+        LastTransform::Matrix(m, options) => {
+            let ids = selected_roots(s)?;
+            apply_transform(s, "Transform Again", ids, m, &merge(options), true)
+        }
+        LastTransform::Each(params) => super::menucmds::transform_each_again(s, &merge(params)),
     }
-    let ids = selected_roots(s)?;
-    apply_transform(s, "Transform Again", ids, m, &options, true)
 }
 
 enum Arrange {

crates/engine/src/lib.rs

@@ -223,6 +223,16 @@ pub struct Interaction {
     pub(crate) copies: Option<Arc<PreviewCopies>>,
 }
 
+/// What Object → Transform → Transform Again (⌘D) repeats ([`SessionState::last_transform`]).
+#[derive(Clone, Debug, PartialEq)]
+pub enum LastTransform {
+    /// The last transform command's matrix, and the options Transform Again passes with it.
+    Matrix(Affine, Value),
+    /// The last Transform Each's parameters: each object transforms about its own bounds, so the
+    /// repeat runs the command on the selection of the moment rather than one matrix.
+    Each(Value),
+}
+
 /// This holds the copies that a transform with `copy` made of an interaction's snapshot
 /// ([`cmd::object::apply_transform`]). A later transform with `copy` of the same objects, while
 /// the document is still that snapshot, starts from `doc` and transforms only the copies.
@@ -280,11 +290,11 @@ pub struct DocState {
     pub interaction: Option<Interaction>,
     /// Edits made while this is open are one undo step (a scrubbed numeric field).
     pub undo_group: Option<UndoGroup>,
-    /// For Object → Transform → Transform Again (⌘D): the last transform's matrix and the options
-    /// Transform Again passes with it. A transform command keeps the `copy`, `typeAreas` and
-    /// `patterns` it used, and `strokes` and `corners` when it scales; a drag keeps the params of its
-    /// last preview with the values that preview noted ([`Session::note_journal`]).
-    pub last_transform: Option<(Affine, Value)>,
+    /// For Object → Transform → Transform Again (⌘D): what the last transformation was. A
+    /// transform command keeps the `copy`, `typeAreas` and `patterns` it used, and `strokes` and
+    /// `corners` when it scales; a drag keeps the params of its last preview with the values that
+    /// preview noted ([`Session::note_journal`]).
+    pub last_transform: Option<LastTransform>,
     /// What Select → Reselect repeats: the last selection command, its params and the objects
     /// selected when it ran (a Same command's reference objects).
     pub last_selection_cmd: Option<(String, Value, Vec<NodeId>)>,
@@ -1622,6 +1632,9 @@ impl Session {
             ("Copy", false) if preview.0 == "object.transform" => it.label = "Move".into(),
             _ => {}
         }
+        // A preview that left the document as it was records no Transform Again either (as
+        // `keep_interaction` records no undo step): the last real transformation stands.
+        let changed = !Arc::ptr_eq(&it.doc, &st.doc);
         st.keep_interaction(it);
         if preview.0 == "object.transform" {
             let m = cmd::matrix_param(&preview.1, "matrix");
@@ -1633,9 +1646,13 @@ impl Session {
                     o.remove("matrix");
                     o.remove("ids");
                 }
-                st.last_transform = Some((m, options));
+                st.last_transform = Some(LastTransform::Matrix(m, options));
                 st.last_perspective = None;
             }
+        } else if preview.0 == "object.transformEach" && changed {
+            // Transform Again repeats it as it stood when OK was pressed.
+            st.last_transform = Some(LastTransform::Each(preview.1.clone()));
+            st.last_perspective = None;
         }
         if perspective_again.is_some() {
             st.last_perspective = perspective_again;

crates/engine/src/tests.rs

@@ -714,7 +714,7 @@ fn a_transform_that_moves_nothing_records_no_undo_step() {
     }
     assert_eq!(
         s.doc().unwrap().last_transform,
-        Some((Affine::IDENTITY, json!({"copy": false, "typeAreas": false, "patterns": false, "objects": true})))
+        Some(LastTransform::Matrix(Affine::IDENTITY, json!({"copy": false, "typeAreas": false, "patterns": false, "objects": true})))
     );
     // A Nudge of 0 with an anchor of the rectangle direct-selected keeps it a live shape.
     let is_live = |s: &Session| matches!(s.doc().unwrap().doc.node(a).unwrap().kind, NodeKind::Path { live: Some(_), .. });

crates/engine/src/tests_menucmds.rs

@@ -388,6 +388,57 @@ fn transform_each_that_changes_nothing_records_no_undo_step() {
     assert_eq!(history(&s), (before.0 + 1, 0, true));
 }
 
+/// #1732: Transform Again (⌘D) repeats a Transform Each — the same transform on the objects
+/// again, another copy when the Transform Each made copies — rather than an earlier transform.
+#[test]
+fn transform_again_repeats_transform_each() {
+    let mut s = session();
+    let a = rect(&mut s, 0.0, 0.0, 100.0, 100.0);
+    sel(&mut s, &[a]);
+    // An earlier Transform must not be what Transform Again repeats.
+    s.execute("object.transform", &json!({"matrix": [1, 0, 0, 1, 5, 5]})).unwrap();
+    s.execute("object.transformEach", &json!({"moveH": 50})).unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(55.0, 5.0, 155.0, 105.0));
+    s.execute("object.transformAgain", &json!({})).unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(105.0, 5.0, 205.0, 105.0), "50 pt more, not 5");
+    s.execute("object.transformAgain", &json!({})).unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(155.0, 5.0, 255.0, 105.0), "again and again");
+    // With Copy, every Transform Again makes another copy, further on each time.
+    let mut s = session();
+    let a = rect(&mut s, 0.0, 0.0, 100.0, 100.0);
+    sel(&mut s, &[a]);
+    let r = s.execute("object.transformEach", &json!({"moveH": 50, "copy": true})).unwrap();
+    let first = NodeId(r["ids"][0].as_u64().unwrap());
+    assert_eq!(bounds(&s, first), Rect::new(50.0, 0.0, 150.0, 100.0));
+    let r = s.execute("object.transformAgain", &json!({})).unwrap();
+    let second = NodeId(r["ids"][0].as_u64().unwrap());
+    assert_ne!(second, first);
+    assert_eq!(bounds(&s, second), Rect::new(100.0, 0.0, 200.0, 100.0), "another copy, 50 pt further");
+    assert_eq!(bounds(&s, a), Rect::new(0.0, 0.0, 100.0, 100.0), "the original stays");
+    assert_eq!(bounds(&s, first), Rect::new(50.0, 0.0, 150.0, 100.0), "and the first copy too");
+}
+
+/// #1732: the Transform Each dialog previews through an interaction; OK commits it, and
+/// Transform Again repeats what the commit recorded. A preview that changed nothing doesn't take
+/// the record over (it records no undo step either).
+#[test]
+fn transform_again_repeats_a_committed_transform_each_preview() {
+    let mut s = session();
+    let a = rect(&mut s, 0.0, 0.0, 100.0, 100.0);
+    sel(&mut s, &[a]);
+    s.begin_interaction("Transform Each").unwrap();
+    s.preview("object.transformEach", &json!({"moveH": 50})).unwrap();
+    s.commit_interaction().unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(50.0, 0.0, 150.0, 100.0));
+    s.execute("object.transformAgain", &json!({})).unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(100.0, 0.0, 200.0, 100.0), "the dialog's Transform Each repeats");
+    s.begin_interaction("Transform Each").unwrap();
+    s.preview("object.transformEach", &json!({})).unwrap();
+    s.commit_interaction().unwrap();
+    s.execute("object.transformAgain", &json!({})).unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(150.0, 0.0, 250.0, 100.0), "still the one that changed something");
+}
+
 #[test]
 fn reset_bounding_box_is_a_noop() {
     let mut s = session();