diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index 58f8658518..5a4da3dae2 100644 --- a/crates/engine/src/session.rs +++ b/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>, 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, 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; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index bfb7296774..865e2cb8bb 100644 --- a/crates/engine/src/tests.rs +++ 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() {