From 52d16685071b3bd755a4648e2368d718ef7d6bac Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 18:44:33 +0100 Subject: [PATCH] Resolve only the navigated style change Every style-definition change sits at the same collapsed anchor at the start of the document, so Accept or Reject there resolved all of them, across authors, and so did any caret put at the document start. Next and Previous Change now remember which style change they selected (style id and revision, for that document and selection), step between style changes that share the anchor, and Accept/Reject resolve that revision alone. A caret placed there any other way resolves no style change; Accept/Reject All and a whole-body selection still resolve them all. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/fmt_revisions.rs | 11 ++++ crates/engine/src/cmd/review.rs | 1 + crates/engine/src/cmd/review/changes.rs | 88 +++++++++++++++++++------ crates/engine/src/session.rs | 4 ++ crates/engine/src/session/documents.rs | 2 + crates/engine/src/session/lifecycle.rs | 1 + crates/engine/src/session/run.rs | 9 +++ crates/engine/src/session/snapshot.rs | 2 + crates/engine/src/tests.rs | 66 ++++++++++++++++++- docs/parity-checklist.md | 4 +- 10 files changed, 162 insertions(+), 26 deletions(-) diff --git a/crates/engine/src/cmd/fmt_revisions.rs b/crates/engine/src/cmd/fmt_revisions.rs index a75af593e4..6da100825c 100644 --- a/crates/engine/src/cmd/fmt_revisions.rs +++ b/crates/engine/src/cmd/fmt_revisions.rs @@ -474,6 +474,17 @@ pub fn resolve_style(st: &mut Style, accept: bool) { } } +/// Accept or reject one revision's property changes in a style definition (in a file the paragraph +/// and run properties can carry different revisions). +pub fn resolve_style_rev(st: &mut Style, rev: u32, accept: bool) { + if let Some(ch) = st.para_change.take_if(|ch| ch.rev == rev).filter(|_| !accept) { + st.para = ch.old; + } + if let Some(ch) = st.chr_change.take_if(|ch| ch.rev == rev).filter(|_| !accept) { + st.chr = ch.old; + } +} + /// Accept or reject a section's change (its headers and footers stay). pub fn resolve_section(s: &mut SectionProps, accept: bool) { let Some(ch) = s.fmt_change.take() else { return }; diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index da65f68e94..f0539f0a1c 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -13,6 +13,7 @@ mod changes; mod comments; mod proofing; +pub(crate) use changes::StyleTarget; pub use changes::{ DELETION_TEXT_CHARS, DeletionBalloon, deletion_balloons, format_changes, has_deletions, has_format_changes, move_changes, tracking_json, }; diff --git a/crates/engine/src/cmd/review/changes.rs b/crates/engine/src/cmd/review/changes.rs index 13fe4c3739..cbbd629f02 100644 --- a/crates/engine/src/cmd/review/changes.rs +++ b/crates/engine/src/cmd/review/changes.rs @@ -100,11 +100,10 @@ fn resolve_cell(s: &mut Session, story: StoryRef, path: &wordcraft_doc::Path, ac Ok(true) } -/// Accept or reject style-definition changes at their collapsed navigation anchor, or when the -/// whole body is selected. A text range that merely begins at the document start must not resolve -/// unrelated style revisions. -fn resolve_styles_sel(s: &mut Session, selected: bool, accept: bool) -> Result { - if !selected { +/// Accept or reject every style-definition change when the whole body is selected. A text range +/// that merely begins at the document start must not resolve unrelated style revisions. +fn resolve_styles_sel(s: &mut Session, whole_body: bool, accept: bool) -> Result { + if !whole_body { return Ok(false); } if !s.doc.styles.styles.iter().any(|st| st.para_change.is_some() || st.chr_change.is_some()) { @@ -116,19 +115,57 @@ fn resolve_styles_sel(s: &mut Session, selected: bool, accept: bool) -> Result Option { + s.review_style.clone().filter(|t| t.document == s.document_id() && t.sel == s.sel) +} + +/// Accept or reject the navigated style change alone: that revision in that style. Returns whether +/// it was still there. +fn resolve_style_change(s: &mut Session, t: &StyleTarget, accept: bool) -> bool { + let revs = |st: &wordcraft_doc::styles::Style| [st.para_change.as_ref().map(|c| c.rev), st.chr_change.as_ref().map(|c| c.rev)]; + if !s.doc.styles.styles.iter().any(|st| st.id == t.style && revs(st).contains(&Some(t.rev))) { + return false; + } + for st in std::sync::Arc::make_mut(&mut s.doc.styles).styles.iter_mut().filter(|st| st.id == t.style) { + fmt_revisions::resolve_style_rev(st, t.rev, accept); + } + true +} + +/// Select change `c` and remember which style change it is, if it is one. +fn select_change(s: &mut Session, c: &Change) { + s.sel = Selection { anchor: c.a.clone(), focus: c.b.clone() }; + s.review_style = c.style.clone().zip(c.rev).map(|(style, rev)| StyleTarget { document: s.document_id(), sel: s.sel.clone(), style, rev }); +} + /// A tracked change, for navigation and the Reviewing Pane: its range, kind (`insert`, -/// `delete`, `format`), revision, and for a formatting change the properties it set. +/// `delete`, `format`), revision, and for a formatting change the properties it set; for a +/// style-definition change, the style's id. struct Change { a: Pos, b: Pos, kind: &'static str, rev: Option, props: Vec, + style: Option, } impl Change { fn format(a: Pos, b: Pos, rev: u32, props: Vec) -> Change { - Change { a, b, kind: "format", rev: Some(rev), props } + Change { a, b, kind: "format", rev: Some(rev), props, style: None } } } @@ -146,7 +183,7 @@ fn mark_change(s: &Session, story: StoryRef, path: &wordcraft_doc::Path) -> Opti } else { a.clone() }; - Some(Change { a, b, kind, rev: p.mark.ins.or(p.mark.del), props: Vec::new() }) + Some(Change { a, b, kind, rev: p.mark.ins.or(p.mark.del), props: Vec::new(), style: None }) } /// The kind of an insertion / deletion change: `insert`, `delete`, or for one end of a move @@ -288,10 +325,19 @@ fn revision_span(p: &Paragraph, off: usize) -> Option> { } fn resolve_sel(s: &mut Session, accept: bool, next: bool) -> CmdResult { + // The style change Next / Previous Change selected: that one alone. A caret put at the + // document start any other way resolves no style change. + let navigated = navigated_style(s); + s.review_style = None; + if let Some(t) = navigated + && resolve_style_change(s, &t, accept) + { + return if next { select_next_unresolved(s) } else { sel_result(s) }; + } let (a, b) = s.sel.ordered(); // Capture this before text revisions are resolved: accepting a deletion or rejecting an // insertion can change the body's end position. - let style_selection = a == s.doc.start_of(StoryRef::Body) && (a == b || b == s.doc.end_of(StoryRef::Body)); + let whole_body = a != b && a == s.doc.start_of(StoryRef::Body) && b == s.doc.end_of(StoryRef::Body); // A caret right before a tracked paragraph mark (and no tracked text around it): that mark. let mut caret_mark = None; // A caret in a paragraph whose own formatting changed (and no tracked text around it). @@ -314,9 +360,6 @@ fn resolve_sel(s: &mut Session, accept: bool, next: bool) -> CmdResult { (a.clone(), a) } None => { - if resolve_styles_sel(s, style_selection, accept)? { - return sel_result(s); - } if resolve_cell(s, a.story, &a.path, accept)? { return if next { select_next_unresolved(s) } else { sel_result(s) }; } @@ -341,7 +384,7 @@ fn resolve_sel(s: &mut Session, accept: bool, next: bool) -> CmdResult { } } crate::cmd::moves::resolve(s, &moves, accept)?; - resolve_styles_sel(s, style_selection, accept)?; + resolve_styles_sel(s, whole_body, accept)?; s.sel = Selection::caret(a); s.clamp_selection(); if next { select_next_unresolved(s) } else { sel_result(s) } @@ -354,7 +397,7 @@ fn changes(s: &Session) -> Vec { let start = s.doc.start_of(StoryRef::Body); for st in &s.doc.styles.styles { for (rev, props) in fmt_revisions::style_diffs(st) { - out.push(Change::format(start.clone(), start.clone(), rev, props)); + out.push(Change { style: Some(st.id.clone()), ..Change::format(start.clone(), start.clone(), rev, props) }); } } for path in s.doc.para_paths(StoryRef::Body) { @@ -381,7 +424,7 @@ fn changes(s: &Session) -> Vec { for (r, c) in p.run_ranges() { let kind = run_kind(&s.doc, c.ins, c.del); if !kind.is_empty() { - out.push(Change { a: mk(&path, r.start), b: mk(&path, r.end), kind, rev: c.ins.or(c.del), props: Vec::new() }); + out.push(Change { a: mk(&path, r.start), b: mk(&path, r.end), kind, rev: c.ins.or(c.del), props: Vec::new(), style: None }); } let Some(ch) = &c.fmt_change else { continue }; let props = fmt_revisions::char_diff(c); @@ -533,13 +576,16 @@ pub fn has_deletions(doc: &wordcraft_doc::Document) -> bool { pub(super) fn nav_change(s: &mut Session, dir: i32) -> CmdResult { let list = changes(s); let caret = s.sel.ordered(); - let t = if dir > 0 { - list.iter().find(|c| c.a >= caret.1 && (c.a > caret.0 || c.b > caret.1)).or(list.first()) - } else { - list.iter().rev().find(|c| c.b <= caret.0 && (c.b < caret.1 || c.a < caret.0)).or(list.last()) + // From a navigated style change, the one next to it in the list (they share their anchor). + let at = navigated_style(s).and_then(|t| list.iter().position(|c| c.style.as_deref() == Some(t.style.as_str()) && c.rev == Some(t.rev))); + let t = match at { + Some(i) if dir > 0 && i + 1 < list.len() => list.get(i + 1), + Some(i) if dir < 0 && i > 0 => list.get(i - 1), + _ if dir > 0 => list.iter().find(|c| c.a >= caret.1 && (c.a > caret.0 || c.b > caret.1)).or(list.first()), + _ => list.iter().rev().find(|c| c.b <= caret.0 && (c.b < caret.1 || c.a < caret.0)).or(list.last()), }; if let Some(c) = t { - s.sel = Selection { anchor: c.a.clone(), focus: c.b.clone() }; + select_change(s, c); } sel_result(s) } @@ -551,7 +597,7 @@ fn select_next_unresolved(s: &mut Session) -> CmdResult { let list = changes(s); let caret = s.sel.ordered().0; if let Some(c) = list.iter().find(|c| c.a >= caret).or(list.first()) { - s.sel = Selection { anchor: c.a.clone(), focus: c.b.clone() }; + select_change(s, c); } sel_result(s) } diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index f1a839006f..089333894b 100644 --- a/crates/engine/src/session.rs +++ b/crates/engine/src/session.rs @@ -96,6 +96,10 @@ pub struct Session { pub new_header_parts: Vec, /// The body selection a command left for a header or footer: `insert.closeHeader` puts it back. pub hf_return: Option, + /// The style-definition change Next / Previous Change selected, for the Accept or Reject that + /// follows (`cmd::review`). It names its document and selection, so it is shared by the open + /// documents and stops counting once either changes. + pub(crate) review_style: Option, /// Undo and redo steps. Shared so `run` can snapshot both stacks cheaply (a pointer per /// step) and put them back exactly when a command fails. history: Vec>, diff --git a/crates/engine/src/session/documents.rs b/crates/engine/src/session/documents.rs index be763af1fd..efd3ed0930 100644 --- a/crates/engine/src/session/documents.rs +++ b/crates/engine/src/session/documents.rs @@ -240,6 +240,8 @@ impl Session { edit_marks: _, go_back_step: _, // Shared by every open document. + // Names the document it belongs to, so it never applies to another one. + review_style: _, user_dict_path: _, clipboard: _, clipboard_text: _, diff --git a/crates/engine/src/session/lifecycle.rs b/crates/engine/src/session/lifecycle.rs index d6a03e2490..f2dea8349b 100644 --- a/crates/engine/src/session/lifecycle.rs +++ b/crates/engine/src/session/lifecycle.rs @@ -110,6 +110,7 @@ impl Session { status: String::new(), new_header_parts, hf_return, + review_style: None, history, undo_evicted, redo, diff --git a/crates/engine/src/session/run.rs b/crates/engine/src/session/run.rs index 3339e181c5..925fc112de 100644 --- a/crates/engine/src/session/run.rs +++ b/crates/engine/src/session/run.rs @@ -49,6 +49,14 @@ impl Session { doc.same_content(&self.doc) && *sel == self.sel && *column == self.column && *pending == self.pending } + /// A navigated style change is for the Accept or Reject that follows: a command that edits + /// or moves the selection forgets it. + fn forget_review_style(&mut self, id: &str, mutates: bool) { + if (mutates && !matches!(id, "review.accept" | "review.reject")) || id.starts_with("caret.") || id.starts_with("select.") { + self.review_style = None; + } + } + /// Run a command by id with JSON params (the single entry point for the UI, CLI, MCP and /// control channel). Mutating commands are undoable; a failed command leaves the document as /// it was; a panic inside a command becomes an error. @@ -71,6 +79,7 @@ impl Session { let ime_text_join = self.ime_text_join.take(); let reg = self.registry.clone(); let Some(spec) = reg.get(id) else { return Err(CmdError::Unknown(id.to_string())) }; + self.forget_review_style(id, spec.mutates); let join = join || (equation_join && matches!(id, "equation.type" | "equation.backspace")) || (id == "text.insert" && ime_text_join.as_ref().is_some_and(|sel| sel == &self.sel)); diff --git a/crates/engine/src/session/snapshot.rs b/crates/engine/src/session/snapshot.rs index 7d9c7c056d..1ac4a60994 100644 --- a/crates/engine/src/session/snapshot.rs +++ b/crates/engine/src/session/snapshot.rs @@ -94,6 +94,8 @@ impl Session { // Headers just created to edit: closing checks they still exist and are empty. new_header_parts: _, hf_return: _, + // Which navigated style change Accept/Reject acts on: it checks the selection itself. + review_style: _, history, undo_evicted, redo, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 04d205ca00..c81523e656 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3284,9 +3284,9 @@ fn tracked_style_definition_changes_are_recorded_and_resolved() { assert!(d.contains("Chr") && d.contains("bold") && d.contains("Para") && d.contains("align"), "{d}"); assert_eq!(ch[0]["start"]["path"], json!([0]), "the change sits at the start of the document"); - // Reject the change at the start of the document (where Word shows it): the old definition - // comes back and the change is gone. - run(&mut s, "select.range", json!({"anchor": {"block": 0, "off": 0}, "focus": {"block": 0, "off": 0}})); + // Go to the change (at the start of the document, where Word shows it) and reject it: the old + // definition comes back and the change is gone. + run(&mut s, "review.nextChange", json!({})); run(&mut s, "review.reject", json!({})); let st = s.doc.styles.get("Heading1").unwrap(); assert_eq!((st.chr.bold, st.para.align), (None, None), "rejecting restores the style as it was"); @@ -3380,6 +3380,66 @@ fn a_navigated_style_change_can_be_resolved_at_its_anchor() { assert!(st.para_change.is_none() && st.chr_change.is_none()); } +/// #2697 review: every style change sits at the same collapsed anchor, so Accept or Reject of +/// the navigated one must resolve that revision only, not another author's style change. A caret +/// put at the document start by other means resolves none of them; Accept/Reject All still +/// resolve them all. +#[test] +fn a_navigated_style_change_is_resolved_alone() { + for (command, accept) in [("review.accept", true), ("review.reject", false)] { + let mut s = s(); + run(&mut s, "document.setText", json!({"text": "body"})); + run(&mut s, "review.trackChanges", json!({"value": true})); + s.author = "Ana".into(); + run(&mut s, "styles.modify", json!({"style": "Heading1", "chr": {"bold": true}})); + s.author = "Bo".into(); + run(&mut s, "styles.modify", json!({"style": "Heading2", "chr": {"italic": true}})); + let changed = |s: &Session, id: &str| s.doc.styles.get(id).unwrap().chr_change.is_some(); + let authors = + |v: &serde_json::Value| v.as_array().unwrap().iter().map(|c| c["author"].as_str().unwrap_or_default().to_string()).collect::>(); + let start = s.doc.start_of(StoryRef::Body); + + // An ordinary caret at the document start is not a navigated style change, even right + // after navigating to one and moving away. + s.sel = crate::Selection::caret(s.doc.end_of(StoryRef::Body)); + run(&mut s, "review.nextChange", json!({})); + assert_eq!(s.sel.ordered(), (start.clone(), start.clone())); + run(&mut s, "caret.set", json!({"pos": {"block": 0, "off": 2}})); + run(&mut s, "select.range", json!({"anchor": {"block": 0, "off": 0}, "focus": {"block": 0, "off": 0}})); + run(&mut s, command, json!({})); + assert!(changed(&s, "Heading1") && changed(&s, "Heading2"), "{command} at a plain caret resolved a style change"); + + // Next Change reaches each style change in turn, though both share the anchor. + s.sel = crate::Selection::caret(s.doc.end_of(StoryRef::Body)); + run(&mut s, "review.nextChange", json!({})); + assert_eq!(s.sel.ordered(), (start.clone(), start.clone())); + run(&mut s, "review.nextChange", json!({})); + assert_eq!(s.sel.ordered(), (start.clone(), start.clone())); + run(&mut s, command, json!({})); + assert!(changed(&s, "Heading1"), "{command} on Bo's style change resolved Ana's too"); + assert!(!changed(&s, "Heading2"), "{command} left the navigated change"); + assert_eq!(s.doc.styles.get("Heading2").unwrap().chr.italic, accept.then_some(true)); + assert_eq!(authors(&run(&mut s, "review.changes", json!({}))), ["Ana"]); + + // Previous Change from the end reaches the one left, and resolves it. + s.sel = crate::Selection::caret(s.doc.end_of(StoryRef::Body)); + run(&mut s, "review.previousChange", json!({})); + run(&mut s, command, json!({})); + assert!(!changed(&s, "Heading1"), "{command} left the navigated change"); + assert_eq!(s.doc.styles.get("Heading1").unwrap().chr.bold, accept.then_some(true)); + + // Accept/Reject All still resolve every style change. + s.author = "Ana".into(); + run(&mut s, "styles.modify", json!({"style": "Heading1", "chr": {"italic": true}})); + s.author = "Bo".into(); + run(&mut s, "styles.modify", json!({"style": "Heading2", "chr": {"bold": true}})); + assert_eq!(authors(&run(&mut s, "review.changes", json!({}))), ["Ana", "Bo"]); + run(&mut s, if accept { "review.acceptAll" } else { "review.rejectAll" }, json!({})); + assert!(!changed(&s, "Heading1") && !changed(&s, "Heading2")); + assert_eq!(run(&mut s, "review.changes", json!({})), json!([])); + } +} + /// #2697 review: identity, relationships, table formatting and gallery metadata do not have a /// tracked-change carrier in OOXML's CT_Style, so changing only those fields creates no false /// paragraph/run formatting revision. diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 998d62e09c..6bb881e440 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -2,14 +2,14 @@ Generated by `cargo xtask parity` (`wordcraft-cli parity --markdown`): the word-processor feature catalog (`crates/engine/src/catalog.rs`) compared with the live command registry. -**508 of 520 catalog features have commands (97.7%).** +**509 of 521 catalog features have commands (97.7%).** | Tab | Live | Total | |---|---|---| | File | 23 | 23 | | Quick Access | 5 | 5 | | Title Bar | 1 | 1 | -| Home | 104 | 105 | +| Home | 105 | 106 | | Insert | 47 | 53 | | Draw | 10 | 11 | | Design | 10 | 10 |