From bbb97175d98e7beaac40abd595f668a576e90ee7 Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 17:09:15 +0200 Subject: [PATCH 1/8] Make split_run take the direct body index of find_content_index Document::split_run resolved its first argument with Document::paragraph, which counts paragraphs, skips tables and enters block content controls. RunPosition, find_content_index, add_comment and insert_paragraph use the direct body child index instead, which is also what HLD 10 and F-X109 state for split_run. With a table or a block content control before the target, splitting a run then anchoring a comment at the same index split one paragraph and commented another, or failed on a run length. split_run now reads the paragraph at that direct slot and writes it back to the same slot, so one resolution serves both steps. An index that names a table, a block content control or preserved XML returns an error naming that kind instead of splitting another paragraph. The Python method also accepts a Paragraph handle, which reaches paragraphs inside block content controls, and it measures the revision bump on the paragraph it split. A handle to a direct body paragraph takes the same native path as its index. A table cell paragraph handle is refused, because its index counts the paragraphs inside cell content controls and Cell::paragraph_mut does not. The rustdoc of RunPosition and Document::paragraph and the HLD 03 and 10 statements now name the index each one takes. GitHub issue #163. --- crates/rdocx-py/python/rdocx/_rdocx.pyi | 2 +- crates/rdocx-py/src/document.rs | 87 +++++++++++++--- crates/rdocx-py/tests/test_core.py | 92 +++++++++++++++++ crates/rdocx-py/tests/typing_smoke.py | 1 + crates/rdocx/src/comments.rs | 3 +- crates/rdocx/src/document.rs | 50 +++++++-- crates/rdocx/tests/regression_test.rs | 131 ++++++++++++++++++++++++ docs/hld/03-architecture.md | 11 +- docs/hld/10-bindings-spec.md | 11 +- 9 files changed, 354 insertions(+), 34 deletions(-) diff --git a/crates/rdocx-py/python/rdocx/_rdocx.pyi b/crates/rdocx-py/python/rdocx/_rdocx.pyi index f5e4ef18..4b3cfcfb 100644 --- a/crates/rdocx-py/python/rdocx/_rdocx.pyi +++ b/crates/rdocx-py/python/rdocx/_rdocx.pyi @@ -428,7 +428,7 @@ class Document: self, story: Story, relationship_id: str, data: bytes ) -> None: ... def split_run( - self, body_index: int, run_index: int, character_offset: int + self, body_index: int | Paragraph, run_index: int, character_offset: int ) -> int: ... def to_pdf(self) -> bytes: ... def render_page_to_png(self, page_index: int, dpi: float = 150.0) -> bytes | None: ... diff --git a/crates/rdocx-py/src/document.rs b/crates/rdocx-py/src/document.rs index b81167c6..5d343dca 100644 --- a/crates/rdocx-py/src/document.rs +++ b/crates/rdocx-py/src/document.rs @@ -7,7 +7,7 @@ use pyo3::prelude::*; use pyo3::types::{PyAny, PyBytes, PyList, PyTuple}; use smallvec::smallvec; -use crate::paragraph::{PyParagraph, PyParagraphCollection}; +use crate::paragraph::{ParagraphLocation, PyParagraph, PyParagraphCollection}; use crate::rdocx_to_pyerr; use crate::table::{PyTable, PyTableCollection}; @@ -979,6 +979,32 @@ impl PyDocument { ))) } + /// Split a run of the direct body paragraph at `body_index`. The revision + /// advances only when a continuation run is created. + fn split_body_run( + &mut self, + py: Python<'_>, + body_index: usize, + run_index: usize, + character_offset: usize, + ) -> PyResult { + let paragraph_index = self.inner.paragraph_index_of_content(body_index); + let run_count = |document: &rdocx::Document| { + paragraph_index + .and_then(|index| document.paragraph(index)) + .map(|paragraph| paragraph.run_count()) + }; + let before = run_count(&self.inner); + let boundary = self + .inner + .split_run(body_index, run_index, character_offset) + .map_err(|error| rdocx_to_pyerr(py, error))?; + if run_count(&self.inner) != before { + self.revisions.bump(); + } + Ok(boundary) + } + /// Run a native mutation that reports how many things it changed. /// /// The GIL is released while it runs, and live handles are staled only @@ -1193,26 +1219,57 @@ impl PyDocument { } fn split_run( - &mut self, + slf: Py, py: Python<'_>, - body_index: usize, + body_index: &Bound<'_, PyAny>, run_index: usize, character_offset: usize, ) -> PyResult { - let before = self - .inner - .paragraph(body_index) - .map(|paragraph| paragraph.run_count()); - let boundary = self + let Ok(paragraph) = body_index.cast::() else { + // An integer is the direct body index, as in `RunPosition`. + let body_index = body_index.extract::().map_err(|error| { + if error.is_instance_of::(py) { + PyTypeError::new_err("body_index must be an int or a Paragraph handle") + } else { + error + } + })?; + return slf + .borrow_mut(py) + .split_body_run(py, body_index, run_index, character_offset); + }; + let paragraph = paragraph.borrow(); + if !paragraph.belongs_to(py, &slf) { + return Err(PyValueError::new_err( + "paragraph handle belongs to a different document", + )); + } + // A cell handle counts paragraphs inside cell content controls, which + // `Cell::paragraph_mut` does not, so it could name another paragraph. + let ParagraphLocation::Body(paragraph_index) = paragraph.validate(py)? else { + return Err(PyValueError::new_err( + "split_run does not accept a table cell paragraph handle", + )); + }; + let mut document = slf.borrow_mut(py); + if let Some(body_index) = document.inner.content_index_of_paragraph(paragraph_index) { + return document.split_body_run(py, body_index, run_index, character_offset); + } + // A paragraph inside a block content control has no direct body index. + let run_count = |document: &rdocx::Document| { + document + .paragraph(paragraph_index) + .map(|paragraph| paragraph.run_count()) + }; + let before = run_count(&document.inner); + let boundary = document .inner - .split_run(body_index, run_index, character_offset) + .paragraph_mut(paragraph_index) + .ok_or_else(|| PyIndexError::new_err("paragraph index out of range"))? + .split_run(run_index, character_offset) .map_err(|error| rdocx_to_pyerr(py, error))?; - let after = self - .inner - .paragraph(body_index) - .map(|paragraph| paragraph.run_count()); - if before != after { - self.revisions.bump(); + if run_count(&document.inner) != before { + document.revisions.bump(); } Ok(boundary) } diff --git a/crates/rdocx-py/tests/test_core.py b/crates/rdocx-py/tests/test_core.py index 0f8ce927..19594290 100644 --- a/crates/rdocx-py/tests/test_core.py +++ b/crates/rdocx-py/tests/test_core.py @@ -1291,6 +1291,98 @@ def test_split_run_rejects_bad_coordinates_without_changing_the_document(): assert document.to_bytes() == before +def test_split_run_takes_the_direct_body_index_after_a_table(): + import rdocx + + # GitHub issue #163: the index find_content_index returns must address + # the same paragraph in split_run. + document = rdocx.Document() + document.add_paragraph("Alpha paragraph before the table.") + document.add_table(1, 1).cell(0, 0).text = "cell" + document.add_paragraph("Beta paragraph after the table.") + document.add_paragraph("Gamma paragraph at the end.") + target = next(p for p in document.paragraphs if p.text.startswith("Beta")) + body_index = document.find_content_index(target) + assert body_index == 2 + + assert document.split_run(body_index, 0, 4) == 1 + assert [[run.text for run in p.runs] for p in document.paragraphs] == [ + ["Alpha paragraph before the table."], + ["Beta", " paragraph after the table."], + ["Gamma paragraph at the end."], + ] + + before = document.to_bytes() + with pytest.raises(rdocx.RdocxError, match="is a table, not a paragraph"): + document.split_run(1, 0, 1) + with pytest.raises(TypeError, match="must be an int or a Paragraph handle"): + document.split_run("Beta", 0, 1) + with pytest.raises(OverflowError): + document.split_run(-1, 0, 1) + assert document.to_bytes() == before + + +def test_split_run_accepts_a_paragraph_handle_in_a_control_or_the_body(): + import rdocx + + document = _replace_document_body( + rdocx.Document(), + "Alpha." + "" + "Control one." + "Control two." + "" + '' + "" + "" + "In control." + "" + "Cell text." + "" + "" + "Beta.", + ) + control_two = document.paragraphs[2] + assert control_two.text == "Control two." + run = control_two.runs[0] + before_no_op = document.to_bytes() + assert document.split_run(control_two, 0, 0) == 0 + assert document.to_bytes() == before_no_op + assert run.text == "Control two." + + assert document.split_run(control_two, 0, 7) == 1 + with pytest.raises(rdocx.StaleElementError): + run.text + with pytest.raises(rdocx.StaleElementError): + document.split_run(control_two, 0, 1) + assert [r.text for r in document.paragraphs[2].runs] == ["Control", " two."] + + # A cell handle is refused, because its index counts the paragraph + # inside the cell's content control and the cell writer does not. + cell = document.tables[0].rows[0].cells[0] + assert [p.text for p in cell.paragraphs] == ["In control.", "Cell text."] + before_cell = document.to_bytes() + for cell_paragraph in cell.paragraphs: + with pytest.raises(ValueError, match="table cell paragraph handle"): + document.split_run(cell_paragraph, 0, 2) + assert document.to_bytes() == before_cell + + beta = document.paragraphs[3] + assert document.find_content_index(beta) == 3 + beta_run = beta.runs[0] + assert document.split_run(beta, 0, 5) == 1 + assert beta_run.text == "Beta." + assert document.split_run(beta, 0, 4) == 1 + assert [r.text for r in document.paragraphs[3].runs] == ["Beta", "."] + + other = rdocx.Document() + other.add_paragraph("elsewhere") + before = document.to_bytes() + with pytest.raises(ValueError, match="different document"): + document.split_run(other.paragraphs[0], 0, 1) + assert document.to_bytes() == before + + def test_python_round_three_authoring_and_inspection_is_typed_and_lossless(): import rdocx diff --git a/crates/rdocx-py/tests/typing_smoke.py b/crates/rdocx-py/tests/typing_smoke.py index 51ebfa05..ae8f1821 100644 --- a/crates/rdocx-py/tests/typing_smoke.py +++ b/crates/rdocx-py/tests/typing_smoke.py @@ -63,6 +63,7 @@ def exercise_rdocx_types(path: Path) -> None: paragraph_format: ParagraphFormat = paragraph.paragraph_format paragraph_format.keep_together = None split_boundary: int = document.split_run(0, 0, 1) + handle_boundary: int = document.split_run(paragraph, 0, 1) paragraphs: ParagraphCollection = document.paragraphs first: Paragraph = paragraphs[0] sliced: list[Paragraph] = paragraphs[:] diff --git a/crates/rdocx/src/comments.rs b/crates/rdocx/src/comments.rs index 1e85cd82..609cb492 100644 --- a/crates/rdocx/src/comments.rs +++ b/crates/rdocx/src/comments.rs @@ -27,7 +27,8 @@ const DEFAULT_COMMENTS_EXTENDED_PART: &str = "/word/commentsExtended.xml"; /// A stable insertion point between runs in a body paragraph. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct RunPosition { - /// Index in the document body's paragraph and table sequence. + /// Direct body child index, where a table or a block content control + /// counts as one child. `Document::find_content_index` returns it. pub body_index: usize, /// Run insertion index in the selected paragraph. pub run_index: usize, diff --git a/crates/rdocx/src/document.rs b/crates/rdocx/src/document.rs index 5c4b2b46..a09eacc7 100644 --- a/crates/rdocx/src/document.rs +++ b/crates/rdocx/src/document.rs @@ -14529,7 +14529,9 @@ impl Document { .collect() } - /// Get an immutable reference to a paragraph by index (among paragraphs only). + /// Get an immutable reference to a paragraph by its index in + /// [`Self::paragraphs`]. That index skips body tables and enters block + /// content controls, so it is not the direct body index. pub fn paragraph(&self, index: usize) -> Option> { self.document .body @@ -14661,7 +14663,8 @@ impl Document { result } - /// Get a mutable reference to a paragraph by index (among paragraphs only). + /// Get a mutable reference to a paragraph by the same index as + /// [`Self::paragraph`]. pub fn paragraph_mut(&mut self, index: usize) -> Option> { self.invalidate_layout(); let mut remaining = index; @@ -14672,6 +14675,14 @@ impl Document { /// Split a direct body paragraph run at a Unicode scalar offset of its /// literal text and return the resulting run boundary. /// + /// `body_index` is the direct body child index that `RunPosition`, + /// [`Self::find_content_index`], [`Self::add_comment`] and + /// [`Self::insert_paragraph`] use, where a table or a block content + /// control counts as one child. An index that names a table, a block + /// content control or preserved XML is an error that names its kind. A + /// paragraph inside a block content control is split through + /// `paragraph_mut(i).split_run`. + /// /// Zero returns the boundary before the selected run and the literal text /// length returns the boundary after it without changing the document. /// Interior offsets create a continuation with cloned run properties while @@ -14683,23 +14694,40 @@ impl Document { run_index: usize, character_offset: usize, ) -> Result { - let original = self.paragraph(body_index).ok_or_else(|| { - Error::Other(format!("body paragraph index {body_index} is out of range")) - })?; - let mut candidate = original.inner.clone(); + let not_a_paragraph = |kind: &str| { + Error::Other(format!( + "body index {body_index} is {kind}, not a paragraph" + )) + }; + let original = match self.document.body.content.get(body_index) { + Some(BodyContent::Paragraph(paragraph)) => paragraph, + Some(BodyContent::Table(_)) => return Err(not_a_paragraph("a table")), + Some(BodyContent::ContentControl(_)) => { + return Err(not_a_paragraph("a block content control")); + } + Some(BodyContent::RawXml(_)) => return Err(not_a_paragraph("preserved XML")), + None => { + return Err(Error::Other(format!( + "body index {body_index} is out of range" + ))); + } + }; + let mut candidate = original.clone(); let boundary = Paragraph { inner: &mut candidate, } .split_run(run_index, character_offset)?; - if candidate == *original.inner { + if candidate == *original { return Ok(boundary); } - self.invalidate_layout(); - let mut remaining = body_index; - let paragraph = nth_paragraph_in_body(&mut self.document.body.content, &mut remaining) - .ok_or_else(|| Error::Other("body paragraph disappeared during split".to_owned()))?; + let Some(BodyContent::Paragraph(paragraph)) = + self.document.body.content.get_mut(body_index) + else { + unreachable!("body index {body_index} was checked to be a paragraph"); + }; *paragraph = candidate; + self.invalidate_layout(); Ok(boundary) } diff --git a/crates/rdocx/tests/regression_test.rs b/crates/rdocx/tests/regression_test.rs index d143b3c7..cc1cd2e1 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -1634,6 +1634,137 @@ fn checked_table_cell_comment_range_is_atomic_and_reopens() { assert_eq!(reopened.comments()[0].text(), "review"); } +/// GitHub issue #163: the index `find_content_index` returns is a direct body +/// child index, and every body API that takes an index must address the same +/// paragraph with it, whatever tables or block content controls precede it. +mod direct_body_index_coordinates { + use rdocx::{Document, RunPosition, RunRange}; + + const TABLE: &str = r#"c00c01c10c11"#; + const CONTROL: &str = r#"Control one.Control two."#; + + fn paragraph(text: &str) -> String { + format!("{text}") + } + + fn document_from_body(body: &str) -> Document { + super::document_with_content_controls(&super::wrap_word_body(body)) + } + + fn run_texts(document: &Document) -> Vec> { + document + .paragraphs() + .iter() + .map(|paragraph| paragraph.runs().map(|run| run.text()).collect()) + .collect() + } + + fn one_run_range(body_index: usize) -> RunRange { + RunRange { + start: RunPosition { + body_index, + run_index: 0, + }, + end: RunPosition { + body_index, + run_index: 1, + }, + } + } + + /// Visible text between the range markers of comment `id` in the saved + /// main part. The tests keep each range inside one paragraph. + fn commented_text(document: &mut Document, id: i32) -> String { + let xml = super::document_xml(document); + let start_marker = format!(r#""#); + let end_marker = format!(r#""#); + let start = xml.find(&start_marker).expect("comment start marker") + start_marker.len(); + let end = start + xml[start..].find(&end_marker).expect("comment end marker"); + super::f_x093_visible_text(&xml[start..end]) + } + + #[test] + fn split_run_takes_the_direct_body_index_after_a_table() { + let mut document = Document::new(); + document.add_paragraph("Alpha paragraph before the table."); + document + .add_table(1, 1) + .row(0) + .unwrap() + .cell(0) + .unwrap() + .set_text("cell"); + document.add_paragraph("Beta paragraph after the table."); + document.add_paragraph("Gamma paragraph at the end."); + let beta = document.find_content_index("Beta").unwrap(); + assert_eq!(beta, 2); + + assert_eq!(document.split_run(beta, 0, 4).unwrap(), 1); + assert_eq!( + run_texts(&document), + [ + vec!["Alpha paragraph before the table."], + vec!["Beta", " paragraph after the table."], + vec!["Gamma paragraph at the end."], + ] + ); + + let id = document + .add_comment(one_run_range(beta), "Ada", None, "Which word?") + .unwrap(); + assert_eq!(commented_text(&mut document, id), "Beta"); + + document.insert_paragraph(beta, "Inserted."); + let texts: Vec = document.paragraphs().iter().map(|p| p.text()).collect(); + assert_eq!( + texts[1..3], + ["Inserted.", "Beta paragraph after the table."] + ); + } + + #[test] + fn split_run_takes_the_direct_body_index_after_a_block_content_control() { + let mut document = document_from_body(&format!( + "{}{CONTROL}{}{}", + paragraph("Alpha."), + paragraph("Beta paragraph."), + paragraph("Gamma.") + )); + let beta = document.find_content_index("Beta").unwrap(); + assert_eq!(beta, 2); + let mut expected = run_texts(&document); + expected[3] = vec!["Beta".to_owned(), " paragraph.".to_owned()]; + + assert_eq!(document.split_run(beta, 0, 4).unwrap(), 1); + assert_eq!(run_texts(&document), expected); + let id = document + .add_comment(one_run_range(beta), "Ada", None, "Which word?") + .unwrap(); + assert_eq!(commented_text(&mut document, id), "Beta"); + } + + #[test] + fn split_run_names_the_body_child_that_is_not_a_paragraph() { + let mut document = document_from_body(&format!( + r#"{}{TABLE}{CONTROL}{}{}"#, + paragraph("Alpha."), + paragraph("Custom."), + paragraph("Beta.") + )); + let before = document.to_bytes().unwrap(); + for (body_index, kind) in [(1, "table"), (2, "content control"), (3, "preserved XML")] { + let error = document.split_run(body_index, 0, 1).unwrap_err(); + assert!( + error.to_string().contains(kind), + "body index {body_index}: {error}" + ); + } + let error = document.split_run(5, 0, 1).unwrap_err(); + assert!(error.to_string().contains("out of range"), "{error}"); + assert_eq!(document.to_bytes().unwrap(), before); + } +} + fn f_x090_cross_part_drawing_package() -> Vec { let mut document = f255_story_document(); document.add_picture(b"body image", "body.png", Length::pt(1.0), Length::pt(1.0)); diff --git a/docs/hld/03-architecture.md b/docs/hld/03-architecture.md index 97ce19c6..341f23cb 100644 --- a/docs/hld/03-architecture.md +++ b/docs/hld/03-architecture.md @@ -1484,13 +1484,18 @@ location only after the image part, relationship, drawing identity, and story content all validate together. `Document::split_run` creates an exact accepted-view run boundary without -changing `RunPosition`. It clones the selected paragraph, resolves the selected +changing `RunPosition`. Its paragraph argument is the same direct body child +index as `RunPosition` and `find_content_index`, and an index that names a +table, a block content control, or preserved XML fails with an error naming +that kind. It clones the selected paragraph, resolves the selected recursive source path, counts Unicode scalar values only in literal text, partitions ordered zero-width children at their source boundary, repairs hyperlink and marker coordinates, and publishes the clone only on success. Zero and end offsets select existing boundaries and leave typed state, layout, -and binding revisions unchanged. A structural edit makes an earlier path-backed -Python run handle stale. +and binding revisions unchanged. A Python `Paragraph` handle to a paragraph +inside a block content control splits through `Document::paragraph_mut`, which +clears the cached layout even for those offsets. A structural edit makes an +earlier path-backed Python run handle stale. Word bookmark mutation input reuses the same top-level `RunPosition` and half-open `RunRange` boundary as comments. `Document::bookmarks` returns diff --git a/docs/hld/10-bindings-spec.md b/docs/hld/10-bindings-spec.md index a6a75eb7..68022d07 100644 --- a/docs/hld/10-bindings-spec.md +++ b/docs/hld/10-bindings-spec.md @@ -1006,12 +1006,17 @@ although third-party editors may renumber them. `RunPosition` and `RunRange` define top-level paragraph run boundaries with an inclusive start and exclusive end. `Document::split_run` splits one direct-body run at a Unicode scalar offset of its literal text, so -such a boundary can fall inside what was one run. The second part keeps the run +such a boundary can fall inside what was one run. Its first argument is the +same direct body child index as `RunPosition` and `find_content_index`. An +index that names a table, a block content control, or preserved XML is an +error naming that kind. The second part keeps the run properties and the enclosing hyperlink. Tabs, breaks, fields, drawings, references, and preserved raw children have zero width. Zero and the literal text length return the existing boundary without mutation. Interior success -returns the new continuation index. Python exposes the same method and advances -the binding revision only when a continuation is created. `CommentRef` exposes +returns the new continuation index. Python exposes the same method and also +accepts a `Paragraph` handle in place of the index, which reaches paragraphs +inside block content controls. A table cell paragraph handle is refused. The +binding revision advances only when a continuation is created. `CommentRef` exposes comment metadata, text, parent identity, and resolved state without permitting part-local mutation. `rdocx-cli comment` lists, adds, replies to, resolves, and removes comments. Add ranges use explicit zero-based, half-open body paragraph From 0547a314f9393e6295bb9f1a8fd77ad58051e74e Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 17:10:49 +0200 Subject: [PATCH 2/8] Report bookmark ranges in the direct body index as well Document::add_bookmark takes a RunRange whose body index is the direct body child index, but Document::bookmarks reports the paragraph ordinal counted recursively through table cells and block content controls. A bookmark added after a table with more than one cell, or after a block content control, read back with a different body index than the one it was added with, so that index could not be passed back to add_comment, split_run or add_bookmark. The recursive ordinal stays in range() because REF numbering in field.rs reads it as its paragraph key. BookmarkRef gains an additive direct_range() that carries the direct body child index, recorded while the paragraphs are collected, and is None when either marker sits in a table cell or a block content control. Run indexes are unchanged and stay the accepted-view boundaries of range(). They equal the direct run indexes add_bookmark takes only in a paragraph without inline content controls or tracked insertions, which GitHub issue #172 covers. GitHub issue #163. --- crates/rdocx/src/comments.rs | 51 +++++++++++++++++++++++++-- crates/rdocx/tests/regression_test.rs | 46 ++++++++++++++++++++++++ docs/hld/03-architecture.md | 4 ++- docs/hld/10-bindings-spec.md | 8 ++++- 4 files changed, 104 insertions(+), 5 deletions(-) diff --git a/crates/rdocx/src/comments.rs b/crates/rdocx/src/comments.rs index 609cb492..c067fb8f 100644 --- a/crates/rdocx/src/comments.rs +++ b/crates/rdocx/src/comments.rs @@ -61,6 +61,7 @@ pub struct BookmarkRef { id: Option, name: Option, range: Option, + direct_range: Option, text: String, issue: Option, } @@ -75,10 +76,28 @@ impl BookmarkRef { } /// Return the accepted-view half-open range reported by `Document::bookmarks`. + /// + /// Its body index is the paragraph ordinal counted recursively through + /// tables and block content controls, which field numbering reads. + /// [`Self::direct_range`] reports the direct body child index instead. pub fn range(&self) -> Option { self.range } + /// Return the same range with the direct body child index that + /// `RunPosition`, `Document::add_bookmark` and + /// `Document::find_content_index` use. + /// + /// It is `None` when either marker sits in a table cell or a block content + /// control, which have no direct body index. Run indexes are the same + /// accepted-view boundaries as [`Self::range`]. They equal the + /// `RunPosition` run index that `Document::add_bookmark` and + /// `Document::add_comment` take only in a paragraph without inline + /// content controls or tracked insertions. + pub fn direct_range(&self) -> Option { + self.direct_range + } + pub fn text(&self) -> &str { &self.text } @@ -257,10 +276,13 @@ impl Document { /// /// Reported body indexes count typed paragraphs recursively through tables and /// block content controls. Reported run indexes use accepted-view run boundaries. + /// `BookmarkRef::direct_range` also reports the direct body child index when + /// both markers sit in direct body paragraphs. pub fn bookmarks(&self) -> Vec { #[derive(Clone)] struct Marker { position: RunPosition, + direct_position: Option, start: bool, id: Option, name: Option, @@ -268,14 +290,28 @@ impl Document { let mut markers = Vec::new(); let mut paragraphs = Vec::new(); - collect_main_story_paragraphs(&self.document.body.content, &mut paragraphs); - for (body_index, paragraph) in paragraphs.into_iter().enumerate() { + for (direct_index, item) in self.document.body.content.iter().enumerate() { + let mut item_paragraphs = Vec::new(); + collect_main_story_paragraphs(std::slice::from_ref(item), &mut item_paragraphs); + let direct_index = matches!(item, BodyContent::Paragraph(_)).then_some(direct_index); + paragraphs.extend( + item_paragraphs + .into_iter() + .map(|paragraph| (paragraph, direct_index)), + ); + } + for (body_index, (paragraph, direct_index)) in paragraphs.into_iter().enumerate() { for marker in ¶graph.bookmark_markers { + let run_index = marker.projected_run_index(); markers.push(Marker { position: RunPosition { body_index, - run_index: marker.projected_run_index(), + run_index, }, + direct_position: direct_index.map(|body_index| RunPosition { + body_index, + run_index, + }), start: marker.is_start(), id: marker.id(), name: marker.name().map(str::to_owned), @@ -295,6 +331,7 @@ impl Document { id: None, name: marker.name.clone(), range: None, + direct_range: None, text: String::new(), issue: Some("bookmark marker has a malformed or missing id".to_owned()), }, @@ -344,6 +381,12 @@ impl Document { (Some(candidate), None) } }; + let direct_range = range.and_then(|_| { + Some(RunRange { + start: markers[starts[0]].direct_position?, + end: markers[ends[0]].direct_position?, + }) + }); let text = range .map(|_| { bookmark_range_text( @@ -361,6 +404,7 @@ impl Document { id: Some(id), name, range, + direct_range, text, issue, }, @@ -386,6 +430,7 @@ impl Document { bookmark.name.as_deref().unwrap_or("") )); bookmark.range = None; + bookmark.direct_range = None; bookmark.text.clear(); } } diff --git a/crates/rdocx/tests/regression_test.rs b/crates/rdocx/tests/regression_test.rs index cc1cd2e1..027a9724 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -1763,6 +1763,52 @@ mod direct_body_index_coordinates { assert!(error.to_string().contains("out of range"), "{error}"); assert_eq!(document.to_bytes().unwrap(), before); } + + #[test] + fn bookmark_direct_range_reports_the_index_add_bookmark_took() { + let mut document = document_from_body(&format!( + "{}{TABLE}{CONTROL}{}", + paragraph("Alpha."), + paragraph("Beta paragraph.") + )); + let beta = document.find_content_index("Beta").unwrap(); + assert_eq!(beta, 3); + document.add_bookmark("beta", one_run_range(beta)).unwrap(); + + let bookmark = document.bookmarks().remove(0); + assert_eq!(bookmark.text(), "Beta paragraph."); + assert_eq!(bookmark.direct_range(), Some(one_run_range(beta))); + // range() keeps the recursive paragraph ordinal that REF numbering + // reads: Alpha, the four cells and the two control paragraphs. + assert_eq!(bookmark.range().unwrap().start.body_index, 7); + } + + #[test] + fn bookmark_direct_range_is_none_when_a_marker_is_nested() { + let marked = |id: u32, name: &str, text: &str| { + format!( + r#"{text}"# + ) + }; + let document = document_from_body(&format!( + r#"Alpha.{}{}Control end.{}"#, + marked(1, "cell", "In a cell."), + marked(2, "control", "In a control."), + marked(4, "direct", "Direct.") + )); + let bookmarks = document.bookmarks(); + let named = |name: &str| { + bookmarks + .iter() + .find(|bookmark| bookmark.name() == Some(name)) + .unwrap() + }; + for name in ["cell", "control", "mixed"] { + assert!(named(name).range().is_some(), "{name}: {:?}", named(name)); + assert_eq!(named(name).direct_range(), None, "{name}"); + } + assert_eq!(named("direct").direct_range(), Some(one_run_range(3))); + } } fn f_x090_cross_part_drawing_package() -> Vec { diff --git a/docs/hld/03-architecture.md b/docs/hld/03-architecture.md index 341f23cb..fb5836d5 100644 --- a/docs/hld/03-architecture.md +++ b/docs/hld/03-architecture.md @@ -1502,7 +1502,9 @@ half-open `RunRange` boundary as comments. `Document::bookmarks` returns immutable correlated summaries in typed main-story paragraph order through tables and block content controls. A reported body index is that recursive paragraph ordinal, and its run index is the accepted-view boundary used to -extract bookmark text. Marker encounter order resolves direction when start +extract bookmark text. `BookmarkRef::direct_range` reports the same range with +the direct body child index of `RunPosition` when both markers sit in direct +body paragraphs, and `None` otherwise. Marker encounter order resolves direction when start and end share one accepted boundary, so end before start remains reversed and start before end is a valid empty range. Isolated projection refresh after a run, comment, or bookmark edit carries the original Word namespace aliases. diff --git a/docs/hld/10-bindings-spec.md b/docs/hld/10-bindings-spec.md index 68022d07..c03b3400 100644 --- a/docs/hld/10-bindings-spec.md +++ b/docs/hld/10-bindings-spec.md @@ -1034,7 +1034,13 @@ preserve a document already redacted through the native facade. Native Word callers use `Document::bookmarks` for immutable `BookmarkRef` summaries and `Document::add_bookmark` for atomic insertion over the existing top-level half-open `RunRange`. A summary exposes an optional id, name, range, -current text, and marker issue. Insertion validates the Word name and both +direct range, current text, and marker issue. The range counts paragraphs +recursively through tables and block content controls. The direct range uses +the `RunPosition` body index that `add_bookmark` takes and is present only when +both markers sit in direct body paragraphs. Its run indexes stay the +accepted-view boundaries of the range, which equal the run indexes +`add_bookmark` takes only in a paragraph without inline content controls or +tracked insertions. Insertion validates the Word name and both boundaries, rejects duplicate or producer-reserved names, and returns the allocated nonnegative id. The shared recursive `Field` model retains the complete `REF` and `PAGEREF` instruction, target argument, cached display, From 58c0c687b28c3df4c86e466c96846044493ba71d Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 17:12:40 +0200 Subject: [PATCH 3/8] Anchor story comments on their paragraph after a block control Document::add_story_comment resolved a StoryRunRange paragraph by counting the paragraph items before it, which are the direct paragraphs of the owner only, and then took that many paragraphs through a resolver that also enters block content controls. When a block content control with paragraphs came before the target, in the body or in a table cell, the comment was anchored on a paragraph inside the control, or refused when that paragraph had too few runs. A body paragraph item now resolves to its direct body slot, the count of direct story items before it, which is the slot StoryItemRef::direct_body_index reports. A cell paragraph item resolves to the direct cell paragraph at the same position. Neither side enters a block content control, so the count and the lookup agree. GitHub issue #163. --- crates/rdocx-py/tests/test_core.py | 34 +++++++++++++++ crates/rdocx/src/document.rs | 38 +++++++++++----- crates/rdocx/tests/regression_test.rs | 62 ++++++++++++++++++++++++++- 3 files changed, 122 insertions(+), 12 deletions(-) diff --git a/crates/rdocx-py/tests/test_core.py b/crates/rdocx-py/tests/test_core.py index 19594290..01c38564 100644 --- a/crates/rdocx-py/tests/test_core.py +++ b/crates/rdocx-py/tests/test_core.py @@ -1383,6 +1383,40 @@ def test_split_run_accepts_a_paragraph_handle_in_a_control_or_the_body(): assert document.to_bytes() == before +def test_story_comment_after_a_block_content_control_anchors_on_its_paragraph(): + import rdocx + + document = _replace_document_body( + rdocx.Document(), + "Alpha." + "" + "Control one." + "" + "Beta paragraph.", + ) + item = next( + item + for item in document.story_items + if item.story.kind == "body" + and item.kind == "paragraph" + and item.text == "Beta paragraph." + ) + comment_id = document.add_comment( + rdocx.StoryRunRange( + start=rdocx.StoryRunPosition(item=item, run_index=0), + end=rdocx.StoryRunPosition(item=item, run_index=1), + ), + author="Ada", + text="Here", + ) + xml = _document_xml(document).decode() + start = xml.index(f'commentRangeStart w:id="{comment_id}"') + end = xml.index(f'commentRangeEnd w:id="{comment_id}"') + assert re.findall(r"]*>([^<]*)", xml[start:end]) == [ + "Beta paragraph." + ] + + def test_python_round_three_authoring_and_inspection_is_typed_and_lossless(): import rdocx diff --git a/crates/rdocx/src/document.rs b/crates/rdocx/src/document.rs index a09eacc7..6564999e 100644 --- a/crates/rdocx/src/document.rs +++ b/crates/rdocx/src/document.rs @@ -13088,7 +13088,7 @@ impl Document { } pub(crate) fn story_paragraph_mut(&mut self, location: &ContentLocation) -> Result<&mut CT_P> { - let (part_name, mut paragraph_index, cell_route) = { + let (part_name, paragraph_slot, cell_route) = { let (source, owner) = self.story_source_and_owner(&location.story)?; let part_name = source.part_name.clone(); let source_xml = source.xml.into_owned(); @@ -13116,15 +13116,25 @@ impl Document { } .into()); } - let paragraph_index = items[..item_index] - .iter() - .filter(|item| item.kind == StoryItemKind::Paragraph) - .count(); + // A paragraph item is a direct child of its owner. In the body it + // resolves to its direct body slot, the count of direct items + // before it, and in a cell to its position among the cell's + // direct paragraphs. Neither counts the paragraphs inside a block + // content control. The body section properties are serialized + // last, so they never precede a paragraph. + let preceding = items[..item_index].iter(); + let paragraph_slot = if location.story.kind == StoryKind::Body { + preceding.filter(|item| item.direct_owner_child).count() + } else { + preceding + .filter(|item| item.kind == StoryItemKind::Paragraph) + .count() + }; let cell_route = (location.story.kind == StoryKind::TableCell && part_name == self.doc_part_name) .then(|| modeled_main_cell_route(&source_xml, &owner)) .transpose()?; - (part_name, paragraph_index, cell_route) + (part_name, paragraph_slot, cell_route) }; if part_name != self.doc_part_name { return Err(Error::Other( @@ -13132,10 +13142,10 @@ impl Document { )); } match location.story.kind { - StoryKind::Body => { - nth_paragraph_in_body(&mut self.document.body.content, &mut paragraph_index) - .ok_or_else(|| Error::Other("comment body paragraph is missing".to_owned())) - } + StoryKind::Body => match self.document.body.content.get_mut(paragraph_slot) { + Some(BodyContent::Paragraph(paragraph)) => Ok(paragraph), + _ => Err(Error::Other("comment body paragraph is missing".to_owned())), + }, StoryKind::TableCell => { let (content_index, mut cell_index) = cell_route.ok_or_else(|| { Error::Other("comment position has no modeled table-cell route".to_owned()) @@ -13154,7 +13164,13 @@ impl Document { BodyContent::Paragraph(_) | BodyContent::RawXml(_) => None, } .ok_or_else(|| Error::Other("comment table cell is missing".to_owned()))?; - nth_paragraph_in_cell(cell, &mut paragraph_index) + cell.content + .iter_mut() + .filter_map(|child| match child { + CellContent::Paragraph(paragraph) => Some(paragraph), + CellContent::Table(_) | CellContent::ContentControl(_) => None, + }) + .nth(paragraph_slot) .ok_or_else(|| Error::Other("comment cell paragraph is missing".to_owned())) } _ => Err(Error::Other( diff --git a/crates/rdocx/tests/regression_test.rs b/crates/rdocx/tests/regression_test.rs index 027a9724..4b0a9faf 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -1638,7 +1638,9 @@ fn checked_table_cell_comment_range_is_atomic_and_reopens() { /// child index, and every body API that takes an index must address the same /// paragraph with it, whatever tables or block content controls precede it. mod direct_body_index_coordinates { - use rdocx::{Document, RunPosition, RunRange}; + use rdocx::{ + Document, RunPosition, RunRange, StoryItemKind, StoryKind, StoryRunPosition, StoryRunRange, + }; const TABLE: &str = r#"c00c01c10c11"#; const CONTROL: &str = r#"Control one.Control two."#; @@ -1809,6 +1811,64 @@ mod direct_body_index_coordinates { } assert_eq!(named("direct").direct_range(), Some(one_run_range(3))); } + + fn comment_on_story_paragraph(document: &mut Document, kind: StoryKind, text: &str) -> i32 { + let story = super::f254_story(document, kind); + let location = document + .story_items(&story) + .unwrap() + .into_iter() + .find(|item| { + item.kind() == StoryItemKind::Paragraph + && item.text().unwrap().as_deref() == Some(text) + }) + .unwrap() + .location() + .clone(); + document + .add_story_comment( + StoryRunRange { + start: StoryRunPosition { + location: location.clone(), + run_index: 0, + }, + end: StoryRunPosition { + location, + run_index: 1, + }, + }, + "Ada", + None, + "Here", + ) + .unwrap() + } + + #[test] + fn story_comment_after_a_block_content_control_anchors_on_its_paragraph() { + let mut document = document_from_body(&format!( + "{}{CONTROL}{}", + paragraph("Alpha."), + paragraph("Beta paragraph.") + )); + let id = comment_on_story_paragraph(&mut document, StoryKind::Body, "Beta paragraph."); + assert_eq!(commented_text(&mut document, id), "Beta paragraph."); + } + + #[test] + fn story_comment_in_a_cell_after_a_block_content_control_anchors_on_its_paragraph() { + let mut document = document_from_body(&format!( + r#"{CONTROL}{}{}"#, + paragraph("Target cell paragraph."), + paragraph("After.") + )); + let id = comment_on_story_paragraph( + &mut document, + StoryKind::TableCell, + "Target cell paragraph.", + ); + assert_eq!(commented_text(&mut document, id), "Target cell paragraph."); + } } fn f_x090_cross_part_drawing_package() -> Vec { From 128e61bdee3ac705a4b31d5becc0e362b9afb925 Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 17:16:53 +0200 Subject: [PATCH 4/8] Re-record the rdocx archive measurement The direct body index changes grow the rdocx package, mostly through the new regression tests, so the crates.io archive row of the root README and its ARCHIVE_MEASUREMENTS entry in scripts/readme_doctests.py are re-measured with cargo package, as the Docs job requires. GitHub issue #163. --- README.md | 2 +- scripts/readme_doctests.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 479b462e..f0ac3418 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ rows are the enforced release-mode bounds plus one dated observation. | Measurement | Value | Version | Platform | Build mode | Input | Command | Statistic | Measured on | |---|---|---|---|---|---|---|---|---| -| Crates.io archive: rdocx | 1,092,256 compressed bytes, 6,498,484 member bytes, 36 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-26 | +| Crates.io archive: rdocx | 1,095,450 compressed bytes, 6,512,553 member bytes, 36 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-26 | | Large-document layout throughput | minimum 250 pages/s, observed 31,019.1 pages/s | rdocx 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | release, one test thread | 1,000 one-page paragraphs with deterministic fonts | `cargo test -p rdocx --test regression_test --release a_thousand_page_document_paginates_and_renders_within_the_declared_limits -- --ignored --exact --nocapture --test-threads=1` | pages per wall-clock second | 2026-09-19 | | Large-document layout peak allocation | maximum 64 MiB, observed 29.03 MiB | rdocx 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | release, one test thread | 1,000 one-page paragraphs with deterministic fonts | `cargo test -p rdocx --test regression_test --release a_thousand_page_document_paginates_and_renders_within_the_declared_limits -- --ignored --exact --nocapture --test-threads=1` | peak live allocation | 2026-09-19 | | Large-document PDF throughput | minimum 1,000 pages/s, observed 60,058.0 pages/s | rdocx 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | release, one test thread | 1,000 deterministic layout pages | `cargo test -p rdocx --test regression_test --release a_thousand_page_document_paginates_and_renders_within_the_declared_limits -- --ignored --exact --nocapture --test-threads=1` | pages per wall-clock second | 2026-09-19 | diff --git a/scripts/readme_doctests.py b/scripts/readme_doctests.py index e24c643b..bd8c280f 100644 --- a/scripts/readme_doctests.py +++ b/scripts/readme_doctests.py @@ -383,7 +383,7 @@ class ReadmeCase: "oxml-opc": (92_122, 355_510, 12), "oxml-pdf": (66_015, 304_432, 14), "oxml-sml": (12_511, 49_803, 6), - "rdocx": (1_092_256, 6_498_484, 36), + "rdocx": (1_095_450, 6_512_553, 36), "rdocx-cli": (33_805, 145_256, 8), "rdocx-html": (15_486, 63_894, 11), "rdocx-layout": (255_752, 1_385_701, 15), From 967425f8c01483a536a15790220ff7b46ccfbab1 Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 21:34:14 +0200 Subject: [PATCH 5/8] Anchor comments and bookmarks on the runs Paragraph.runs lists Comment and bookmark run positions counted only the direct runs of a paragraph, while Paragraph.runs, rdocx text --json, split_run and bookmarks() also count the runs inside inline content controls and tracked insertions. A run index read from either view landed one or more runs further right, so add_comment, add_story_comment, the CLI comment add and add_bookmark wrote their markers on the wrong text and reported success, and the last runs of such a paragraph could not be addressed at all. The accepted view is now the single run index space. The new CT_P::anchor_accepted_range resolves both boundaries to a physical place. Markers go inside w:sdtContent when a range starts or ends between two runs of a control, and around the whole w:sdt when the range covers it. A range that crosses the edge of a control, has a boundary between two runs of a tracked insertion or move, sits next to a tracked change inside a hyperlink, or continues into another paragraph from inside a control is refused instead of shifted. ParagraphRef::runs() now lists the same accepted-view runs as Paragraph::runs(). remove_comment also left markers inside w:sdtContent behind, because it ignored the preserved children there. It now removes them and any reference run left empty. It also recognizes the fixed w prefix of the markers added in the session when the source document gives the Word namespace another prefix, as ElementTree-based producers do. Saving renumbers the added bookmarks in document order, and that pass rewrote only the markers at paragraph level. A bookmark inside a control kept its old id, which could then collide with another one and make the saved file fail to reopen. The pass now reaches the markers inside inline controls, and the in-session bookmark projection also reads the preserved runs of a control written with another prefix. GitHub issue #172. --- crates/rdocx-cli/README.md | 7 +- crates/rdocx-cli/src/main.rs | 6 +- crates/rdocx-cli/tests/integration.rs | 85 +++ crates/rdocx-oxml/src/content_control.rs | 86 +++ crates/rdocx-oxml/src/text.rs | 663 ++++++++++++++++++++++- crates/rdocx-py/tests/test_core.py | 100 ++++ crates/rdocx/src/comments.rs | 269 +++++---- crates/rdocx/src/paragraph.rs | 9 +- crates/rdocx/tests/regression_test.rs | 390 ++++++++++++- docs/hld/03-architecture.md | 15 +- docs/hld/10-bindings-spec.md | 14 +- 11 files changed, 1449 insertions(+), 195 deletions(-) diff --git a/crates/rdocx-cli/README.md b/crates/rdocx-cli/README.md index 25ecc0fe..61d4da98 100644 --- a/crates/rdocx-cli/README.md +++ b/crates/rdocx-cli/README.md @@ -56,8 +56,11 @@ rdocx toc rebuild report.docx -o refreshed.docx ``` Comment `add` ranges use zero-based body paragraph and run boundaries. The -start is inclusive and the end is exclusive. Comment replies, resolution, and -removal select a decimal comment id. +start is inclusive and the end is exclusive. Run boundaries count the runs that +`text --json` lists, including the runs inside inline content controls and +tracked insertions. A range that cannot be anchored exactly, such as one that +crosses the edge of an inline content control, is refused. Comment replies, +resolution, and removal select a decimal comment id. Revision `list` reports the main story. Revision `accept` and `reject` operate across every supported story and accept at most one selector: `--id`, diff --git a/crates/rdocx-cli/src/main.rs b/crates/rdocx-cli/src/main.rs index 54e451e4..5af44025 100644 --- a/crates/rdocx-cli/src/main.rs +++ b/crates/rdocx-cli/src/main.rs @@ -246,13 +246,15 @@ struct CommentRangeArgs { /// Zero-based body paragraph index at the inclusive start #[arg(long)] start_paragraph: usize, - /// Zero-based run boundary at the inclusive start + /// Zero-based run boundary at the inclusive start, counting the runs that + /// `text --json` lists #[arg(long)] start_run: usize, /// Zero-based body paragraph index at the exclusive end #[arg(long)] end_paragraph: usize, - /// Zero-based run boundary at the exclusive end + /// Zero-based run boundary at the exclusive end, counting the runs that + /// `text --json` lists #[arg(long)] end_run: usize, } diff --git a/crates/rdocx-cli/tests/integration.rs b/crates/rdocx-cli/tests/integration.rs index e4cdd98f..79bf5123 100644 --- a/crates/rdocx-cli/tests/integration.rs +++ b/crates/rdocx-cli/tests/integration.rs @@ -963,6 +963,91 @@ fn comment_commands_round_trip_one_resolved_thread() { assert_eq!(Document::open(&input).unwrap().comments().len(), 0); } +/// GitHub issue #172: `comment add` counts runs the way `text --json` lists +/// them, including the runs of an inline content control. +#[test] +fn comment_add_counts_the_runs_that_text_json_lists() { + let temp = TempWorkspace::new("comment-inline-control"); + let input = temp.path.join("input.docx"); + let mut document = fixture_document(&[]); + let mut paragraph = document.add_paragraph(""); + for text in ["before ", "TAR", "GET", " after"] { + paragraph.add_run(text); + } + document.save(&input).unwrap(); + let mut package = OpcPackage::open(&input).unwrap(); + let part = package.main_document_part().unwrap(); + let xml = String::from_utf8(package.get_part(&part).unwrap().to_vec()).unwrap(); + let start = xml[..xml.find(">TAR<").unwrap()].rfind("").unwrap(); + let end = + xml.find(">GET<").unwrap() + xml[xml.find(">GET<").unwrap()..].find("").unwrap(); + let xml = format!( + "{}{}{}", + &xml[..start], + &xml[start..end], + &xml[end + "".len()..] + ); + package.set_part(&part, xml.into_bytes()); + package.save(&input).unwrap(); + + let text = cli(&["text", path_text(&input), "--json"]); + assert_success(&text, "text --json"); + let value: serde_json::Value = serde_json::from_slice(&text.stdout).unwrap(); + let runs = value["paragraphs"][0]["runs"] + .as_array() + .unwrap() + .iter() + .map(|run| run["text"].as_str().unwrap()) + .collect::>(); + assert_eq!(runs, ["before ", "TAR", "GET", " after"]); + + let add = |start_run: &str, end_run: &str, output: &Path| { + cli(&[ + "comment", + "add", + path_text(&input), + "--start-paragraph", + "0", + "--start-run", + start_run, + "--end-paragraph", + "0", + "--end-run", + end_run, + "--author", + "Alice", + "--text", + "Here", + "--output", + path_text(output), + ]) + }; + let added = temp.path.join("added.docx"); + assert_success(&add("1", "3", &added), "comment add"); + let package = OpcPackage::open(&added).unwrap(); + let xml = String::from_utf8(package.get_part(&part).unwrap().to_vec()).unwrap(); + let anchored = + &xml[xml.find("") + .skip(1) + .map(|text| &text[..text.find("").unwrap()]) + .collect::(); + assert_eq!(anchored_text, "TARGET"); + + // From inside the control to after it cannot be anchored exactly. + let refused = temp.path.join("refused.docx"); + let output = add("2", "4", &refused); + assert_eq!(output.status.code(), Some(1)); + assert!( + String::from_utf8_lossy(&output.stderr) + .contains("crosses the edge of an inline content control"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(!refused.exists()); +} + #[test] fn revision_filters_change_only_matching_revisions() { let temp = TempWorkspace::new("revision-filters"); diff --git a/crates/rdocx-oxml/src/content_control.rs b/crates/rdocx-oxml/src/content_control.rs index 8ff31cd5..85cc910f 100644 --- a/crates/rdocx-oxml/src/content_control.rs +++ b/crates/rdocx-oxml/src/content_control.rs @@ -806,6 +806,92 @@ impl CT_Sdt { } } + /// Insert direct `w:sdtContent` children before `index`, keeping the + /// revision and retained run source positions after it aligned. + pub(crate) fn insert_content(&mut self, index: usize, children: Vec) -> bool { + if index > self.content.len() { + return false; + } + let count = children.len(); + for (boundary, _) in &mut self.revisions { + if *boundary >= index { + *boundary += count; + } + } + for source in &mut self.inline_run_sources { + if source.content_index >= index { + source.content_index += count; + } + } + self.content.splice(index..index, children); + true + } + + /// Remove the selected comment range markers and the reference runs + /// that are direct `w:sdtContent` children. + /// + /// A run loses only the selected references and is removed when nothing + /// else remains in it. Nested controls and block content are left to the + /// caller. + #[doc(hidden)] + pub fn remove_comment_anchors(&mut self, ids: &[i32]) { + // Markers added in this session carry the fixed `w` prefix whatever + // prefix the source document gave the Word namespace. + let mut marker_prefixes = vec!["w".to_owned()]; + marker_prefixes.extend(self.content_word_prefixes.iter().cloned()); + let removed = self + .content + .iter_mut() + .map(|child| match child { + SdtContent::Run(run) => { + run.remove_comment_references(ids) + && run.content.is_empty() + && run.extra_xml.is_empty() + && run.alt_drawings.is_empty() + } + SdtContent::RawXml(raw) => { + crate::text::raw_comment_marker_id(raw, &marker_prefixes) + .is_some_and(|id| ids.contains(&id)) + } + _ => false, + }) + .collect::>(); + if !removed.contains(&true) { + return; + } + let kept_before = |index: usize| removed[..index].iter().filter(|remove| !**remove).count(); + self.revisions + .retain(|(boundary, _)| !removed.get(*boundary).copied().unwrap_or(false)); + for (boundary, _) in &mut self.revisions { + *boundary = kept_before(*boundary); + } + self.inline_run_sources + .retain(|source| !removed.get(source.content_index).copied().unwrap_or(false)); + for source in &mut self.inline_run_sources { + source.content_index = kept_before(source.content_index); + } + self.content = std::mem::take(&mut self.content) + .into_iter() + .zip(removed) + .filter_map(|(child, remove)| (!remove).then_some(child)) + .collect(); + } + + /// Remap the facade-authored bookmark markers among the direct and nested + /// inline `w:sdtContent` children. + pub(crate) fn remap_authored_bookmark_ids( + &mut self, + remap: &std::collections::HashMap, + ) { + for content in &mut self.content { + match content { + SdtContent::RawXml(raw) => crate::text::remap_authored_bookmark_marker(raw, remap), + SdtContent::ContentControl(control) => control.remap_authored_bookmark_ids(remap), + _ => {} + } + } + } + pub(crate) fn word_prefixes(&self) -> &[String] { &self.word_prefixes } diff --git a/crates/rdocx-oxml/src/text.rs b/crates/rdocx-oxml/src/text.rs index cd5501ca..d9214b5e 100644 --- a/crates/rdocx-oxml/src/text.rs +++ b/crates/rdocx-oxml/src/text.rs @@ -3462,6 +3462,58 @@ pub enum RunSplitError { BookmarkProjection, } +/// The range kind that [`CT_P::anchor_accepted_range`] writes. +#[doc(hidden)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RangeAnchor<'a> { + /// Comment range markers, with the reference run right after the end. + Comment(i32), + /// Bookmark markers. + Bookmark { id: i32, name: &'a str }, +} + +/// Why [`CT_P::anchor_accepted_range`] could not place a range exactly. +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] +pub enum RangeAnchorError { + #[error("run boundary {boundary} exceeds the paragraph run count {run_count}")] + OutOfRange { boundary: usize, run_count: usize }, + #[error("run range {start}..{end} ends before it starts")] + Reversed { start: usize, end: usize }, + #[error("run range {start}..{end} crosses the edge of an inline content control")] + CrossesControl { start: usize, end: usize }, + #[error( + "run boundary {boundary} falls inside an inline content control, where a range that continues into another paragraph cannot start or end" + )] + InsideControl { boundary: usize }, + #[error("run boundary {boundary} falls inside a tracked insertion or move")] + InsideRevision { boundary: usize }, + #[error("run boundary {boundary} sits next to a tracked change inside a hyperlink")] + HyperlinkRevision { boundary: usize }, + #[error("range markers could not be written into the paragraph")] + Write, +} + +/// One physical place for a range marker. +#[derive(Debug, Clone, PartialEq, Eq)] +enum MarkerSite { + /// A position among the paragraph-level children at a direct run boundary. + Paragraph { boundary: usize, position: usize }, + /// A `w:sdtContent` child index in the inline control that `controls` + /// reaches: a paragraph control index, then nested content indexes. + Control { controls: Vec, index: usize }, +} + +/// A direct run boundary and a position among its paragraph-level children. +type ChildPosition = (usize, usize); + +/// One paragraph-level child at a direct run boundary, in serialization order. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum BoundaryItem { + Control(usize), + Marker(usize), + Raw(usize), +} + /// `CT_P` — A paragraph element containing runs and properties. #[derive(Debug, Clone, PartialEq)] #[allow(non_snake_case)] @@ -4569,6 +4621,497 @@ impl CT_P { projected } + /// Write range markers at accepted-view run boundaries, the run index + /// space of [`Self::accepted_run_paths`]. + /// + /// The markers go inside `w:sdtContent` when the range starts or ends + /// between two runs of an inline content control, and around the whole + /// control when the range covers it. A missing side means the range + /// continues into another paragraph, so that side must not fall inside a + /// control. A comment reference run follows the comment end marker. A + /// range that cannot be written exactly is refused and the paragraph is + /// unchanged. + #[doc(hidden)] + pub fn anchor_accepted_range( + &mut self, + start: Option, + end: Option, + anchor: RangeAnchor<'_>, + ) -> std::result::Result<(), RangeAnchorError> { + let (start_site, end_site) = self.accepted_range_sites(start, end)?; + let mut paragraph = self.clone(); + // The end goes first. A start site never follows it, so the end and + // its reference run leave the start site where it was. + if let Some(site) = end_site { + paragraph.insert_range_marker(site, anchor, false)?; + } + if let Some(site) = start_site { + paragraph.insert_range_marker(site, anchor, true)?; + } + if !paragraph.refresh_bookmark_projection() { + return Err(RangeAnchorError::Write); + } + *self = paragraph; + Ok(()) + } + + /// Resolve where the start and end markers of a range go. + fn accepted_range_sites( + &self, + start: Option, + end: Option, + ) -> std::result::Result<(Option, Option), RangeAnchorError> { + let paths = self.accepted_run_paths(); + let run_count = paths.len(); + for boundary in [start, end].into_iter().flatten() { + if boundary > run_count { + return Err(RangeAnchorError::OutOfRange { + boundary, + run_count, + }); + } + } + // The recursive owner of a run is its path without the final run step. + let owner = |index: usize| { + let segments = paths[index].segments(); + &segments[..segments.len() - 1] + }; + // The deepest owner holding the runs on both sides of a boundary. + let shared = |boundary: usize| match boundary.checked_sub(1) { + Some(left) if boundary < run_count => { + &owner(left)[..common_prefix_len(owner(left), owner(boundary))] + } + _ => &[] as &[AcceptedRunPathSegment], + }; + let (owner_path, blamed) = match (start, end) { + (Some(start), Some(end)) if start > end => { + return Err(RangeAnchorError::Reversed { start, end }); + } + (Some(start), Some(end)) if start == end => (shared(start), start), + (Some(start), Some(end)) => { + // The outermost owner that holds the first and the last run + // of the range and reaches both boundaries. + let (start_depth, end_depth) = (shared(start).len(), shared(end).len()); + let depth = start_depth.max(end_depth); + let first = owner(start); + if depth > common_prefix_len(first, owner(end - 1)) { + return Err(RangeAnchorError::CrossesControl { start, end }); + } + let blamed = if start_depth >= end_depth { start } else { end }; + (&first[..depth], blamed) + } + (Some(boundary), None) | (None, Some(boundary)) => { + let shared = shared(boundary); + if shared + .iter() + .any(|segment| matches!(segment, AcceptedRunPathSegment::Revision(_))) + { + return Err(RangeAnchorError::InsideRevision { boundary }); + } else if !shared.is_empty() { + return Err(RangeAnchorError::InsideControl { boundary }); + } + (shared, boundary) + } + (None, None) => return Ok((None, None)), + }; + let mut controls = Vec::with_capacity(owner_path.len()); + for segment in owner_path { + match *segment { + AcceptedRunPathSegment::ContentControl(index) => controls.push(index), + AcceptedRunPathSegment::Revision(_) | AcceptedRunPathSegment::Run(_) => { + return Err(RangeAnchorError::InsideRevision { boundary: blamed }); + } + } + } + + let end_site = end + .map(|boundary| self.marker_site(&paths, &controls, boundary, false)) + .transpose()?; + let start_site = match (start, end) { + (Some(start), Some(end)) if start == end => end_site.clone(), + _ => start + .map(|boundary| self.marker_site(&paths, &controls, boundary, true)) + .transpose()?, + }; + Ok((start_site, end_site)) + } + + /// Place a marker just before the run after `boundary` for a start, or + /// just after the run before it for an end, inside the owner `controls`. + fn marker_site( + &self, + paths: &[AcceptedRunPath], + controls: &[usize], + boundary: usize, + start: bool, + ) -> std::result::Result { + if !controls.is_empty() { + let path = if start { + paths.get(boundary) + } else { + boundary.checked_sub(1).map(|index| &paths[index]) + } + .ok_or(RangeAnchorError::Write)?; + let control = self.control_at(controls).ok_or(RangeAnchorError::Write)?; + let index = match path.segments()[controls.len()] { + AcceptedRunPathSegment::Run(index) + | AcceptedRunPathSegment::ContentControl(index) => index, + AcceptedRunPathSegment::Revision(index) => { + control + .revisions() + .get(index) + .ok_or(RangeAnchorError::Write)? + .0 + } + }; + return Ok(MarkerSite::Control { + controls: controls.to_vec(), + index: index + usize::from(!start), + }); + } + // A paragraph-level marker must fall between the top-level owners of + // both neighbouring runs, which a revision inside a hyperlink can + // prevent. + let before_right = paths + .get(boundary) + .map(|path| self.paragraph_owner_positions(path).0); + let after_left = boundary + .checked_sub(1) + .map(|index| self.paragraph_owner_positions(&paths[index]).1); + let (Some(before_right), Some(after_left)) = ( + before_right.unwrap_or(Some(( + self.runs.len(), + self.boundary_items(self.runs.len()).len(), + ))), + after_left.unwrap_or(Some((0, 0))), + ) else { + return Err(RangeAnchorError::HyperlinkRevision { boundary }); + }; + let (boundary, position) = if start { before_right } else { after_left }; + Ok(MarkerSite::Paragraph { boundary, position }) + } + + /// Positions just before and just after the paragraph-level owner of one + /// accepted run, or `None` where no paragraph-level child can go. + fn paragraph_owner_positions( + &self, + path: &AcceptedRunPath, + ) -> (Option, Option) { + let around = |boundary: usize, item: BoundaryItem| { + let items = self.boundary_items(boundary); + match items.iter().position(|candidate| *candidate == item) { + Some(position) => (Some((boundary, position)), Some((boundary, position + 1))), + None => (None, None), + } + }; + let raw_at = |boundary: usize, slot: usize| { + self.extra_xml + .iter() + .enumerate() + .filter(|(_, (position, _))| *position == boundary) + .nth(slot) + .map(|(index, _)| BoundaryItem::Raw(index)) + }; + match path.segments()[0] { + AcceptedRunPathSegment::Run(index) => ( + Some((index, self.boundary_items(index).len())), + Some((index + 1, 0)), + ), + AcceptedRunPathSegment::ContentControl(index) => self + .content_controls + .get(index) + .map_or((None, None), |(boundary, _, _, _)| { + around(*boundary, BoundaryItem::Control(index)) + }), + AcceptedRunPathSegment::Revision(index) => { + let Some((boundary, slot, _)) = self.revisions.get(index) else { + return (None, None); + }; + let boundary = *boundary; + let Some(hyperlink) = hyperlink_revision_index(*slot) else { + return raw_at(boundary, *slot) + .map_or((None, None), |item| around(boundary, item)); + }; + match self.hyperlinks.get(hyperlink) { + Some(hyperlink) if hyperlink.preserved_raw_before.is_some() => { + raw_at(boundary, hyperlink.preserved_raw_before.unwrap_or_default()) + .map_or((None, None), |item| around(boundary, item)) + } + // A closing hyperlink writes its revision before the + // paragraph children at its end boundary, and an open + // one writes it after them. + Some(hyperlink) + if hyperlink.run_start < hyperlink.run_end + && boundary == hyperlink.run_end => + { + (None, Some((boundary, 0))) + } + Some(hyperlink) if hyperlink.run_start < hyperlink.run_end => { + (Some((boundary, self.boundary_items(boundary).len())), None) + } + _ => (None, None), + } + } + } + } + + /// Reach the inline control that `controls` names. + fn control_at(&self, controls: &[usize]) -> Option<&CT_Sdt> { + let (first, rest) = controls.split_first()?; + let mut control = &self.content_controls.get(*first)?.3; + for index in rest { + let SdtContent::ContentControl(nested) = control.content.get(*index)? else { + return None; + }; + control = nested; + } + Some(control) + } + + fn control_at_mut(&mut self, controls: &[usize]) -> Option<&mut CT_Sdt> { + let (first, rest) = controls.split_first()?; + let mut control = &mut self.content_controls.get_mut(*first)?.3; + for index in rest { + let SdtContent::ContentControl(nested) = control.content.get_mut(*index)? else { + return None; + }; + control = nested; + } + Some(control) + } + + fn insert_range_marker( + &mut self, + site: MarkerSite, + anchor: RangeAnchor<'_>, + start: bool, + ) -> std::result::Result<(), RangeAnchorError> { + let marker_xml = range_marker_xml(anchor, start).ok_or(RangeAnchorError::Write)?; + let reference = match anchor { + RangeAnchor::Comment(id) if !start => Some(CT_R { + properties: None, + content: vec![RunContent::CommentReference { id, raw_before: 0 }], + extra_xml: Vec::new(), + extra_xml_positions: Vec::new(), + alt_drawings: Vec::new(), + }), + RangeAnchor::Comment(_) | RangeAnchor::Bookmark { .. } => None, + }; + let inserted = match site { + MarkerSite::Control { controls, index } => { + let mut children = vec![SdtContent::RawXml(marker_xml)]; + children.extend(reference.map(SdtContent::Run)); + self.control_at_mut(&controls) + .is_some_and(|control| control.insert_content(index, children)) + } + MarkerSite::Paragraph { boundary, position } => { + let mut items = self.boundary_items(boundary); + let item = match anchor { + RangeAnchor::Comment(id) => { + self.comment_ranges.push(if start { + CommentRangeMarker::Start { + id, + run_index: boundary, + raw_before: 0, + has_child_content: false, + } + } else { + CommentRangeMarker::End { + id, + run_index: boundary, + raw_before: 0, + has_child_content: false, + } + }); + BoundaryItem::Marker(self.comment_ranges.len() - 1) + } + RangeAnchor::Bookmark { .. } => { + self.extra_xml.push((boundary, marker_xml)); + BoundaryItem::Raw(self.extra_xml.len() - 1) + } + }; + items.insert(position.min(items.len()), item); + self.place_boundary_items(&[(boundary, items)]); + match reference { + Some(run) => self.insert_run_after_boundary_items(boundary, position + 1, run), + None => true, + } + } + }; + if inserted { + Ok(()) + } else { + Err(RangeAnchorError::Write) + } + } + + /// List the paragraph-level children at one direct run boundary in the + /// order that serialization writes them. + fn boundary_items(&self, boundary: usize) -> Vec { + let raws = self + .extra_xml + .iter() + .enumerate() + .filter(|(_, (position, _))| *position == boundary) + .map(|(index, _)| index) + .collect::>(); + let mut items = Vec::new(); + for raw_slot in 0..=raws.len() { + let markers = self + .comment_ranges + .iter() + .enumerate() + .filter(|(_, marker)| { + marker.run_index() == boundary + && marker.raw_before().min(raws.len()) == raw_slot + }) + .map(|(index, _)| index) + .collect::>(); + for marker_slot in 0..=markers.len() { + items.extend( + self.content_controls + .iter() + .enumerate() + .filter(|(_, (at, raw_before, markers_before, _))| { + *at == boundary + && (*raw_before).min(raws.len()) == raw_slot + && (*markers_before).min(markers.len()) == marker_slot + }) + .map(|(index, _)| BoundaryItem::Control(index)), + ); + if let Some(marker) = markers.get(marker_slot) { + items.push(BoundaryItem::Marker(*marker)); + } + } + if let Some(raw) = raws.get(raw_slot) { + items.push(BoundaryItem::Raw(*raw)); + } + } + items + } + + /// Give each listed direct run boundary exactly its listed children in + /// order, and move every projection that names a moved raw child. + fn place_boundary_items(&mut self, boundaries: &[(usize, Vec)]) { + let mut raw_moves = Vec::new(); + let mut placed_raws = Vec::new(); + let mut placed_markers = Vec::new(); + for (boundary, items) in boundaries { + let (mut raws, mut markers) = (0, 0); + for item in items { + match *item { + BoundaryItem::Control(index) => { + let control = &mut self.content_controls[index]; + (control.0, control.1, control.2) = (*boundary, raws, markers); + } + BoundaryItem::Marker(index) => { + match &mut self.comment_ranges[index] { + CommentRangeMarker::Start { + run_index, + raw_before, + .. + } + | CommentRangeMarker::End { + run_index, + raw_before, + .. + } => (*run_index, *raw_before) = (*boundary, raws), + } + markers += 1; + placed_markers.push(index); + } + BoundaryItem::Raw(index) => { + let old_boundary = self.extra_xml[index].0; + let old_slot = self.extra_xml[..index] + .iter() + .filter(|(position, _)| *position == old_boundary) + .count(); + raw_moves.push(((old_boundary, old_slot), (*boundary, raws))); + placed_raws.push(index); + raws += 1; + markers = 0; + } + } + } + } + // Serialization reads the raw children and markers of one boundary in + // vector order, so the placed ones move to the end in list order. + let mut extra_xml = std::mem::take(&mut self.extra_xml) + .into_iter() + .map(Some) + .collect::>(); + let placed = placed_raws + .iter() + .zip(&raw_moves) + .filter_map(|(index, (_, (boundary, _)))| { + extra_xml[*index].take().map(|(_, raw)| (*boundary, raw)) + }) + .collect::>(); + self.extra_xml = extra_xml.into_iter().flatten().chain(placed).collect(); + let mut comment_ranges = std::mem::take(&mut self.comment_ranges) + .into_iter() + .map(Some) + .collect::>(); + let placed = placed_markers + .iter() + .filter_map(|index| comment_ranges[*index].take()) + .collect::>(); + self.comment_ranges = comment_ranges.into_iter().flatten().chain(placed).collect(); + + let moved = |at: usize, slot: usize| { + raw_moves + .iter() + .find(|(old, _)| *old == (at, slot)) + .map(|(_, new)| *new) + }; + let mut moved_hyperlinks = Vec::new(); + for (hyperlink_index, hyperlink) in self.hyperlinks.iter_mut().enumerate() { + if hyperlink.run_start == hyperlink.run_end + && let Some(slot) = hyperlink.preserved_raw_before + && let Some((boundary, slot)) = moved(hyperlink.run_start, slot) + { + moved_hyperlinks.push((hyperlink_index, hyperlink.run_start, boundary)); + (hyperlink.run_start, hyperlink.run_end) = (boundary, boundary); + hyperlink.preserved_raw_before = Some(slot); + } + } + for (at, slot, _) in &mut self.revisions { + if let Some(hyperlink) = hyperlink_revision_index(*slot) { + if let Some((_, _, boundary)) = moved_hyperlinks + .iter() + .find(|(index, old, _)| *index == hyperlink && *old == *at) + { + *at = *boundary; + } + } else if let Some(new) = moved(*at, *slot) { + (*at, *slot) = new; + } + } + for (at, slot, _) in &mut self.equations { + if let Some(new) = moved(*at, *slot) { + (*at, *slot) = new; + } + } + } + + /// Insert a direct run at `boundary` after the first `kept` children + /// there, so the remaining children follow the new run. + fn insert_run_after_boundary_items(&mut self, boundary: usize, kept: usize, run: CT_R) -> bool { + let items = self.boundary_items(boundary); + // The insertion moves every child of the boundary after the new run. + if !self.insert_unwrapped_run(boundary, run) { + return false; + } + let (before, after) = items.split_at(kept.min(items.len())); + if !before.is_empty() { + self.place_boundary_items(&[ + (boundary, before.to_vec()), + (boundary + 1, after.to_vec()), + ]); + } + true + } + /// Remap facade-authored bookmark marker ids without reserializing any /// unrelated preserved child XML. #[doc(hidden)] @@ -4577,30 +5120,11 @@ impl CT_P { remap: &std::collections::HashMap, ) -> bool { for (_, raw) in &mut self.extra_xml { - let Ok(text) = std::str::from_utf8(raw) else { - continue; - }; - if !(text.starts_with("() else { - continue; - }; - let Some(updated) = remap.get(&old) else { - continue; - }; - let mut replaced = Vec::with_capacity(raw.len()); - replaced.extend_from_slice(&raw[..value_start]); - replaced.extend_from_slice(updated.to_string().as_bytes()); - replaced.extend_from_slice(&raw[value_end..]); - *raw = replaced; + remap_authored_bookmark_marker(raw, remap); + } + // Markers anchored inside an inline control are its content children. + for (_, _, _, control) in &mut self.content_controls { + control.remap_authored_bookmark_ids(remap); } self.refresh_bookmark_projection() } @@ -4611,10 +5135,17 @@ impl CT_P { format!("\0r\0{R_NS}"), format!("\0mc\0{}", crate::namespace::MC_NS), ]; + // Inline controls keep the source prefix of their preserved runs, + // which the isolated paragraph no longer declares. for prefix in self .bookmark_markers .iter() .flat_map(|marker| marker.word_prefixes.iter()) + .chain( + self.content_controls + .iter() + .flat_map(|(_, _, _, control)| control.word_prefixes()), + ) { if !word_prefixes.contains(prefix) { word_prefixes.push(prefix.clone()); @@ -7406,6 +7937,90 @@ fn raw_xml_count_at(extra_xml: &[(usize, Vec)], run_index: usize) -> usize { .count() } +fn common_prefix_len(left: &[AcceptedRunPathSegment], right: &[AcceptedRunPathSegment]) -> usize { + left.iter() + .zip(right) + .take_while(|(left, right)| left == right) + .count() +} + +/// Serialize one canonical comment or bookmark range marker. +fn range_marker_xml(anchor: RangeAnchor<'_>, start: bool) -> Option> { + let (tag, id, name) = match anchor { + RangeAnchor::Comment(id) if start => ("w:commentRangeStart", id, None), + RangeAnchor::Comment(id) => ("w:commentRangeEnd", id, None), + RangeAnchor::Bookmark { id, name } if start => ("w:bookmarkStart", id, Some(name)), + RangeAnchor::Bookmark { id, .. } => ("w:bookmarkEnd", id, None), + }; + let mut value = itoa::Buffer::new(); + let mut element = BytesStart::new(tag); + element.push_attribute(("w:id", value.format(id))); + if let Some(name) = name { + element.push_attribute(("w:name", name)); + } + let mut raw = Vec::new(); + Writer::new(&mut raw) + .write_event(Event::Empty(element)) + .ok()?; + Some(raw) +} + +/// Rewrite the id of one facade-authored bookmark marker through `remap`, +/// leaving any other raw child unchanged. +pub(crate) fn remap_authored_bookmark_marker( + raw: &mut Vec, + remap: &std::collections::HashMap, +) { + let Ok(text) = std::str::from_utf8(raw) else { + return; + }; + if !(text.starts_with("() else { + return; + }; + let Some(updated) = remap.get(&old) else { + return; + }; + let mut replaced = Vec::with_capacity(raw.len()); + replaced.extend_from_slice(&raw[..value_start]); + replaced.extend_from_slice(updated.to_string().as_bytes()); + replaced.extend_from_slice(&raw[value_end..]); + *raw = replaced; +} + +/// Return the id of a preserved comment range marker, or `None` for any +/// other raw child. +pub(crate) fn raw_comment_marker_id(raw: &[u8], word_prefixes: &[String]) -> Option { + let mut reader = Reader::from_reader(raw); + let mut buffer = Vec::new(); + loop { + match reader.read_event_into(&mut buffer).ok()? { + Event::Start(element) | Event::Empty(element) => { + let prefixes = word_prefixes_at(&element, word_prefixes).ok()?; + let name = element.name(); + if !is_word_element(name.as_ref(), b"commentRangeStart", &prefixes) + && !is_word_element(name.as_ref(), b"commentRangeEnd", &prefixes) + { + return None; + } + return required_word_i32_attribute(&element, b"id", &prefixes).ok(); + } + Event::Eof => return None, + _ => {} + } + buffer.clear(); + } +} + fn parse_hyperlink_children( raw: &[u8], word_prefixes: &[String], diff --git a/crates/rdocx-py/tests/test_core.py b/crates/rdocx-py/tests/test_core.py index 01c38564..afabed02 100644 --- a/crates/rdocx-py/tests/test_core.py +++ b/crates/rdocx-py/tests/test_core.py @@ -1417,6 +1417,106 @@ def test_story_comment_after_a_block_content_control_anchors_on_its_paragraph(): ] +# GitHub issue #172: a run index read from Paragraph.runs anchors on that run, +# including the runs inside an inline content control. +_INLINE_CONTROL_PARAGRAPH = ( + 'before ' + "{runs}" + ' after' +) + + +def _anchored_texts(document, comment_id): + xml = _document_xml(document).decode() + start = xml.index(f'commentRangeStart w:id="{comment_id}"') + end = xml.index(f'commentRangeEnd w:id="{comment_id}"') + return re.findall(r"]*>([^<]*)", xml[start:end]) + + +def _run_range(start, end, body_index=0): + import rdocx + + return rdocx.RunRange( + start=rdocx.RunPosition(body_index=body_index, run_index=start), + end=rdocx.RunPosition(body_index=body_index, run_index=end), + ) + + +def test_comment_run_index_counts_the_runs_of_an_inline_content_control(): + import rdocx + + document = _replace_document_body( + rdocx.Document(), + _INLINE_CONTROL_PARAGRAPH.format(runs="TARGET"), + ) + runs = [run.text for run in document.paragraphs[0].runs] + assert runs == ["before ", "TARGET", " after"] + + target = document.add_comment(_run_range(1, 2), author="A", text="x") + assert _anchored_texts(document, target) == ["TARGET"] + # The reference run of the first comment is run 2 now. + assert [run.text for run in document.paragraphs[0].runs][3] == " after" + last = document.add_comment(_run_range(3, 4), author="A", text="y") + assert _anchored_texts(document, last) == [" after"] + + reopened = rdocx.Document.from_bytes(document.to_bytes()) + assert [comment.text for comment in reopened.comments] == ["x", "y"] + assert _anchored_texts(reopened, target) == ["TARGET"] + + +def test_story_comment_run_index_counts_the_runs_of_an_inline_content_control(): + import rdocx + + document = _replace_document_body( + rdocx.Document(), + _INLINE_CONTROL_PARAGRAPH.format(runs="TARGET"), + ) + item = next( + item + for item in document.story_items + if item.story.kind == "body" and item.kind == "paragraph" + ) + comment_id = document.add_comment( + rdocx.StoryRunRange( + start=rdocx.StoryRunPosition(item=item, run_index=1), + end=rdocx.StoryRunPosition(item=item, run_index=2), + ), + author="A", + text="x", + ) + assert _anchored_texts(document, comment_id) == ["TARGET"] + + +def test_comment_ranges_that_cannot_be_anchored_exactly_are_refused(): + import rdocx + + document = _replace_document_body( + rdocx.Document(), + _INLINE_CONTROL_PARAGRAPH.format( + runs="AB" + ), + ) + before = document.to_bytes() + for start, end in ((0, 2), (2, 4)): + with pytest.raises( + rdocx.RdocxError, match="crosses the edge of an inline content control" + ): + document.add_comment(_run_range(start, end), author="A", text="x") + assert document.to_bytes() == before + + comment_id = document.add_comment(_run_range(2, 3), author="A", text="x") + assert _anchored_texts(document, comment_id) == ["B"] + assert document.remove_comment(comment_id) + xml = _document_xml(document).decode() + assert "commentRange" not in xml and "commentReference" not in xml + assert [run.text for run in document.paragraphs[0].runs] == [ + "before ", + "A", + "B", + " after", + ] + + def test_python_round_three_authoring_and_inspection_is_typed_and_lossless(): import rdocx diff --git a/crates/rdocx/src/comments.rs b/crates/rdocx/src/comments.rs index c067fb8f..a1d0e6ec 100644 --- a/crates/rdocx/src/comments.rs +++ b/crates/rdocx/src/comments.rs @@ -8,10 +8,10 @@ use rdocx_oxml::comments_extended::{CT_CommentEx, CT_CommentsEx}; use rdocx_oxml::content_control::{CT_Sdt, SdtContent}; use rdocx_oxml::document::BodyContent; use rdocx_oxml::table::{CT_Row, CT_Tbl, CT_Tc, CellContent}; -use rdocx_oxml::text::{CT_P, CT_R, CommentRangeMarker, RunContent}; +use rdocx_oxml::text::{CT_P, RangeAnchor}; #[cfg(test)] -use rdocx_oxml::text::HyperlinkSpan; +use rdocx_oxml::text::{CT_R, CommentRangeMarker, HyperlinkSpan, RunContent}; use crate::{ContentLocation, Document, Error, Result}; @@ -30,7 +30,9 @@ pub struct RunPosition { /// Direct body child index, where a table or a block content control /// counts as one child. `Document::find_content_index` returns it. pub body_index: usize, - /// Run insertion index in the selected paragraph. + /// Run boundary in the selected paragraph, counted over the runs that + /// `Paragraph::runs` lists, including the runs inside inline content + /// controls and tracked insertions. pub run_index: usize, } @@ -90,10 +92,9 @@ impl BookmarkRef { /// /// It is `None` when either marker sits in a table cell or a block content /// control, which have no direct body index. Run indexes are the same - /// accepted-view boundaries as [`Self::range`]. They equal the - /// `RunPosition` run index that `Document::add_bookmark` and - /// `Document::add_comment` take only in a paragraph without inline - /// content controls or tracked insertions. + /// accepted-view boundaries as [`Self::range`], which are the + /// `RunPosition` run indexes that `Document::add_bookmark` and + /// `Document::add_comment` take. pub fn direct_range(&self) -> Option { self.direct_range } @@ -439,6 +440,13 @@ impl Document { } /// Insert a bookmark over a half-open range of body paragraph runs. + /// + /// Run indexes count the runs that `Paragraph::runs` lists. The markers + /// go inside an inline content control when the range starts or ends + /// between two of its runs, and around the control when the range covers + /// it. A range that cannot be anchored exactly, such as one that crosses + /// the edge of a control or ends between two runs of a tracked insertion, + /// is an error and leaves the document unchanged. pub fn add_bookmark(&mut self, name: &str, range: RunRange) -> Result { self.insert_bookmark(name, range) } @@ -455,39 +463,12 @@ impl Document { } let mut identifiers = self.identifiers.clone(); let id = identifiers.reserve_bookmark_id()?; - - if range.start.body_index == range.end.body_index { - let mut paragraph = body_paragraph(&self.document.body.content, range.start.body_index) - .expect("bookmark range was validated") - .clone(); - if !paragraph.insert_bookmark_start(range.start.run_index, id, name) - || !paragraph.insert_bookmark_end(range.end.run_index, id) - { - return Err(Error::Other( - "bookmark insertion failed validation".to_owned(), - )); - } - *body_paragraph_mut(&mut self.document.body.content, range.start.body_index) - .expect("bookmark range was validated") = paragraph; - } else { - let mut start = body_paragraph(&self.document.body.content, range.start.body_index) - .expect("bookmark range was validated") - .clone(); - let mut end = body_paragraph(&self.document.body.content, range.end.body_index) - .expect("bookmark range was validated") - .clone(); - if !start.insert_bookmark_start(range.start.run_index, id, name) - || !end.insert_bookmark_end(range.end.run_index, id) - { - return Err(Error::Other( - "bookmark insertion failed validation".to_owned(), - )); - } - *body_paragraph_mut(&mut self.document.body.content, range.start.body_index) - .expect("bookmark range was validated") = start; - *body_paragraph_mut(&mut self.document.body.content, range.end.body_index) - .expect("bookmark range was validated") = end; - } + anchor_body_range( + &mut self.document.body.content, + range, + RangeAnchor::Bookmark { id, name }, + "bookmark", + )?; self.identifiers = identifiers; self.invalidate_layout(); Ok(id) @@ -527,6 +508,11 @@ impl Document { } /// Add a comment over a half-open range of body paragraph runs. + /// + /// Run indexes count the runs that `Paragraph::runs` lists, and the + /// range markers are placed as [`Self::add_bookmark`] places its markers. + /// The reference run follows the end marker. A range that cannot be + /// anchored exactly is an error and leaves the document unchanged. pub fn add_comment( &mut self, range: RunRange, @@ -599,17 +585,16 @@ impl Document { "comment story range start must not follow its end".to_owned(), )); } - let start_original = self.story_paragraph_mut(&range.start.location)?.clone(); - let end_original = self.story_paragraph_mut(&range.end.location)?.clone(); - for (label, position, paragraph) in [ - ("start", &range.start, &start_original), - ("end", &range.end, &end_original), - ] { - if position.run_index > paragraph.runs.len() { + let mut start = self.story_paragraph_mut(&range.start.location)?.clone(); + let mut end = self.story_paragraph_mut(&range.end.location)?.clone(); + for (label, position, paragraph) in + [("start", &range.start, &start), ("end", &range.end, &end)] + { + let run_count = paragraph.accepted_run_paths().len(); + if position.run_index > run_count { return Err(Error::Other(format!( - "comment range {label} run index {} exceeds paragraph run count {}", + "comment range {label} run index {} exceeds paragraph run count {run_count}", position.run_index, - paragraph.runs.len() ))); } } @@ -623,37 +608,29 @@ impl Document { let mut identifiers = self.identifiers.clone(); let id = identifiers.reserve_comment_id()?; if range.start.location == range.end.location { - let mut paragraph = start_original; - insert_comment_reference(&mut paragraph, range.end.run_index, id); - paragraph.comment_ranges.push(CommentRangeMarker::Start { - id, - run_index: range.start.run_index, - raw_before: raw_count_at(¶graph, range.start.run_index), - has_child_content: false, - }); - paragraph.comment_ranges.push(CommentRangeMarker::End { - id, - run_index: range.end.run_index, - raw_before: raw_count_at(¶graph, range.end.run_index), - has_child_content: false, - }); - *self.story_paragraph_mut(&range.start.location)? = paragraph; + anchor_paragraph_range( + &mut start, + Some(range.start.run_index), + Some(range.end.run_index), + RangeAnchor::Comment(id), + "comment", + )?; + *self.story_paragraph_mut(&range.start.location)? = start; } else { - let mut start = start_original; - let mut end = end_original; - insert_comment_reference(&mut end, range.end.run_index, id); - start.comment_ranges.push(CommentRangeMarker::Start { - id, - run_index: range.start.run_index, - raw_before: raw_count_at(&start, range.start.run_index), - has_child_content: false, - }); - end.comment_ranges.push(CommentRangeMarker::End { - id, - run_index: range.end.run_index, - raw_before: raw_count_at(&end, range.end.run_index), - has_child_content: false, - }); + anchor_paragraph_range( + &mut start, + Some(range.start.run_index), + None, + RangeAnchor::Comment(id), + "comment", + )?; + anchor_paragraph_range( + &mut end, + None, + Some(range.end.run_index), + RangeAnchor::Comment(id), + "comment", + )?; *self.story_paragraph_mut(&range.start.location)? = start; *self.story_paragraph_mut(&range.end.location)? = end; } @@ -736,25 +713,12 @@ impl Document { extra_attributes: Vec::new(), }); - let end = body_paragraph_mut(&mut self.document.body.content, range.end.body_index) - .expect("range was validated"); - insert_comment_reference(end, range.end.run_index, id); - let start = body_paragraph_mut(&mut self.document.body.content, range.start.body_index) - .expect("range was validated"); - start.comment_ranges.push(CommentRangeMarker::Start { - id, - run_index: range.start.run_index, - raw_before: raw_count_at(start, range.start.run_index), - has_child_content: false, - }); - let end = body_paragraph_mut(&mut self.document.body.content, range.end.body_index) - .expect("range was validated"); - end.comment_ranges.push(CommentRangeMarker::End { - id, - run_index: range.end.run_index, - raw_before: raw_count_at(end, range.end.run_index), - has_child_content: false, - }); + anchor_body_range( + &mut self.document.body.content, + range, + RangeAnchor::Comment(id), + "comment", + )?; self.identifiers = identifiers; self.comments_dirty = true; self.invalidate_layout(); @@ -1046,11 +1010,11 @@ impl Document { position.body_index )) })?; - if position.run_index > paragraph.runs.len() { + let run_count = paragraph.accepted_run_paths().len(); + if position.run_index > run_count { return Err(Error::Other(format!( - "comment range {label} run index {} exceeds paragraph run count {}", + "comment range {label} run index {} exceeds paragraph run count {run_count}", position.run_index, - paragraph.runs.len() ))); } } @@ -1242,11 +1206,11 @@ fn validate_bookmark_range(content: &[BodyContent], range: RunRange) -> Result<( position.body_index )) })?; - if position.run_index > paragraph.runs.len() { + let run_count = paragraph.accepted_run_paths().len(); + if position.run_index > run_count { return Err(Error::Other(format!( - "bookmark range {label} run index {} exceeds paragraph run count {}", + "bookmark range {label} run index {} exceeds paragraph run count {run_count}", position.run_index, - paragraph.runs.len() ))); } } @@ -1307,6 +1271,54 @@ fn body_paragraph(content: &[BodyContent], index: usize) -> Option<&CT_P> { } } +/// Write range markers over a validated body range. The paragraphs change +/// only when every marker can be placed exactly. +fn anchor_body_range( + content: &mut [BodyContent], + range: RunRange, + anchor: RangeAnchor<'_>, + label: &str, +) -> Result<()> { + let (start, end) = (range.start, range.end); + let paragraph = |content: &[BodyContent], index| { + body_paragraph(content, index) + .cloned() + .expect("range was validated") + }; + let mut first = paragraph(content, start.body_index); + if start.body_index == end.body_index { + anchor_paragraph_range( + &mut first, + Some(start.run_index), + Some(end.run_index), + anchor, + label, + )?; + } else { + let mut last = paragraph(content, end.body_index); + anchor_paragraph_range(&mut first, Some(start.run_index), None, anchor, label)?; + anchor_paragraph_range(&mut last, None, Some(end.run_index), anchor, label)?; + *body_paragraph_mut(content, end.body_index).expect("range was validated") = last; + } + *body_paragraph_mut(content, start.body_index).expect("range was validated") = first; + Ok(()) +} + +/// Write range markers at accepted-view run boundaries, the run index space +/// that `Paragraph::runs` lists. A missing side continues in another +/// paragraph. +fn anchor_paragraph_range( + paragraph: &mut CT_P, + start: Option, + end: Option, + anchor: RangeAnchor<'_>, + label: &str, +) -> Result<()> { + paragraph + .anchor_accepted_range(start, end, anchor) + .map_err(|error| Error::Other(format!("{label} range cannot be anchored: {error}"))) +} + fn collect_main_story_paragraphs<'a>(content: &'a [BodyContent], output: &mut Vec<&'a CT_P>) { for item in content { match item { @@ -1507,32 +1519,6 @@ fn take_paragraph<'a>(paragraph: &'a CT_P, remaining: &mut usize) -> Option<&'a } } -fn raw_count_at(paragraph: &CT_P, run_index: usize) -> usize { - paragraph - .extra_xml - .iter() - .filter(|(position, _)| *position == run_index) - .count() -} - -fn comment_reference_run(id: i32) -> CT_R { - CT_R { - properties: None, - content: vec![RunContent::CommentReference { id, raw_before: 0 }], - extra_xml: Vec::new(), - extra_xml_positions: Vec::new(), - alt_drawings: Vec::new(), - } -} - -fn insert_comment_reference(paragraph: &mut CT_P, run_index: usize, id: i32) { - let inserted = paragraph.insert_unwrapped_run(run_index, comment_reference_run(id)); - debug_assert!( - inserted, - "validated comment insertion index must remain valid" - ); -} - fn thread_root_para_id(extended: &CT_CommentsEx, para_id: &str) -> String { let mut current = para_id.to_owned(); let mut seen = HashSet::new(); @@ -1678,15 +1664,16 @@ fn remove_anchors_from_cell(cell: &mut CT_Tc, ids: &HashSet) { } fn remove_anchors_from_control(control: &mut CT_Sdt, ids: &HashSet) { + // Markers written inside `w:sdtContent` are preserved children there. + control.remove_comment_anchors(&ids.iter().copied().collect::>()); for content in &mut control.content { match content { SdtContent::Paragraph(paragraph) => remove_anchors_from_paragraph(paragraph, ids), SdtContent::Table(table) => remove_anchors_from_table(table, ids), SdtContent::Row(row) => remove_anchors_from_row(row, ids), SdtContent::Cell(cell) => remove_anchors_from_cell(cell, ids), - SdtContent::Run(run) => remove_comment_references_from_run(run, ids), SdtContent::ContentControl(control) => remove_anchors_from_control(control, ids), - SdtContent::RawXml(_) => {} + SdtContent::Run(_) | SdtContent::RawXml(_) => {} } } } @@ -1699,11 +1686,6 @@ fn remove_anchors_from_paragraph(paragraph: &mut CT_P, ids: &HashSet) { paragraph.remove_comment_anchors(&ids); } -fn remove_comment_references_from_run(run: &mut CT_R, ids: &HashSet) { - let ids = ids.iter().copied().collect::>(); - run.remove_comment_references(&ids); -} - fn remap_raw_positions(extra_xml: &mut [(usize, Vec)], removed: &[bool]) { for (position, _) in extra_xml { *position = position.saturating_sub( @@ -2149,9 +2131,16 @@ mod tests { extra_xml_positions: Vec::new(), alt_drawings: Vec::new(), }); - let mut reference = comment_reference_run(7); - reference.properties = Some(Default::default()); - paragraph.runs.push(reference); + paragraph.runs.push(CT_R { + properties: Some(Default::default()), + content: vec![RunContent::CommentReference { + id: 7, + raw_before: 0, + }], + extra_xml: Vec::new(), + extra_xml_positions: Vec::new(), + alt_drawings: Vec::new(), + }); remove_anchors_from_paragraph(&mut paragraph, &HashSet::from([7])); diff --git a/crates/rdocx/src/paragraph.rs b/crates/rdocx/src/paragraph.rs index cd94abe6..416fb658 100644 --- a/crates/rdocx/src/paragraph.rs +++ b/crates/rdocx/src/paragraph.rs @@ -2561,9 +2561,14 @@ impl<'a> ParagraphRef<'a> { .map(|twips| Length::twips(twips.0.saturating_neg())) } - /// Get an iterator over immutable run references. + /// Get an iterator over immutable accepted-view run references, the runs + /// that [`Self::run`] indexes, including those inside inline content + /// controls and tracked insertions. pub fn runs(&self) -> impl Iterator> { - self.inner.runs.iter().map(|r| RunRef { inner: r }) + self.inner + .accepted_bookmark_runs() + .into_iter() + .map(|inner| RunRef { inner }) } /// Iterate over the paragraph's ruby annotations in source order. diff --git a/crates/rdocx/tests/regression_test.rs b/crates/rdocx/tests/regression_test.rs index 4b0a9faf..c80fa897 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -1871,6 +1871,339 @@ mod direct_body_index_coordinates { } } +/// GitHub issue #172: a run index read from `Paragraph::runs` or +/// `rdocx text --json` counts the runs inside inline content controls and +/// tracked insertions, and comment and bookmark anchoring use the same count. +mod accepted_run_index_anchoring { + use rdocx::{ + Document, RunPosition, RunRange, StoryItemKind, StoryKind, StoryRunPosition, StoryRunRange, + }; + + /// `before ` | control `TARGET` | ` after`, the issue reproduction. + const INLINE_CONTROL: &str = r#"before TARGET after"#; + /// `before ` | control `A` `B` | ` after`. + const TWO_RUN_CONTROL: &str = r#"before AB after"#; + /// The same paragraph in a document that names the Word namespace `ns0`, + /// as ElementTree-based producers write it. + const NS0_TWO_RUN_CONTROL: &str = r#"before AB after"#; + + fn document_from_body(body: &str) -> Document { + super::document_with_content_controls(&super::wrap_word_body(body)) + } + + fn range(start: (usize, usize), end: (usize, usize)) -> RunRange { + RunRange { + start: RunPosition { + body_index: start.0, + run_index: start.1, + }, + end: RunPosition { + body_index: end.0, + run_index: end.1, + }, + } + } + + fn run_texts(document: &Document, index: usize) -> Vec { + document + .paragraph(index) + .unwrap() + .runs() + .map(|run| run.text()) + .collect() + } + + /// The `w:t` text of an XML slice that may cross element boundaries. + fn slice_text(xml: &str) -> String { + let mut text = String::new(); + let mut rest = xml; + while let Some(index) = rest.find("') else { + break; + }; + if !(rest.starts_with('>') || rest.starts_with(' ')) || rest[..open_end].ends_with('/') + { + continue; + } + rest = &rest[open_end + 1..]; + let close = rest.find("").expect("text element end"); + text.push_str(&rest[..close]); + rest = &rest[close..]; + } + text + } + + /// The saved main part and the part between the range markers of `id`, + /// markers included. + fn anchored_xml(document: &mut Document, id: i32) -> (String, String) { + let xml = super::document_xml(document); + let start = xml + .find(&format!(r#""#)) + .expect("comment start marker"); + let end_marker = format!(r#""#); + let end = xml.find(&end_marker).expect("comment end marker") + end_marker.len(); + let anchored = xml[start..end].to_owned(); + (xml, anchored) + } + + fn comment(document: &mut Document, range: RunRange) -> rdocx::Result { + document.add_comment(range, "Ada", None, "Here") + } + + #[test] + fn comment_run_index_counts_the_runs_of_an_inline_control() { + let mut document = document_from_body(INLINE_CONTROL); + assert_eq!(run_texts(&document, 0), ["before ", "TARGET", " after"]); + + let id = comment(&mut document, range((0, 1), (0, 2))).unwrap(); + let (xml, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), "TARGET"); + // The range covers the whole control, so its markers surround it. + assert!(anchored.contains("") && anchored.contains("")); + let end = xml.find("").unwrap(); + assert!(end < reference && reference < after, "{xml}"); + + // The last run of the paragraph is addressable too. + let id = comment(&mut document, range((0, 3), (0, 4))).unwrap(); + let (_, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), " after"); + + let mut reopened = Document::from_bytes(&document.to_bytes().unwrap()).unwrap(); + assert_eq!(reopened.comments().len(), 2); + assert_eq!(reopened.content_controls()[0].text(), "TARGET"); + let (_, anchored) = anchored_xml(&mut reopened, id); + assert_eq!(slice_text(&anchored), " after"); + } + + #[test] + fn comment_inside_a_control_is_written_inside_its_content() { + let mut document = document_from_body(TWO_RUN_CONTROL); + assert_eq!(run_texts(&document, 0), ["before ", "A", "B", " after"]); + + let id = comment(&mut document, range((0, 1), (0, 2))).unwrap(); + let (xml, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), "A"); + let content = xml.find("").unwrap(); + let start = xml.find("B").unwrap(); + let content_end = xml.find("").unwrap(); + assert!(content < start && reference < b && b < content_end, "{xml}"); + + let mut reopened = Document::from_bytes(&document.to_bytes().unwrap()).unwrap(); + assert_eq!(reopened.content_controls()[0].text(), "AB"); + let (_, anchored) = anchored_xml(&mut reopened, id); + assert_eq!(slice_text(&anchored), "A"); + } + + #[test] + fn comment_on_a_nested_control_goes_around_it_inside_the_outer_one() { + let mut document = document_from_body( + r#"XYZ"#, + ); + assert_eq!(run_texts(&document, 0), ["X", "Y", "Z"]); + let id = comment(&mut document, range((0, 1), (0, 2))).unwrap(); + let (xml, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), "Y"); + let x = xml.find("X").unwrap(); + let z = xml.find("Z").unwrap(); + let inner = xml.rfind("").unwrap(); + let start = xml.find("A BC"# + )); + assert_eq!(run_texts(&document, 1), ["A ", "B", "C"]); + let before = document.to_bytes().unwrap(); + for (range, reason) in [ + // From outside the control to between its two runs. + ( + range((0, 0), (0, 2)), + "crosses the edge of an inline content control", + ), + ( + range((0, 2), (0, 4)), + "crosses the edge of an inline content control", + ), + // Between two runs of one tracked insertion. + (range((1, 2), (1, 3)), "inside a tracked insertion"), + // A range that continues into another paragraph from inside a control. + (range((0, 2), (1, 1)), "inside an inline content control"), + (range((0, 4), (0, 5)), "exceeds paragraph run count 4"), + ] { + let error = comment(&mut document, range).unwrap_err().to_string(); + assert!(error.contains(reason), "{range:?}: {error}"); + let error = document.add_bookmark("refused", range).unwrap_err(); + assert!(error.to_string().contains(reason), "{range:?}: {error}"); + } + assert_eq!(document.to_bytes().unwrap(), before); + + // The whole insertion can be commented, and so can a range that + // starts before the control and continues into the next paragraph. + let id = comment(&mut document, range((1, 1), (1, 3))).unwrap(); + let (_, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), "BC"); + let id = comment(&mut document, range((0, 1), (1, 1))).unwrap(); + let (_, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), "AB afterA "); + } + + #[test] + fn removing_a_comment_clears_markers_inside_control_content() { + let mut document = document_from_body(TWO_RUN_CONTROL); + let original = super::document_xml(&mut document); + let id = comment(&mut document, range((0, 2), (0, 3))).unwrap(); + assert!(super::document_xml(&mut document).contains("AB"#, + id = 0 + )); + assert!(document.remove_comment(id).unwrap()); + let removed = super::document_xml(&mut document); + assert!(!removed.contains("commentRange"), "{removed}"); + assert!(!removed.contains("commentReference"), "{removed}"); + assert!( + removed.contains("AB"), + "{removed}" + ); + } + + #[test] + fn removing_a_comment_clears_fixed_prefix_markers_in_a_document_of_another_prefix() { + // The markers added inside the control use the fixed `w` prefix. + let mut document = super::document_with_content_controls(NS0_TWO_RUN_CONTROL); + assert_eq!(run_texts(&document, 0), ["before ", "A", "B", " after"]); + let marker_counts = |xml: &str| { + ["commentRangeStart", "commentRangeEnd", "commentReference"] + .map(|name| xml.matches(name).count()) + }; + + let id = comment(&mut document, range((0, 2), (0, 3))).unwrap(); + assert_eq!(marker_counts(&super::document_xml(&mut document)), [1; 3]); + assert!(document.remove_comment(id).unwrap()); + let removed = super::document_xml(&mut document); + assert_eq!(marker_counts(&removed), [0; 3], "{removed}"); + + // The next comment reuses the id and owns the only marker pair. + let next = comment(&mut document, range((0, 0), (0, 1))).unwrap(); + assert_eq!(next, id); + let xml = super::document_xml(&mut document); + assert_eq!(marker_counts(&xml), [1; 3], "{xml}"); + let (_, anchored) = anchored_xml(&mut document, next); + assert_eq!(slice_text(&anchored), "before "); + } + + #[test] + fn bookmarks_inside_and_around_a_control_keep_their_text_and_distinct_ids() { + for body in [ + super::wrap_word_body(TWO_RUN_CONTROL), + NS0_TWO_RUN_CONTROL.to_owned(), + ] { + let mut document = super::document_with_content_controls(&body); + // Saving renumbers added bookmarks in document order, which runs + // against the order they were added in. + for (name, range) in [ + ("inside", range((0, 2), (0, 3))), + ("after", range((0, 3), (0, 4))), + ("before", range((0, 0), (0, 1))), + ] { + document.add_bookmark(name, range).unwrap(); + } + let reopened = Document::from_bytes(&document.to_bytes().unwrap()).unwrap(); + for document in [&document, &reopened] { + let bookmarks = document + .bookmarks() + .iter() + .map(|bookmark| { + ( + bookmark.name().unwrap().to_owned(), + bookmark.text().to_owned(), + ) + }) + .collect::>(); + assert_eq!( + bookmarks, + [ + ("before".to_owned(), "before ".to_owned()), + ("inside".to_owned(), "B".to_owned()), + ("after".to_owned(), " after".to_owned()), + ] + ); + } + let mut ids = reopened + .bookmarks() + .iter() + .map(|bookmark| bookmark.id()) + .collect::>(); + ids.dedup(); + assert_eq!(ids, [Some(0), Some(1), Some(2)]); + } + } + + #[test] + fn story_comment_and_bookmark_use_the_same_run_index() { + let mut document = document_from_body(INLINE_CONTROL); + let story = super::f254_story(&document, StoryKind::Body); + let location = document + .story_items(&story) + .unwrap() + .into_iter() + .find(|item| item.kind() == StoryItemKind::Paragraph) + .unwrap() + .location() + .clone(); + let position = |run_index| StoryRunPosition { + location: location.clone(), + run_index, + }; + let id = document + .add_story_comment( + StoryRunRange { + start: position(1), + end: position(2), + }, + "Ada", + None, + "Here", + ) + .unwrap(); + let (_, anchored) = anchored_xml(&mut document, id); + assert_eq!(slice_text(&anchored), "TARGET"); + + let mut document = document_from_body(TWO_RUN_CONTROL); + document + .add_bookmark("second", range((0, 2), (0, 3))) + .unwrap(); + let reopened = Document::from_bytes(&document.to_bytes().unwrap()).unwrap(); + for document in [&document, &reopened] { + let bookmark = &document.bookmarks()[0]; + assert_eq!(bookmark.text(), "B"); + assert_eq!(bookmark.direct_range(), Some(range((0, 2), (0, 3)))); + } + } +} + fn f_x090_cross_part_drawing_package() -> Vec { let mut document = f255_story_document(); document.add_picture(b"body image", "body.png", Length::pt(1.0), Length::pt(1.0)); @@ -15406,30 +15739,48 @@ fn comment_insertion_keeps_content_at_the_hyperlink_end_boundary() { r#"onetwoend{raw}"# )); let mut document = document_with_content_controls(&xml); + let range = |end| RunRange { + start: RunPosition { + body_index: 0, + run_index: 0, + }, + end: RunPosition { + body_index: 0, + run_index: end, + }, + }; + // The accepted view lists `one`, `two` and the inserted `end`. Ending the + // range after `two` would need an end marker inside the hyperlink before + // the insertion, which the paragraph model cannot place, so it is refused + // rather than silently widened over `end` as before GitHub issue #172. + let before = document.to_bytes().unwrap(); + let error = document + .add_comment(range(2), "Ada", None, "review") + .unwrap_err(); + assert!( + error + .to_string() + .contains("next to a tracked change inside a hyperlink"), + "{error}" + ); + assert_eq!(document.to_bytes().unwrap(), before); + let comment_id = document - .add_comment( - RunRange { - start: RunPosition { - body_index: 0, - run_index: 0, - }, - end: RunPosition { - body_index: 0, - run_index: 2, - }, - }, - "Ada", - None, - "review", - ) + .add_comment(range(3), "Ada", None, "review") .unwrap(); let inserted = document_xml(&mut document); let revision = inserted.find(r#"w:id="43""#).unwrap(); let raw_position = inserted.find(raw).unwrap(); let hyperlink_end = inserted.find("").unwrap(); + let range_end = inserted.find(" Date: Sun, 27 Sep 2026 21:40:22 +0200 Subject: [PATCH 6/8] Let story comments reach paragraphs inside block controls add_comment could not reach a paragraph inside a block content control. Its direct body index names the control, a StoryRunRange over the control item was refused as not a paragraph, and story_items lists no item for the paragraphs inside a control. A body ContentLocation may now have two segments, the control's story item index and the paragraph's position among the control's paragraphs, and add_story_comment resolves it inside the control. The new Document::paragraph_story_location returns that path, or the one-segment path of a direct paragraph, for a paragraph index. In Python, StoryRunPosition accepts a Paragraph handle in place of a StoryItem and builds the item from it. Table cell handles are refused. rebuild_toc replaces the cached entry paragraphs, which drops the comment and bookmark markers placed on them, and its documentation now says so. GitHub issue #163. --- crates/rdocx-py/python/rdocx/_rdocx.pyi | 9 +- crates/rdocx-py/src/document.rs | 66 +++++++++++++-- crates/rdocx-py/src/paragraph.rs | 2 +- crates/rdocx-py/tests/test_core.py | 51 ++++++++++++ crates/rdocx-py/tests/typing_smoke.py | 3 +- crates/rdocx/src/comments.rs | 8 +- crates/rdocx/src/document.rs | 105 +++++++++++++++++++++--- crates/rdocx/src/field.rs | 4 + crates/rdocx/tests/regression_test.rs | 94 +++++++++++++++++++++ docs/hld/03-architecture.md | 6 +- docs/hld/10-bindings-spec.md | 10 ++- 11 files changed, 333 insertions(+), 25 deletions(-) diff --git a/crates/rdocx-py/python/rdocx/_rdocx.pyi b/crates/rdocx-py/python/rdocx/_rdocx.pyi index 4b3cfcfb..8aa11dc8 100644 --- a/crates/rdocx-py/python/rdocx/_rdocx.pyi +++ b/crates/rdocx-py/python/rdocx/_rdocx.pyi @@ -225,7 +225,14 @@ class StoryItem: @_final class StoryRunPosition: - def __new__(cls, *, item: StoryItem, run_index: int) -> StoryRunPosition: ... + @_overload + def __new__( + cls, *, item: StoryItem, run_index: int, paragraph: None = None + ) -> StoryRunPosition: ... + @_overload + def __new__( + cls, *, item: None = None, run_index: int, paragraph: Paragraph + ) -> StoryRunPosition: ... @property def item(self) -> StoryItem: ... @property diff --git a/crates/rdocx-py/src/document.rs b/crates/rdocx-py/src/document.rs index 5d343dca..6e6a313f 100644 --- a/crates/rdocx-py/src/document.rs +++ b/crates/rdocx-py/src/document.rs @@ -390,13 +390,26 @@ pub struct PyStoryRunPosition { #[pymethods] impl PyStoryRunPosition { + /// Take a `StoryItem`, or a `Paragraph` handle, which also reaches a + /// paragraph inside a block content control. #[new] - #[pyo3(signature = (*, item, run_index))] - fn new(item: PyRef<'_, PyStoryItem>, run_index: usize) -> Self { - Self { - item: item.clone(), - run_index, - } + #[pyo3(signature = (*, item = None, run_index, paragraph = None))] + fn new( + py: Python<'_>, + item: Option>, + run_index: usize, + paragraph: Option>, + ) -> PyResult { + let item = match (item, paragraph) { + (Some(item), None) => item.clone(), + (None, Some(paragraph)) => PyDocument::paragraph_story_item(py, ¶graph)?, + _ => { + return Err(PyTypeError::new_err( + "StoryRunPosition takes exactly one of item and paragraph", + )); + } + }; + Ok(Self { item, run_index }) } #[getter] @@ -933,6 +946,47 @@ impl PyDocument { }) } + /// Snapshot the story item of a body paragraph handle. A paragraph + /// inside a block content control has no story item of its own, so it + /// gets the two-segment path of `paragraph_story_location` with the + /// paragraph text and no XML. + fn paragraph_story_item(py: Python<'_>, paragraph: &PyParagraph) -> PyResult { + let ParagraphLocation::Body(paragraph_index) = paragraph.validate(py)? else { + return Err(PyValueError::new_err( + "StoryRunPosition does not accept a table cell paragraph handle", + )); + }; + let document = paragraph.document.borrow(py); + let location = document + .inner + .paragraph_story_location(paragraph_index) + .map_err(|error| rdocx_to_pyerr(py, error))? + .ok_or_else(|| PyIndexError::new_err("paragraph index out of range"))?; + let [control_index, _] = location.index_path() else { + return document.story_item_snapshot(py, &location); + }; + let control = document.story_item_snapshot( + py, + &rdocx::ContentLocation::new( + location.story().clone(), + rdocx::StoryItemKind::ContentControl, + vec![*control_index], + ), + )?; + Ok(PyStoryItem { + story: control.story, + kind: "paragraph".to_owned(), + index_path: location.index_path().to_vec(), + direct_body_index: control.direct_body_index, + text: document + .inner + .paragraph(paragraph_index) + .map(|paragraph| paragraph.text()), + xml: Vec::new(), + revision: document.revisions.current(), + }) + } + fn direct_content_index( slf: &Py, py: Python<'_>, diff --git a/crates/rdocx-py/src/paragraph.rs b/crates/rdocx-py/src/paragraph.rs index ca1eed82..a3aa8bd6 100644 --- a/crates/rdocx-py/src/paragraph.rs +++ b/crates/rdocx-py/src/paragraph.rs @@ -55,7 +55,7 @@ pub(crate) fn paragraph_location(path: &ContentPath) -> PyResult, + pub(crate) document: Py, path: ContentPath, } diff --git a/crates/rdocx-py/tests/test_core.py b/crates/rdocx-py/tests/test_core.py index afabed02..ec2b2090 100644 --- a/crates/rdocx-py/tests/test_core.py +++ b/crates/rdocx-py/tests/test_core.py @@ -1517,6 +1517,57 @@ def test_comment_ranges_that_cannot_be_anchored_exactly_are_refused(): ] +def test_story_run_position_takes_a_paragraph_handle_inside_a_block_control(): + import rdocx + + # GitHub issue #163: a paragraph inside a block content control has no + # story item of its own, and a handle reaches it. + document = _replace_document_body( + rdocx.Document(), + "Alpha." + "" + "Control one." + "Control two." + "" + '' + "Cell." + "" + "Beta.", + ) + control_two = document.paragraphs[2] + assert control_two.text == "Control two." + position = rdocx.StoryRunPosition(paragraph=control_two, run_index=0) + assert len(position.item.index_path) == 2 + assert position.item.text == "Control two." + assert position.item.direct_body_index == 1 + comment_id = document.add_comment( + rdocx.StoryRunRange( + start=position, + end=rdocx.StoryRunPosition(paragraph=control_two, run_index=1), + ), + author="Ada", + text="Here", + ) + assert _anchored_texts(document, comment_id) == ["Control two."] + xml = _document_xml(document).decode() + assert xml.index("") < xml.index("commentRangeStart") + assert xml.index("commentRangeEnd") < xml.index("") + with pytest.raises(rdocx.StaleElementError): + rdocx.StoryRunPosition(paragraph=control_two, run_index=0) + + beta = document.paragraphs[3] + beta_item = next(item for item in document.story_items if item.text == "Beta.") + assert rdocx.StoryRunPosition(paragraph=beta, run_index=0).item == beta_item + + cell_paragraph = document.tables[0].rows[0].cells[0].paragraphs[0] + with pytest.raises(ValueError, match="table cell paragraph handle"): + rdocx.StoryRunPosition(paragraph=cell_paragraph, run_index=0) + with pytest.raises(TypeError, match="exactly one of item and paragraph"): + rdocx.StoryRunPosition(item=beta_item, paragraph=beta, run_index=0) + with pytest.raises(TypeError, match="exactly one of item and paragraph"): + rdocx.StoryRunPosition(run_index=0) + + def test_python_round_three_authoring_and_inspection_is_typed_and_lossless(): import rdocx diff --git a/crates/rdocx-py/tests/typing_smoke.py b/crates/rdocx-py/tests/typing_smoke.py index ae8f1821..2165f877 100644 --- a/crates/rdocx-py/tests/typing_smoke.py +++ b/crates/rdocx-py/tests/typing_smoke.py @@ -108,7 +108,8 @@ def exercise_rdocx_types(path: Path) -> None: b"png", "image.png", Inches(1), Inches(1), after=story_items[0] ) story_position = StoryRunPosition(item=story_items[0], run_index=0) - story_range = StoryRunRange(start=story_position, end=story_position) + handle_position = StoryRunPosition(paragraph=document.paragraphs[0], run_index=0) + story_range = StoryRunRange(start=story_position, end=handle_position) story_comment_id: int = document.add_comment( story_range, author="Ada", text="story review" ) diff --git a/crates/rdocx/src/comments.rs b/crates/rdocx/src/comments.rs index a1d0e6ec..9b5d5014 100644 --- a/crates/rdocx/src/comments.rs +++ b/crates/rdocx/src/comments.rs @@ -543,6 +543,11 @@ impl Document { } /// Add a dated comment over a checked body or table-cell run range. + /// + /// A body location can also name a paragraph inside a block content + /// control with the two-segment path that + /// [`Document::paragraph_story_location`] returns. Run indexes count the + /// runs that `Paragraph::runs` lists, as in [`Document::add_comment`]. pub fn add_story_comment_with_date( &mut self, range: StoryRunRange, @@ -558,7 +563,8 @@ impl Document { Ok(id) } - /// Add a comment over a checked body or table-cell run range. + /// Add a comment over a checked body or table-cell run range, as + /// [`Self::add_story_comment_with_date`] does without a date. pub fn add_story_comment( &mut self, range: StoryRunRange, diff --git a/crates/rdocx/src/document.rs b/crates/rdocx/src/document.rs index 6564999e..2feda35e 100644 --- a/crates/rdocx/src/document.rs +++ b/crates/rdocx/src/document.rs @@ -13088,18 +13088,26 @@ impl Document { } pub(crate) fn story_paragraph_mut(&mut self, location: &ContentLocation) -> Result<&mut CT_P> { - let (part_name, paragraph_slot, cell_route) = { + let (part_name, paragraph_slot, ordinal, cell_route) = { let (source, owner) = self.story_source_and_owner(&location.story)?; let part_name = source.part_name.clone(); let source_xml = source.xml.into_owned(); - if location.index_path.len() != 1 { - return Err(StoryError::InvalidPath { - path: location.index_path.clone(), + // A paragraph inside a block content control of the body is the + // control item followed by the paragraph's position among the + // paragraphs of that control. + let (item_index, ordinal) = match location.index_path.as_slice() { + [item_index] => (*item_index, None), + [item_index, ordinal] if location.story.kind == StoryKind::Body => { + (*item_index, Some(*ordinal)) } - .into()); - } + _ => { + return Err(StoryError::InvalidPath { + path: location.index_path.clone(), + } + .into()); + } + }; let items = scan_story_items(&source_xml, &owner)?; - let item_index = location.index_path[0]; let item = items.get(item_index).ok_or(StoryError::OutOfBounds { index: item_index, len: items.len(), @@ -13109,7 +13117,15 @@ impl Document { "comment positions must identify paragraphs".to_owned(), )); } - if item.kind != location.item_kind { + if ordinal.is_some() + && (item.kind != StoryItemKind::ContentControl || !item.direct_owner_child) + { + return Err(Error::Other( + "a two-segment comment position must start with a block content control" + .to_owned(), + )); + } + if ordinal.is_none() && item.kind != location.item_kind { return Err(StoryError::KindMismatch { expected: location.item_kind, actual: item.kind, @@ -13134,7 +13150,7 @@ impl Document { && part_name == self.doc_part_name) .then(|| modeled_main_cell_route(&source_xml, &owner)) .transpose()?; - (part_name, paragraph_slot, cell_route) + (part_name, paragraph_slot, ordinal, cell_route) }; if part_name != self.doc_part_name { return Err(Error::Other( @@ -13142,10 +13158,20 @@ impl Document { )); } match location.story.kind { - StoryKind::Body => match self.document.body.content.get_mut(paragraph_slot) { - Some(BodyContent::Paragraph(paragraph)) => Ok(paragraph), - _ => Err(Error::Other("comment body paragraph is missing".to_owned())), - }, + StoryKind::Body => { + match (self.document.body.content.get_mut(paragraph_slot), ordinal) { + (Some(BodyContent::Paragraph(paragraph)), None) => Ok(paragraph), + (Some(BodyContent::ContentControl(control)), Some(mut remaining)) => { + nth_paragraph_in_control(control, &mut remaining).ok_or_else(|| { + Error::Other(format!( + "comment content control has no paragraph {}", + ordinal.unwrap_or_default() + )) + }) + } + _ => Err(Error::Other("comment body paragraph is missing".to_owned())), + } + } StoryKind::TableCell => { let (content_index, mut cell_index) = cell_route.ok_or_else(|| { Error::Other("comment position has no modeled table-cell route".to_owned()) @@ -15078,6 +15104,59 @@ impl Document { .position(|content| matches!(content, BodyContent::Paragraph(paragraph) if std::ptr::eq(paragraph, target))) } + /// Return the checked body story location of the paragraph addressed by + /// `paragraph_index`, the index of [`Self::paragraph_mut`]. + /// + /// A direct body paragraph has its one-segment story item path. A + /// paragraph inside a block content control, which has no story item of + /// its own, has a two-segment path: the control's story item index, then + /// the paragraph's position among the control's paragraphs. Only + /// [`Self::add_story_comment`] accepts the two-segment form. Returns + /// `None` when the index is out of range. + pub fn paragraph_story_location( + &self, + paragraph_index: usize, + ) -> Result> { + let mut remaining = paragraph_index; + let mut target = None; + for (slot, child) in self.document.body.content.iter().enumerate() { + let count = match child { + BodyContent::Paragraph(_) => 1, + BodyContent::ContentControl(control) => paragraph_count_in_control(control), + BodyContent::Table(_) | BodyContent::RawXml(_) => 0, + }; + if remaining < count { + let ordinal = matches!(child, BodyContent::ContentControl(_)).then_some(remaining); + target = Some((slot, ordinal)); + break; + } + remaining -= count; + } + let Some((slot, ordinal)) = target else { + return Ok(None); + }; + let story = self + .stories()? + .into_iter() + .find(|story| story.kind == StoryKind::Body) + .ok_or_else(|| Error::Other("document body story is missing".to_owned()))?; + let (source, owner) = self.story_source_and_owner(&story)?; + // The body section properties come last, so the n-th direct item is + // the direct body child at slot n. + let item_index = scan_story_items(source.xml.as_ref(), &owner)? + .iter() + .enumerate() + .filter(|(_, item)| item.direct_owner_child) + .nth(slot) + .map(|(index, _)| index) + .ok_or_else(|| Error::Other("body paragraph has no story item".to_owned()))?; + Ok(Some(ContentLocation::new( + story, + StoryItemKind::Paragraph, + std::iter::once(item_index).chain(ordinal).collect(), + ))) + } + /// Return the direct body index of the table addressed by `table_index`. pub fn content_index_of_table(&self, table_index: usize) -> Option { let target = self.table(table_index)?.inner; diff --git a/crates/rdocx/src/field.rs b/crates/rdocx/src/field.rs index 6d3f6089..024bd8b1 100644 --- a/crates/rdocx/src/field.rs +++ b/crates/rdocx/src/field.rs @@ -1014,6 +1014,10 @@ impl Document { /// The operation stages bookmarks, cached entries, and deterministic page /// targets on an independent document. Any malformed or ambiguous source /// leaves the receiver unchanged. + /// + /// The cached entry paragraphs are replaced, so comment and bookmark + /// markers placed on them are dropped with them. A comment anchored only + /// there stays in the comments part without an anchor. pub fn rebuild_toc(&mut self) -> Result { let mut candidate = self.clone_for_staging(); candidate.prepare_staged_package()?; diff --git a/crates/rdocx/tests/regression_test.rs b/crates/rdocx/tests/regression_test.rs index c80fa897..9a63c9f6 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -2204,6 +2204,100 @@ mod accepted_run_index_anchoring { } } +/// GitHub issue #163: a comment reaches a paragraph inside a block +/// content control through a two-segment story location. +mod block_control_comment_positions { + use rdocx::{ContentLocation, Document, StoryItemKind, StoryRunPosition, StoryRunRange}; + + const BODY: &str = r#"Alpha.Control one.Control two.Beta."#; + + fn comment_on(document: &mut Document, location: &ContentLocation) -> rdocx::Result { + let position = |run_index| StoryRunPosition { + location: location.clone(), + run_index, + }; + document.add_story_comment( + StoryRunRange { + start: position(0), + end: position(1), + }, + "Ada", + None, + "Here", + ) + } + + #[test] + fn story_comment_reaches_a_paragraph_inside_a_block_control() { + let mut document = super::document_with_content_controls(&super::wrap_word_body(BODY)); + assert_eq!(document.paragraph(2).unwrap().text(), "Control two."); + let location = document.paragraph_story_location(2).unwrap().unwrap(); + let control_item = document + .story_item_snapshots() + .unwrap() + .into_iter() + .position(|item| item.location().item_kind() == StoryItemKind::ContentControl) + .unwrap(); + assert_eq!(location.index_path(), [control_item, 1]); + + let id = comment_on(&mut document, &location).unwrap(); + let xml = super::document_xml(&mut document); + let start = xml + .find(&format!(r#""#)) + .unwrap(); + let end = xml + .find(&format!(r#""#)) + .unwrap(); + assert_eq!(super::f_x093_visible_text(&xml[start..end]), "Control two."); + let content = xml.find("").unwrap(); + let content_end = xml.find("").unwrap(); + assert!(content < start && end < content_end, "{xml}"); + + let reopened = Document::from_bytes(&document.to_bytes().unwrap()).unwrap(); + assert_eq!(reopened.comments()[0].id(), id); + assert_eq!( + reopened.content_controls()[0].text(), + "Control one.Control two." + ); + + // A direct paragraph keeps the one-segment path of its story item. + let beta = document.paragraph_story_location(3).unwrap().unwrap(); + assert_eq!(beta.index_path().len(), 1); + let id = comment_on(&mut document, &beta).unwrap(); + let xml = super::document_xml(&mut document); + let start = xml + .find(&format!(r#""#)) + .unwrap(); + let end = xml + .find(&format!(r#""#)) + .unwrap(); + assert_eq!(super::f_x093_visible_text(&xml[start..end]), "Beta."); + assert!(document.paragraph_story_location(4).unwrap().is_none()); + } + + #[test] + fn two_segment_positions_must_name_a_paragraph_of_a_block_control() { + let mut document = super::document_with_content_controls(&super::wrap_word_body(BODY)); + let location = document.paragraph_story_location(1).unwrap().unwrap(); + let alpha = document.paragraph_story_location(0).unwrap().unwrap(); + let before = document.to_bytes().unwrap(); + for (path, reason) in [ + ( + vec![alpha.index_path()[0], 0], + "must start with a block content control", + ), + (vec![location.index_path()[0], 2], "has no paragraph 2"), + (vec![location.index_path()[0], 0, 0], "invalid"), + ] { + let bad = + ContentLocation::new(location.story().clone(), StoryItemKind::Paragraph, path); + let error = comment_on(&mut document, &bad).unwrap_err().to_string(); + assert!(error.to_lowercase().contains(reason), "{error}"); + } + assert_eq!(document.to_bytes().unwrap(), before); + } +} + fn f_x090_cross_part_drawing_package() -> Vec { let mut document = f255_story_document(); document.add_picture(b"body image", "body.png", Length::pt(1.0), Length::pt(1.0)); diff --git a/docs/hld/03-architecture.md b/docs/hld/03-architecture.md index 3def44da..1b474311 100644 --- a/docs/hld/03-architecture.md +++ b/docs/hld/03-architecture.md @@ -1476,7 +1476,11 @@ collision-free comment and paragraph ids, updates the comment parts and all three anchors together, then invalidates layout once. `CommentRef` is a read-only view over the typed comment and its comments-extended thread entry. `StoryRunPosition` and `StoryRunRange` add checked `ContentLocation` ownership -for body and table-cell paragraphs without changing `RunPosition`. The staged +for body and table-cell paragraphs without changing `RunPosition`. A body +location can also name a paragraph inside a block content control with a +two-segment path, the control's story item then the paragraph's position among +its paragraphs, which `Document::paragraph_story_location` returns for a +paragraph index. The staged path validates both endpoints and edits cloned paragraphs before it creates comment relationships, so any path, run, or package failure publishes nothing. Replies follow paragraph-id parent linkage, resolution applies to the thread diff --git a/docs/hld/10-bindings-spec.md b/docs/hld/10-bindings-spec.md index 6e69db10..449d9f49 100644 --- a/docs/hld/10-bindings-spec.md +++ b/docs/hld/10-bindings-spec.md @@ -241,6 +241,11 @@ field cache update operations. `RunPosition` and `RunRange` are constructible frozen values for zero-based half-open run ranges. `StoryRunPosition` and `StoryRunRange` are parallel frozen values whose `StoryItem` snapshots can identify direct body or table-cell paragraphs. +`StoryRunPosition` also accepts a body `Paragraph` handle. A handle to a +paragraph inside a block content control yields an item with the two-segment +path of `Document::paragraph_story_location`, the control's story item index +then the paragraph's position among the control's paragraphs, which only +comment positions accept. `Document.add_comment` accepts either range form. The original direct-body constructors and call shape remain unchanged. `Comment`, `ComparisonDiagnostic`, `BoundingBox`, `LayoutFragment`, @@ -685,7 +690,10 @@ bundled-font page targets and returns `TocRebuildReport` with entry and newly allocated bookmark counts plus exact retained-field diagnostics in physical source order. `diagnostic_count()` is derived from the owned diagnostic collection. A document without a TOC is unchanged and returns empty counts and -diagnostics. `rdocx-cli toc rebuild` publishes the validated result to an +diagnostics. The cached entry paragraphs are replaced, so comment and bookmark +markers on them, including those in a table of contents content control, are +dropped with them, and a comment anchored only there stays unanchored in the +comments part. `rdocx-cli toc rebuild` publishes the validated result to an explicit output and reports the counts through a schema-1 main-story record. Python exposes the same operation and returns diagnostics as an immutable tuple with a derived `diagnostic_count` property. WASM does not expose this operation. From df80360b8ffbfae59a43d1cbfa8bd1f6e1281146 Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 21:44:55 +0200 Subject: [PATCH 7/8] Add add_comment_on_text to comment on a piece of text Anchoring a comment on part of a run took a text search, two split_run calls and run index bookkeeping, which the coordinate mismatches of the run and body indexes made unreliable. Document::add_comment_on_text, and Document.add_comment_on_text in Python, comments on the zero-based occurrence of a literal text in the main story, through tables and block content controls. Matches are case-sensitive, non-overlapping and within one paragraph, over the literal run text that split_run offsets count. The runs at both ends of the match are split with CT_P::split_accepted_literal_span and the comment is anchored like add_comment, so a match inside an inline content control is commented there and one that crosses a control edge is refused. A missing occurrence is an error and the document is unchanged. The comment and thread entry creation shared by the three add paths moves to one helper. The literal text leaves out field results, so a match could span one and the comment then covered more than the anchor. The text between the new markers is read back with CT_P::comment_range_text, and a match whose range would show anything other than the anchor is refused. GitHub issue #163. --- crates/rdocx-oxml/src/text.rs | 103 ++++++++++++++ crates/rdocx-py/python/rdocx/_rdocx.pyi | 10 ++ crates/rdocx-py/src/document.rs | 24 ++++ crates/rdocx-py/tests/test_core.py | 56 ++++++++ crates/rdocx-py/tests/typing_smoke.py | 3 + crates/rdocx/src/comments.rs | 174 +++++++++++++++++------ crates/rdocx/src/document.rs | 5 +- crates/rdocx/tests/regression_test.rs | 175 +++++++++++++++++++++++- docs/hld/10-bindings-spec.md | 16 ++- 9 files changed, 523 insertions(+), 43 deletions(-) diff --git a/crates/rdocx-oxml/src/text.rs b/crates/rdocx-oxml/src/text.rs index d9214b5e..49876926 100644 --- a/crates/rdocx-oxml/src/text.rs +++ b/crates/rdocx-oxml/src/text.rs @@ -4109,6 +4109,109 @@ impl CT_P { } } + /// Return the literal text of the accepted-view runs, the text that + /// split offsets count. Tabs, breaks and other non-text content have no + /// width. + #[doc(hidden)] + pub fn accepted_literal_text(&self) -> String { + accepted_paragraph_runs(self) + .into_iter() + .flat_map(|run| run.content.iter().map(CT_R::literal_text)) + .collect() + } + + /// Return the text of every `t` element, in any namespace, between the + /// range markers of comment `id`, or `None` when this paragraph does not + /// hold both markers. + /// + /// Unlike [`Self::accepted_literal_text`], this includes the text of + /// preserved children that the accepted view leaves out, such as a + /// `w:fldSimple` result, so it is the text the commented range shows. + #[doc(hidden)] + pub fn comment_range_text(&self, id: i32) -> Option { + let mut xml = Vec::new(); + self.to_xml(&mut Writer::new(&mut xml)).ok()?; + let is_marker = |element: &BytesStart<'_>, local: &[u8]| { + matches_local_name(element.name().as_ref(), local) + && element + .attributes() + .flatten() + .find(|attribute| matches_local_name(attribute.key.as_ref(), b"id")) + .and_then(|attribute| std::str::from_utf8(&attribute.value).ok()?.parse().ok()) + == Some(id) + }; + let mut reader = Reader::from_reader(xml.as_slice()); + let mut buffer = Vec::new(); + let mut text = None::; + loop { + match reader.read_event_into(&mut buffer).ok()? { + Event::Start(element) | Event::Empty(element) + if is_marker(&element, b"commentRangeStart") => + { + text = Some(String::new()); + } + Event::Start(element) | Event::Empty(element) + if is_marker(&element, b"commentRangeEnd") => + { + return text; + } + Event::Start(element) if matches_local_name(element.name().as_ref(), b"t") => { + let content = crate::xml_text::read_element_text(&mut reader, element.name()); + if let Some(text) = text.as_mut() { + text.push_str(&content); + } + } + Event::Eof => return None, + _ => {} + } + buffer.clear(); + } + } + + /// Split accepted-view runs so that the non-empty literal text span + /// `[start, end)`, in Unicode scalar values of + /// [`Self::accepted_literal_text`], covers whole runs, and return the run + /// boundaries around it. + #[doc(hidden)] + pub fn split_accepted_literal_span( + &mut self, + start: usize, + end: usize, + ) -> Result<(usize, usize)> { + let lengths = accepted_paragraph_runs(self) + .into_iter() + .map(CT_R::literal_len) + .collect::>(); + // The run holding one literal character and its offset in that run. + let locate = |character: usize| { + let mut run_start = 0; + lengths.iter().enumerate().find_map(|(index, len)| { + let found = (character < run_start + len).then_some((index, character - run_start)); + run_start += len; + found + }) + }; + let outside = || { + OxmlError::InvalidValue(format!( + "literal span {start}..{end} is not inside the paragraph text" + )) + }; + let ((start_run, start_offset), (end_run, end_offset)) = match end.checked_sub(1) { + Some(last) if start < end => ( + locate(start).ok_or_else(outside)?, + locate(last).ok_or_else(outside)?, + ), + _ => return Err(outside()), + }; + // The end goes first, so the start run keeps its index. + let path = self.accepted_run_paths()[end_run].clone(); + let end_boundary = self.split_accepted_run(&path, end_run, end_offset + 1)?; + let path = self.accepted_run_paths()[start_run].clone(); + let start_boundary = self.split_accepted_run(&path, start_run, start_offset)?; + // A split inside the start run adds one run before the end boundary. + Ok((start_boundary, end_boundary + usize::from(start_offset > 0))) + } + pub(crate) fn split_accepted_run_segments( &mut self, path: &[AcceptedRunPathSegment], diff --git a/crates/rdocx-py/python/rdocx/_rdocx.pyi b/crates/rdocx-py/python/rdocx/_rdocx.pyi index 8aa11dc8..b23f18fb 100644 --- a/crates/rdocx-py/python/rdocx/_rdocx.pyi +++ b/crates/rdocx-py/python/rdocx/_rdocx.pyi @@ -479,6 +479,16 @@ class Document: initials: str | None = None, date: str | None = None, ) -> int: ... + def add_comment_on_text( + self, + anchor: str, + *, + author: str, + text: str, + occurrence: int = 0, + initials: str | None = None, + date: str | None = None, + ) -> int: ... def reply_to( self, parent_id: int, *, author: str, text: str, date: str | None = None ) -> int: ... diff --git a/crates/rdocx-py/src/document.rs b/crates/rdocx-py/src/document.rs index 6e6a313f..989a114c 100644 --- a/crates/rdocx-py/src/document.rs +++ b/crates/rdocx-py/src/document.rs @@ -1636,6 +1636,30 @@ impl PyDocument { Ok(id) } + /// Comment on the `occurrence`-th match of `anchor`, counted from zero, + /// in the main story. Matching is case-sensitive, non-overlapping and + /// within one paragraph. A match whose range would also show other text, + /// such as a field result, raises. + #[pyo3(signature = (anchor, *, author, text, occurrence = 0, initials = None, date = None))] + #[allow(clippy::too_many_arguments)] + fn add_comment_on_text( + &mut self, + anchor: &str, + author: &str, + text: &str, + occurrence: usize, + initials: Option<&str>, + date: Option<&str>, + py: Python<'_>, + ) -> PyResult { + let id = self + .inner + .add_comment_on_text(anchor, occurrence, author, initials, text, date) + .map_err(|error| rdocx_to_pyerr(py, error))?; + self.revisions.bump(); + Ok(id) + } + #[pyo3(signature = (parent_id, *, author, text, date = None))] fn reply_to( &mut self, diff --git a/crates/rdocx-py/tests/test_core.py b/crates/rdocx-py/tests/test_core.py index ec2b2090..da0f7ab9 100644 --- a/crates/rdocx-py/tests/test_core.py +++ b/crates/rdocx-py/tests/test_core.py @@ -1568,6 +1568,62 @@ def test_story_run_position_takes_a_paragraph_handle_inside_a_block_control(): rdocx.StoryRunPosition(run_index=0) +def test_add_comment_on_text_anchors_the_occurrence_after_a_table(): + import rdocx + + # GitHub issue #163 asks for a helper that comments on a piece of text. + # This is the fixture of the issue. + def fixture(): + document = rdocx.Document() + document.add_paragraph("Alpha paragraph before the table.") + document.add_table(1, 1).cell(0, 0).text = "cell" + document.add_paragraph("Beta paragraph after the table.") + document.add_paragraph("Gamma paragraph at the end.") + return document + + document = fixture() + held = document.paragraphs[1] + comment_id = document.add_comment_on_text("Beta", author="Ada", text="Here") + assert _anchored_texts(document, comment_id) == ["Beta"] + assert [run.text for run in document.paragraphs[1].runs if run.text] == [ + "Beta", + " paragraph after the table.", + ] + with pytest.raises(rdocx.StaleElementError): + held.text + + document = fixture() + comment_id = document.add_comment_on_text( + "paragraph", + author="Ada", + text="Second", + occurrence=1, + initials="AL", + date="2026-09-27T10:00:00Z", + ) + assert _anchored_texts(document, comment_id) == ["paragraph"] + assert [run.text for run in document.paragraphs[1].runs if run.text] == [ + "Beta ", + "paragraph", + " after the table.", + ] + reopened = rdocx.Document.from_bytes(document.to_bytes()) + [comment] = reopened.comments + assert (comment.text, comment.initials, comment.date) == ( + "Second", + "AL", + "2026-09-27T10:00:00Z", + ) + + before = document.to_bytes() + for anchor, occurrence in (("beta", 0), ("paragraph", 3), ("", 0)): + with pytest.raises(rdocx.RdocxError): + document.add_comment_on_text( + anchor, author="Ada", text="x", occurrence=occurrence + ) + assert document.to_bytes() == before + + def test_python_round_three_authoring_and_inspection_is_typed_and_lossless(): import rdocx diff --git a/crates/rdocx-py/tests/typing_smoke.py b/crates/rdocx-py/tests/typing_smoke.py index 2165f877..8ddd610f 100644 --- a/crates/rdocx-py/tests/typing_smoke.py +++ b/crates/rdocx-py/tests/typing_smoke.py @@ -90,6 +90,9 @@ def exercise_rdocx_types(path: Path) -> None: initials=None, date="2026-09-16T10:15:30Z", ) + text_comment_id: int = document.add_comment_on_text( + "review", author="Ada", text="here", occurrence=0, initials=None, date=None + ) reply_id: int = document.reply_to( comment_id, author="Grace", diff --git a/crates/rdocx/src/comments.rs b/crates/rdocx/src/comments.rs index 9b5d5014..5a6dea03 100644 --- a/crates/rdocx/src/comments.rs +++ b/crates/rdocx/src/comments.rs @@ -13,6 +13,7 @@ use rdocx_oxml::text::{CT_P, RangeAnchor}; #[cfg(test)] use rdocx_oxml::text::{CT_R, CommentRangeMarker, HyperlinkSpan, RunContent}; +use crate::document::visit_body_paragraphs_mut; use crate::{ContentLocation, Document, Error, Result}; pub(crate) const COMMENTS_EXTENDED_REL_TYPE: &str = @@ -643,33 +644,7 @@ impl Document { self.ensure_comment_models()?; self.ensure_comment_relationships()?; - let para_id = allocate_para_id(self.comments.as_ref(), self.comments_extended.as_ref())?; - let mut comment_paragraph = CT_P::new(); - comment_paragraph.add_run(text); - self.comments - .as_mut() - .expect("comment model was initialized") - .comments - .push(CT_Comment { - id, - author: Some(author.to_owned()), - date: date.map(str::to_owned), - initials: initials.map(str::to_owned), - paragraphs: vec![comment_paragraph], - paragraph_ids: vec![Some(para_id.clone())], - extra_attributes: Vec::new(), - extra_xml: Vec::new(), - }); - self.comments_extended - .as_mut() - .expect("comments-extended model was initialized") - .comments - .push(CT_CommentEx { - para_id, - para_id_parent: None, - done: None, - extra_attributes: Vec::new(), - }); + self.push_comment_definition(id, author, initials, text, date)?; self.identifiers = identifiers; self.comments_dirty = true; self.invalidate_layout(); @@ -690,8 +665,139 @@ impl Document { self.ensure_comment_relationships()?; let mut identifiers = self.identifiers.clone(); let id = identifiers.reserve_comment_id()?; - let para_id = allocate_para_id(self.comments.as_ref(), self.comments_extended.as_ref())?; + self.push_comment_definition(id, author, initials, text, date)?; + anchor_body_range( + &mut self.document.body.content, + range, + RangeAnchor::Comment(id), + "comment", + )?; + self.identifiers = identifiers; + self.comments_dirty = true; + self.invalidate_layout(); + Ok(id) + } + + /// Add a comment on the `occurrence`-th match of `anchor`, counted from + /// zero, in the main story. + /// + /// Matches are case-sensitive and non-overlapping, in document order + /// through body paragraphs, tables and block content controls, and one + /// match never spans two paragraphs. They are found in the literal run + /// text that [`Document::split_run`] offsets count, so tabs and breaks + /// have no width. The runs at both ends of the match are split and the + /// comment is anchored on the runs between the splits as + /// [`Self::add_comment`] anchors a run range. `date`, when present, must + /// be an RFC 3339 timestamp. A missing occurrence or a match that cannot + /// be anchored exactly is an error and leaves the document unchanged. A + /// match is not exact when its range would also show text that the + /// literal text leaves out, such as the result of a field between two of + /// its runs. + pub fn add_comment_on_text( + &mut self, + anchor: &str, + occurrence: usize, + author: &str, + initials: Option<&str>, + text: &str, + date: Option<&str>, + ) -> Result { + let mut candidate = self.clone_for_staging(); + let id = candidate + .add_comment_on_text_staged(anchor, occurrence, author, initials, text, date)?; + candidate.flush_dirty_related_story_models()?; + self.commit_staged_mutation(candidate); + Ok(id) + } + fn add_comment_on_text_staged( + &mut self, + anchor: &str, + occurrence: usize, + author: &str, + initials: Option<&str>, + text: &str, + date: Option<&str>, + ) -> Result { + validate_comment_date(date)?; + if anchor.is_empty() { + return Err(Error::Other( + "comment anchor text must not be empty".to_owned(), + )); + } + self.ensure_comment_models()?; + self.ensure_comment_relationships()?; + let mut identifiers = self.identifiers.clone(); + let id = identifiers.reserve_comment_id()?; + let mut remaining = occurrence; + let mut anchored = None; + visit_body_paragraphs_mut(&mut self.document.body.content, &mut |paragraph| { + if anchored.is_some() { + return; + } + let literal = paragraph.accepted_literal_text(); + let matches = literal + .match_indices(anchor) + .map(|(byte, _)| byte) + .collect::>(); + let Some(byte) = matches.get(remaining) else { + remaining -= matches.len(); + return; + }; + let start = literal[..*byte].chars().count(); + let end = start + anchor.chars().count(); + anchored = Some( + paragraph + .split_accepted_literal_span(start, end) + .map_err(|error| { + Error::Other(format!("comment anchor text cannot be split: {error}")) + }) + .and_then(|(start, end)| { + anchor_paragraph_range( + paragraph, + Some(start), + Some(end), + RangeAnchor::Comment(id), + "comment", + ) + }) + .and_then(|()| { + // Preserved children such as `w:fldSimple` are not in + // the literal text, but the range shows their text. + let shown = paragraph.comment_range_text(id).unwrap_or_default(); + if shown == anchor { + Ok(()) + } else { + Err(Error::Other(format!( + "comment anchor text {anchor:?} occurrence {occurrence} cannot be anchored exactly: its range would show {shown:?}" + ))) + } + }), + ); + }); + anchored.ok_or_else(|| { + Error::Other(format!( + "comment anchor text {anchor:?} has no occurrence {occurrence}" + )) + })??; + self.push_comment_definition(id, author, initials, text, date)?; + self.identifiers = identifiers; + self.comments_dirty = true; + self.invalidate_layout(); + Ok(id) + } + + /// Append comment `id`, holding one text paragraph, and its thread entry + /// to the comment models, which must already exist. + fn push_comment_definition( + &mut self, + id: i32, + author: &str, + initials: Option<&str>, + text: &str, + date: Option<&str>, + ) -> Result<()> { + let para_id = allocate_para_id(self.comments.as_ref(), self.comments_extended.as_ref())?; let mut paragraph = CT_P::new(); paragraph.add_run(text); self.comments @@ -718,17 +824,7 @@ impl Document { done: None, extra_attributes: Vec::new(), }); - - anchor_body_range( - &mut self.document.body.content, - range, - RangeAnchor::Comment(id), - "comment", - )?; - self.identifiers = identifiers; - self.comments_dirty = true; - self.invalidate_layout(); - Ok(id) + Ok(()) } /// Add a reply linked to the selected comment paragraph. diff --git a/crates/rdocx/src/document.rs b/crates/rdocx/src/document.rs index 2feda35e..21ac80dd 100644 --- a/crates/rdocx/src/document.rs +++ b/crates/rdocx/src/document.rs @@ -9389,7 +9389,10 @@ fn visit_sdt(control: &CT_Sdt, visitor: &mut impl FnMut(&CT_P)) { } } -fn visit_body_paragraphs_mut(content: &mut [BodyContent], visitor: &mut impl FnMut(&mut CT_P)) { +pub(crate) fn visit_body_paragraphs_mut( + content: &mut [BodyContent], + visitor: &mut impl FnMut(&mut CT_P), +) { for item in content { match item { BodyContent::Paragraph(paragraph) => visit_paragraph_mut(paragraph, visitor), diff --git a/crates/rdocx/tests/regression_test.rs b/crates/rdocx/tests/regression_test.rs index 9a63c9f6..074a3e96 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -1914,7 +1914,7 @@ mod accepted_run_index_anchoring { } /// The `w:t` text of an XML slice that may cross element boundaries. - fn slice_text(xml: &str) -> String { + pub(super) fn slice_text(xml: &str) -> String { let mut text = String::new(); let mut rest = xml; while let Some(index) = rest.find(" Document { + let mut document = Document::new(); + document.add_paragraph("Alpha paragraph before the table."); + document + .add_table(1, 1) + .row(0) + .unwrap() + .cell(0) + .unwrap() + .set_text("cell"); + document.add_paragraph("Beta paragraph after the table."); + document.add_paragraph("Gamma paragraph at the end."); + document + } + + /// The paragraph text around the markers of comment `id`, split as + /// before, inside and after the range. + fn anchored(document: &mut Document, id: i32) -> [String; 3] { + let xml = super::document_xml(document); + let start = xml + .find(&format!(r#""#)) + .unwrap(); + let end = xml + .find(&format!(r#""#)) + .unwrap(); + let paragraph_start = xml[..start].rfind("").unwrap(); + let paragraph_end = end + xml[end..].find("").unwrap(); + [ + super::accepted_run_index_anchoring::slice_text(&xml[paragraph_start..start]), + super::accepted_run_index_anchoring::slice_text(&xml[start..end]), + super::accepted_run_index_anchoring::slice_text(&xml[end..paragraph_end]), + ] + } + + fn comment(document: &mut Document, anchor: &str, occurrence: usize) -> rdocx::Result { + document.add_comment_on_text(anchor, occurrence, "Ada", None, "Here", None) + } + + #[test] + fn comment_on_text_anchors_the_requested_occurrence_after_a_table() { + let mut document = issue_fixture(); + let id = comment(&mut document, "Beta", 0).unwrap(); + assert_eq!( + anchored(&mut document, id), + ["", "Beta", " paragraph after the table."] + ); + + // Zero-based and in document order: Alpha, then Beta. + let mut document = issue_fixture(); + let id = comment(&mut document, "paragraph", 1).unwrap(); + assert_eq!( + anchored(&mut document, id), + ["Beta ", "paragraph", " after the table."] + ); + let reopened = Document::from_bytes(&document.to_bytes().unwrap()).unwrap(); + assert_eq!(reopened.comments()[0].text(), "Here"); + assert_eq!( + reopened.paragraphs()[1].text(), + "Beta paragraph after the table." + ); + + // Matches do not overlap, and a table cell is searched too. + let mut document = issue_fixture(); + document.add_paragraph("aaaa"); + let id = comment(&mut document, "aa", 1).unwrap(); + assert_eq!(anchored(&mut document, id), ["aa", "aa", ""]); + let id = comment(&mut document, "cell", 0).unwrap(); + assert_eq!(anchored(&mut document, id), ["", "cell", ""]); + } + + #[test] + fn comment_on_text_splits_runs_and_keeps_their_formatting() { + let mut document = Document::new(); + let mut paragraph = document.add_paragraph(""); + paragraph.add_run("Hello ").bold(true); + paragraph.add_run("world").italic(true); + let id = comment(&mut document, "lo wo", 0).unwrap(); + assert_eq!(anchored(&mut document, id), ["Hel", "lo wo", "rld"]); + let paragraph = document.paragraph(0).unwrap(); + let runs = paragraph + .runs() + .filter(|run| !run.text().is_empty()) + .map(|run| (run.text(), run.is_bold(), run.is_italic())) + .collect::>(); + assert_eq!( + runs, + [ + ("Hel".to_owned(), true, false), + ("lo ".to_owned(), true, false), + ("wo".to_owned(), false, true), + ("rld".to_owned(), false, true), + ] + ); + } + + #[test] + fn comment_on_text_refuses_a_range_that_would_show_a_field_result() { + // The literal text `AB after tail xy` leaves out the tab, the + // `w:fldSimple` result `7` and the complex field result `9`. + let mut document = super::document_with_content_controls(&super::wrap_word_body( + r#"AB after7 tail x NUMPAGES 9y"#, + )); + let before = document.to_bytes().unwrap(); + for (anchor, reason) in [ + ( + "after tail", + r#"cannot be anchored exactly: its range would show "after7 tail""#, + ), + ( + "xy", + r#"cannot be anchored exactly: its range would show "x9y""#, + ), + ("7", "has no occurrence 0"), + ("x9y", "has no occurrence 0"), + ] { + let error = comment(&mut document, anchor, 0).unwrap_err(); + assert!(error.to_string().contains(reason), "{anchor}: {error}"); + } + assert_eq!(document.to_bytes().unwrap(), before); + + // A tab has no width, and a range next to a field leaves it out. + let id = comment(&mut document, "AB", 0).unwrap(); + assert_eq!(anchored(&mut document, id), ["", "AB", " after7 tail x9y"]); + let id = comment(&mut document, " after", 0).unwrap(); + assert_eq!(anchored(&mut document, id)[1], " after"); + let id = comment(&mut document, " tail x", 0).unwrap(); + assert_eq!(anchored(&mut document, id)[1], " tail x"); + } + + #[test] + fn comment_on_text_reaches_controls_and_refuses_what_it_cannot_anchor() { + let mut document = super::document_with_content_controls(&super::wrap_word_body( + r#"before TARGETInside a block control."#, + )); + let before = document.to_bytes().unwrap(); + for (anchor, occurrence, reason) in [ + ("missing", 0, "has no occurrence 0"), + ("target", 0, "has no occurrence 0"), + ("TARGET", 1, "has no occurrence 1"), + ("", 0, "must not be empty"), + ("e TAR", 0, "crosses the edge of an inline content control"), + ] { + let error = comment(&mut document, anchor, occurrence).unwrap_err(); + assert!(error.to_string().contains(reason), "{anchor}: {error}"); + } + assert_eq!(document.to_bytes().unwrap(), before); + + let id = comment(&mut document, "ARG", 0).unwrap(); + let xml = super::document_xml(&mut document); + let start = xml.find("").unwrap() < start, "{xml}"); + assert_eq!(anchored(&mut document, id)[1], "ARG"); + let id = comment(&mut document, "block", 0).unwrap(); + assert_eq!( + anchored(&mut document, id), + ["Inside a ", "block", " control."] + ); + } +} + fn f_x090_cross_part_drawing_package() -> Vec { let mut document = f255_story_document(); document.add_picture(b"body image", "body.png", Length::pt(1.0), Length::pt(1.0)); diff --git a/docs/hld/10-bindings-spec.md b/docs/hld/10-bindings-spec.md index 449d9f49..7970b190 100644 --- a/docs/hld/10-bindings-spec.md +++ b/docs/hld/10-bindings-spec.md @@ -246,8 +246,10 @@ paragraph inside a block content control yields an item with the two-segment path of `Document::paragraph_story_location`, the control's story item index then the paragraph's position among the control's paragraphs, which only comment positions accept. -`Document.add_comment` accepts either range form. The original direct-body -constructors and call shape remain unchanged. +`Document.add_comment` accepts either range form. +`Document.add_comment_on_text` comments on the zero-based occurrence of an +exact text in the main story without run index bookkeeping. The original +direct-body constructors and call shape remain unchanged. `Comment`, `ComparisonDiagnostic`, `BoundingBox`, `LayoutFragment`, `LayoutPage`, `TocRebuildReport`, and `Revision` are frozen typed snapshots. `Document.revisions` lists main-document revisions with a snake_case `kind`, @@ -1007,6 +1009,16 @@ Native Word callers can inspect comments through `Document::comments` and author threads through `add_comment`, `reply_to`, `resolve_comment`, and `remove_comment`. The additive native `add_comment_with_date` and `reply_to_with_date` methods accept an optional validated RFC 3339 timestamp. +The additive native `add_comment_on_text` anchors a comment on the zero-based, +non-overlapping, case-sensitive occurrence of a literal text in main-story +paragraphs, through tables and block content controls. It splits the runs at +both ends of the match, anchors the runs between the splits like +`add_comment`, and refuses a missing occurrence or a match that cannot be +anchored exactly without changing the document. Tabs and breaks have no width +in the literal text, and a match whose range would also show text that the +literal text leaves out, such as a field result, is not exact. Python exposes +it with keyword `author`, `text`, `occurrence`, `initials` and `date` +arguments. The Python `add_comment` and `reply_to` methods expose the same value as the optional `date` keyword. Omission writes no date and remains deterministic. Returned ids keep naming the same comment or reply after rdocx save and reopen, From 573d8c670a5e7d6504ace27c39cb81fd584f4a40 Mon Sep 17 00:00:00 2001 From: Hadrien Mary Date: Sun, 27 Sep 2026 21:51:47 +0200 Subject: [PATCH 8/8] Re-record the archive measurements of rdocx, rdocx-oxml and rdocx-cli The accepted-view anchoring, the block content control comment path and add_comment_on_text change the sources, tests and README of these three published crates, so their crates.io archive rows in scripts/readme_doctests.py and the crate READMEs are measured again on top of the stacked tree. The measurement dates are left as recorded. GitHub issues #172 and #163. --- README.md | 2 +- crates/rdocx-cli/README.md | 2 +- crates/rdocx-oxml/README.md | 2 +- scripts/readme_doctests.py | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f0ac3418..02999302 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ rows are the enforced release-mode bounds plus one dated observation. | Measurement | Value | Version | Platform | Build mode | Input | Command | Statistic | Measured on | |---|---|---|---|---|---|---|---|---| -| Crates.io archive: rdocx | 1,095,450 compressed bytes, 6,512,553 member bytes, 36 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-26 | +| Crates.io archive: rdocx | 1,102,999 compressed bytes, 6,548,573 member bytes, 36 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-26 | | Large-document layout throughput | minimum 250 pages/s, observed 31,019.1 pages/s | rdocx 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | release, one test thread | 1,000 one-page paragraphs with deterministic fonts | `cargo test -p rdocx --test regression_test --release a_thousand_page_document_paginates_and_renders_within_the_declared_limits -- --ignored --exact --nocapture --test-threads=1` | pages per wall-clock second | 2026-09-19 | | Large-document layout peak allocation | maximum 64 MiB, observed 29.03 MiB | rdocx 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | release, one test thread | 1,000 one-page paragraphs with deterministic fonts | `cargo test -p rdocx --test regression_test --release a_thousand_page_document_paginates_and_renders_within_the_declared_limits -- --ignored --exact --nocapture --test-threads=1` | peak live allocation | 2026-09-19 | | Large-document PDF throughput | minimum 1,000 pages/s, observed 60,058.0 pages/s | rdocx 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | release, one test thread | 1,000 deterministic layout pages | `cargo test -p rdocx --test regression_test --release a_thousand_page_document_paginates_and_renders_within_the_declared_limits -- --ignored --exact --nocapture --test-threads=1` | pages per wall-clock second | 2026-09-19 | diff --git a/crates/rdocx-cli/README.md b/crates/rdocx-cli/README.md index 61d4da98..6028886a 100644 --- a/crates/rdocx-cli/README.md +++ b/crates/rdocx-cli/README.md @@ -22,7 +22,7 @@ and produces fixed or flow output without an Office host. | Measurement | Value | Version | Platform | Build mode | Input | Command | Statistic | Measured on | |---|---|---|---|---|---|---|---|---| -| Crates.io archive: rdocx-cli | 33,805 compressed bytes, 145,256 member bytes, 8 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx-cli` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-19 | +| Crates.io archive: rdocx-cli | 34,584 compressed bytes, 148,741 member bytes, 8 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx-cli` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-19 | ## Use it when diff --git a/crates/rdocx-oxml/README.md b/crates/rdocx-oxml/README.md index 11fef241..fc67c546 100644 --- a/crates/rdocx-oxml/README.md +++ b/crates/rdocx-oxml/README.md @@ -17,7 +17,7 @@ schema order, and retains unmodelled XML alongside typed edits. | Measurement | Value | Version | Platform | Build mode | Input | Command | Statistic | Measured on | |---|---|---|---|---|---|---|---|---| -| Crates.io archive: rdocx-oxml | 367,500 compressed bytes, 2,380,047 member bytes, 32 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx-oxml` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-19 | +| Crates.io archive: rdocx-oxml | 374,656 compressed bytes, 2,413,911 member bytes, 32 members | 0.14.0 | macOS 26.6.2, Apple M5 Max, arm64 | `cargo package --locked --no-verify` | Tracked `rdocx-oxml` package inventory | `python3 scripts/readme_doctests.py --record-measurements` | gzip archive bytes, tar member bytes, tar member count | 2026-09-19 | ## Use it when diff --git a/scripts/readme_doctests.py b/scripts/readme_doctests.py index bd8c280f..dd3536e2 100644 --- a/scripts/readme_doctests.py +++ b/scripts/readme_doctests.py @@ -383,12 +383,12 @@ class ReadmeCase: "oxml-opc": (92_122, 355_510, 12), "oxml-pdf": (66_015, 304_432, 14), "oxml-sml": (12_511, 49_803, 6), - "rdocx": (1_095_450, 6_512_553, 36), - "rdocx-cli": (33_805, 145_256, 8), + "rdocx": (1_102_999, 6_548_573, 36), + "rdocx-cli": (34_584, 148_741, 8), "rdocx-html": (15_486, 63_894, 11), "rdocx-layout": (255_752, 1_385_701, 15), "rdocx-opc": (3_655, 9_668, 6), - "rdocx-oxml": (367_500, 2_380_047, 32), + "rdocx-oxml": (374_656, 2_413_911, 32), "rdocx-pdf": (8_111, 26_758, 6), "rpptx": (407_658, 2_122_094, 16), "rpptx-chart": (6_648, 21_136, 6),