#4001: an undo group whose first command changed nothing keeps its step

A group of commands that run as one undo step — a dialog's OK, the frames of a
drag — has a step only once something in it changes. When the group's first
command changed nothing, the no-op path put that step back, so the commands
that joined it had nothing to merge into: their changes were not undoable and
the undo depth did not grow. That is the Page Setup test failure CI hit, which
only shows with the font checkout CI builds with.

The session now tracks whether the group has a step (`join_anchor`): the
dispatch pushes one for a joined command when it has none, and a no-op or
rejected command that puts its own step back leaves the group without one.

A command that changes nothing also leaves the unsaved-changes flag as it was,
even when the command calls `Session::touch()` itself.

And the no-op test uses the session's `replaces_document` indicator as well as
the document identity: `review.compare` with an `original` replaces the
document in place, without `set_document`, so two identical comparisons
produce the same document and the same identity.

Tests: `a_joined_command_starts_its_group_when_the_first_changed_nothing`,
`a_repeated_comparison_is_still_an_edit`, and the dirty check in
`a_command_that_changes_nothing_is_not_an_edit`.

crates/engine/src/session.rs

@@ -323,8 +323,8 @@ struct Undo {
 
 /// What [`Session::restore`] needs to take commands back exactly: the document, the selection,
 /// the undo and redo stacks (also the oldest undo steps the undo limit pushes out meanwhile), the
-/// open typing group, a pending [`Session::join_next_undo`] and the dirty flag. See
-/// [`Session::edit_snapshot`].
+/// open typing group, a pending [`Session::join_next_undo`] (and whether its group has a step)
+/// and the dirty flag. See [`Session::edit_snapshot`].
 pub struct EditSnapshot {
     doc: Document,
     sel: Selection,
@@ -337,6 +337,7 @@ pub struct EditSnapshot {
     redo: Vec<Arc<Undo>>,
     typing_open: bool,
     join_next: bool,
+    join_anchor: bool,
     dirty: bool,
 }
 
@@ -416,6 +417,10 @@ pub struct Session {
     typing_open: bool,
     /// The next mutating command joins the previous undo step (later frames of a drag).
     join_next: bool,
+    /// Whether the undo group has a step of its own for the next joined command to merge
+    /// into. A command that changed nothing leaves the group without one, so the next
+    /// command in the group starts the step the group needs.
+    join_anchor: bool,
     /// The newest automatic capitalisation, for Word's "Automatically add words to list" (#1857).
     pub(crate) autocap: Option<crate::cmd::tools::AutoCap>,
     rev: u64,
@@ -689,6 +694,7 @@ impl Session {
             redo: Vec::new(),
             typing_open: false,
             join_next: false,
+            join_anchor: false,
             autocap: None,
             rev: 1,
             document_id: 1,
@@ -999,10 +1005,11 @@ impl Session {
         }
     }
 
-    /// Snapshot for undo before a change.
-    pub fn checkpoint(&mut self, label: &str) {
+    /// Snapshot for undo before a change. Returns whether it took a step: typing that is
+    /// already in an open step is not a new one.
+    pub fn checkpoint(&mut self, label: &str) -> bool {
         if label == "Typing" && self.typing_open {
-            return;
+            return false;
         }
         self.typing_open = label == "Typing";
         self.history.push(Arc::new(Undo {
@@ -1017,6 +1024,9 @@ impl Session {
             self.undo_evicted += 1;
         }
         self.redo.clear();
+        // The newest step is what a joined command merges into.
+        self.join_anchor = true;
+        true
     }
     /// Record `doc`/`sel` as their own undo step after the fact and close the typing group, so
     /// Undo right after an automatic change (AutoFormat, AutoCorrect) reverts only that change.
@@ -1027,6 +1037,7 @@ impl Session {
             self.undo_evicted += 1;
         }
         self.typing_open = false;
+        self.join_anchor = true;
     }
     /// Make the next mutating command part of the previous undo step instead of a new one, so a
     /// drag that runs a command every frame is a single Undo. Call it on every frame of the drag
@@ -1156,6 +1167,7 @@ impl Session {
         self.history.clear();
         self.redo.clear();
         self.typing_open = false;
+        self.join_anchor = false;
     }
 
     /// The version snapshots belong to the document they were taken from: keeping the old ones
@@ -1232,7 +1244,8 @@ impl Session {
     /// may want to take it back without a trace (cheap: the document's blocks are shared).
     ///
     /// Covered: `doc`, `sel`, the undo stack (its length, and its oldest steps when it is near
-    /// the limit), `redo`, the open typing group, a pending `join_next_undo` and `dirty`. `rev`,
+    /// the limit), `redo`, the open typing group, a pending `join_next_undo` (and whether its
+    /// group has a step to merge into) and `dirty`. `rev`,
     /// `cache` and `layout` are derived (`restore` bumps `rev`). The other fields are not
     /// covered: a caller that lets the command change them (the view, the find state, the
     /// clipboard, macros, the file path…) saves and puts them back itself.
@@ -1277,6 +1290,7 @@ impl Session {
             redo,
             typing_open,
             join_next,
+            join_anchor,
             autocap: _,
             rev: _,
             cache: _,
@@ -1330,6 +1344,7 @@ impl Session {
             redo: redo.clone(),
             typing_open: *typing_open,
             join_next: *join_next,
+            join_anchor: *join_anchor,
             dirty: *dirty,
         }
     }
@@ -1338,7 +1353,7 @@ impl Session {
     /// selection or undo/redo (undo steps the limit pushed out meanwhile come back). The layout
     /// is recomputed.
     pub fn restore(&mut self, snap: EditSnapshot) {
-        let EditSnapshot { doc, sel, history_len, evicted, head, redo, typing_open, join_next, dirty } = snap;
+        let EditSnapshot { doc, sel, history_len, evicted, head, redo, typing_open, join_next, join_anchor, dirty } = snap;
         let gone = usize::try_from(self.undo_evicted.saturating_sub(evicted)).unwrap_or(usize::MAX);
         if gone > 0 {
             let back = gone.min(head.len());
@@ -1353,6 +1368,7 @@ impl Session {
         self.redo = redo;
         self.typing_open = typing_open;
         self.join_next = join_next;
+        self.join_anchor = join_anchor;
         self.touch();
         self.dirty = dirty;
     }
@@ -1458,6 +1474,9 @@ impl Session {
         // new one, with a new undo history: it is not an edit of this document, whatever the
         // new document holds.
         let document_id_before = self.document_id;
+        // Whether the document has unsaved changes: a command that changes nothing leaves it
+        // as it was, and a failed one leaves no trace.
+        let dirty_before = spec.mutates.then_some(self.dirty);
         // These mutating commands can replace the open document and discard its saved versions.
         // Keep the versions until the command and the protected-region post-check have both
         // succeeded; rejected commands must leave the old document and its snapshots together.
@@ -1478,10 +1497,15 @@ impl Session {
         if spec.mutates && !typing {
             self.typing_open = false;
         }
+        // Whether this command pushed an undo step (a no-op command's step is dropped below,
+        // leaving its undo group without one).
+        let mut pushed = false;
         if spec.mutates {
             let label = if typing { "Typing" } else { spec.label };
-            if !join || self.history.is_empty() {
-                self.checkpoint(label);
+            // A joined command merges into its group's step; a group whose first command
+            // changed nothing has none, so the next command in it starts one (#4001).
+            if !join || self.history.is_empty() || !self.join_anchor {
+                pushed = self.checkpoint(label);
             }
         } else if !spec.id.starts_with("caret.") && !spec.id.starts_with("view.") {
             // Non-mutating commands other than caret movement keep the typing group.
@@ -1526,10 +1550,14 @@ impl Session {
                     self.doc.ensure_nonempty();
                     self.doc.prune_text_boxes();
                     // Whether the command left the document, the selection and the formatting
-                    // waiting at the caret as they were — and the same document open.
+                    // waiting at the caret as they were. A command that replaces the document —
+                    // a mail merge, a comparison — is not an edit of this one, whatever the new
+                    // document holds: `replaces_document` names the commands that may replace
+                    // it, and `document_id` catches any other that does (through
+                    // `set_document`, which starts a new history the old one must not overwrite).
                     let unchanged = |s: &Session| match (&before_doc, &pending_before) {
                         (Some((doc, sel, _, _, _, _, column)), Some(pending)) => {
-                            s.document_id == document_id_before && s.unchanged(doc, sel, column, pending)
+                            !replaces_document && s.document_id == document_id_before && s.unchanged(doc, sel, column, pending)
                         }
                         _ => false,
                     };
@@ -1550,13 +1578,21 @@ impl Session {
                     no_op = !changed && !rerouted && unchanged(self);
                     if no_op {
                         // The step the dispatch pushed is not an edit: put the stacks back as
-                        // they were, so the redo stack survives and no step is evicted.
+                        // they were, so the redo stack survives and no step is evicted. The
+                        // group it belonged to is left without a step of its own, so the next
+                        // command in it starts one.
                         if let Some((_, _, history, redo, typing_open, evicted, _)) = &before_doc {
                             self.history = history.clone();
                             self.redo = redo.clone();
                             self.typing_open = *typing_open;
                             self.undo_evicted = *evicted;
                         }
+                        if let Some(dirty) = dirty_before {
+                            self.dirty = dirty;
+                        }
+                        if pushed {
+                            self.join_anchor = false;
+                        }
                     }
                 }
                 self.clamp_selection();
@@ -1605,6 +1641,13 @@ impl Session {
                     // The steps the command pushed out are back, so they no longer count as gone.
                     self.undo_evicted = ev;
                     self.column = c;
+                    // Its step is gone with them: its undo group has none to merge into.
+                    if pushed {
+                        self.join_anchor = false;
+                    }
+                }
+                if let Some(dirty) = dirty_before {
+                    self.dirty = dirty;
                 }
                 if let Some(b) = blocks_before {
                     self.building_blocks = b;

crates/engine/src/tests.rs

@@ -301,6 +301,14 @@ fn a_command_that_changes_nothing_is_not_an_edit() {
     let depth = s.undo_depth();
     run(&mut s, "para.align", json!({"value": "center"}));
     assert_eq!(s.undo_depth(), depth, "no undo step");
+    // A command that changes nothing leaves the unsaved-changes flag as it was, even when it
+    // calls `touch` itself: a saved document is not marked edited for nothing.
+    let mut clean = Session::new(wordcraft_doc::Document::new());
+    run(&mut clean, "para.align", json!({"value": "left"}));
+    clean.dirty = false;
+    run(&mut clean, "para.align", json!({"value": "left"}));
+    assert!(!clean.dirty, "no unsaved changes");
+    assert_eq!(clean.undo_depth(), 1, "only the first align is a step");
 }
 
 /// Five hundred commands that change nothing used to fill the whole undo stack and push the
@@ -331,6 +339,35 @@ fn a_no_op_command_keeps_the_redo_stack() {
     assert_eq!(text(&s), "Hello");
 }
 
+/// Commands that run as one undo step (`join_next_undo`: a dialog's OK, the frames of a drag):
+/// a group whose first command changes nothing has no step to merge into, so the next command
+/// in it starts the group's step instead of merging into an older edit (#23 follow-up).
+#[test]
+fn a_joined_command_starts_its_group_when_the_first_changed_nothing() {
+    let mut s = s();
+    run(&mut s, "text.insert", json!({"text": "Hello"}));
+    let (depth, margins) = (s.undo_depth(), (s.doc.last_section.margin_left, s.doc.last_section.margin_top));
+    // Portrait on a portrait page changes nothing.
+    run(&mut s, "layout.orientation", json!({"value": "portrait"}));
+    assert_eq!(s.undo_depth(), depth, "the first command of the group is no edit");
+    // The next command of the group starts the step the group needs.
+    s.join_next_undo();
+    run(&mut s, "layout.margins", json!({"left": 100.0}));
+    assert_eq!(s.doc.last_section.margin_left, 100.0);
+    assert_eq!(s.undo_depth(), depth + 1, "the group's one step");
+    // A command that changes nothing in the middle of the group leaves that step alone, and
+    // the next one merges into it.
+    s.join_next_undo();
+    run(&mut s, "layout.orientation", json!({"value": "portrait"}));
+    s.join_next_undo();
+    run(&mut s, "layout.margins", json!({"top": 60.0}));
+    assert_eq!(s.undo_depth(), depth + 1, "still the group's one step");
+    // Undo takes the whole group back and no more: the typing is still there.
+    run(&mut s, "edit.undo", json!({}));
+    assert_eq!((s.doc.last_section.margin_left, s.doc.last_section.margin_top), margins, "the margins the group started from");
+    assert_eq!(text(&s), "Hello");
+}
+
 /// Bold with nothing selected arms the formatting for the next word: the document is the
 /// same, so only the caret's formatting changed — and that is an edit (#23).
 #[test]
@@ -8434,6 +8471,29 @@ fn a_version_snapshot_does_not_survive_an_untitled_comparison() {
     }
 }
 
+/// A comparison with an `original` replaces the open document in place — the handler loads the
+/// original into `s.doc` without `set_document` — so `document_id` does not move and two
+/// identical comparisons produce identical documents. A comparison is an edit of its own all
+/// the same: the `replaces_document` guard keeps its undo step (#4004 review).
+#[test]
+fn a_repeated_comparison_is_still_an_edit() {
+    let dir = std::env::temp_dir().join(format!("wordcraft-compare-twice-{}", std::process::id()));
+    std::fs::create_dir_all(&dir).unwrap();
+    let original = dir.join("original.docx");
+    let mut other = Session::new(wordcraft_doc::Document::from_text("Original document."));
+    other.run("file.save", &json!({"path": original.to_string_lossy()})).unwrap();
+
+    let mut s = Session::new(wordcraft_doc::Document::from_text("Current document."));
+    let compare = json!({"original": original.to_string_lossy(), "text": "Revised document."});
+    s.run("review.compare", &compare).unwrap();
+    let depth = s.undo_depth();
+    assert!(s.dirty);
+    s.run("review.compare", &compare).unwrap();
+    assert_eq!(s.undo_depth(), depth + 1, "the second comparison is an edit of its own");
+    assert!(s.dirty, "the comparison result is unsaved");
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 /// A rejected comparison leaves the current document and its snapshots together.
 #[test]
 fn a_rejected_comparison_keeps_the_current_versions() {