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 |