diff --git a/README.md b/README.md index 479b462e..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,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,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 25ecc0fe..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 @@ -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/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/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..49876926 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)] @@ -4057,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], @@ -4569,6 +4724,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 +5223,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 +5238,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 +8040,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/python/rdocx/_rdocx.pyi b/crates/rdocx-py/python/rdocx/_rdocx.pyi index f5e4ef18..b23f18fb 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 @@ -428,7 +435,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: ... @@ -472,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 b81167c6..989a114c 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}; @@ -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<'_>, @@ -979,6 +1033,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 +1273,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) } @@ -1525,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/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 0f8ce927..da0f7ab9 100644 --- a/crates/rdocx-py/tests/test_core.py +++ b/crates/rdocx-py/tests/test_core.py @@ -1291,6 +1291,339 @@ 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_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." + ] + + +# 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_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_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 51ebfa05..8ddd610f 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[:] @@ -89,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", @@ -107,7 +111,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 1e85cd82..5a6dea03 100644 --- a/crates/rdocx/src/comments.rs +++ b/crates/rdocx/src/comments.rs @@ -8,11 +8,12 @@ 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::document::visit_body_paragraphs_mut; use crate::{ContentLocation, Document, Error, Result}; pub(crate) const COMMENTS_EXTENDED_REL_TYPE: &str = @@ -27,9 +28,12 @@ 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. + /// 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, } @@ -60,6 +64,7 @@ pub struct BookmarkRef { id: Option, name: Option, range: Option, + direct_range: Option, text: String, issue: Option, } @@ -74,10 +79,27 @@ 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`], which are the + /// `RunPosition` run indexes that `Document::add_bookmark` and + /// `Document::add_comment` take. + pub fn direct_range(&self) -> Option { + self.direct_range + } + pub fn text(&self) -> &str { &self.text } @@ -256,10 +278,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, @@ -267,14 +292,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), @@ -294,6 +333,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()), }, @@ -343,6 +383,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( @@ -360,6 +406,7 @@ impl Document { id: Some(id), name, range, + direct_range, text, issue, }, @@ -385,6 +432,7 @@ impl Document { bookmark.name.as_deref().unwrap_or("") )); bookmark.range = None; + bookmark.direct_range = None; bookmark.text.clear(); } } @@ -393,6 +441,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) } @@ -409,39 +464,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) @@ -481,6 +509,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, @@ -511,6 +544,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, @@ -526,7 +564,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, @@ -553,17 +592,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() ))); } } @@ -577,70 +615,36 @@ 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; } 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(); @@ -661,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 @@ -689,30 +824,7 @@ impl Document { done: None, 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, - }); - self.identifiers = identifiers; - self.comments_dirty = true; - self.invalidate_layout(); - Ok(id) + Ok(()) } /// Add a reply linked to the selected comment paragraph. @@ -1000,11 +1112,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() ))); } } @@ -1196,11 +1308,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() ))); } } @@ -1261,6 +1373,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 { @@ -1461,32 +1621,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(); @@ -1632,15 +1766,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(_) => {} } } } @@ -1653,11 +1788,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( @@ -2103,9 +2233,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/document.rs b/crates/rdocx/src/document.rs index 5c4b2b46..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), @@ -13088,18 +13091,26 @@ 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, 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,22 +13120,40 @@ 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, } .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, ordinal, cell_route) }; if part_name != self.doc_part_name { return Err(Error::Other( @@ -13133,8 +13162,18 @@ 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())) + 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(|| { @@ -13154,7 +13193,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( @@ -14529,7 +14574,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 +14708,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 +14720,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 +14739,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) } @@ -15034,6 +15107,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/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 d143b3c7..074a3e96 100644 --- a/crates/rdocx/tests/regression_test.rs +++ b/crates/rdocx/tests/regression_test.rs @@ -1634,6 +1634,843 @@ 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, StoryItemKind, StoryKind, StoryRunPosition, StoryRunRange, + }; + + 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); + } + + #[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 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."); + } +} + +/// 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. + pub(super) 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 "); + + // A comment on text inside the control is removed as cleanly. + let on_text = document + .add_comment_on_text("B", 0, "Ada", None, "Here", None) + .unwrap(); + assert_eq!(marker_counts(&super::document_xml(&mut document)), [2; 3]); + assert!(document.remove_comment(on_text).unwrap()); + assert_eq!(marker_counts(&super::document_xml(&mut document)), [1; 3]); + } + + #[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)))); + } + } +} + +/// 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); + } +} + +/// GitHub issue #163: `add_comment_on_text` anchors a comment on a +/// piece of text without index bookkeeping. +mod comment_on_text { + use rdocx::Document; + + fn issue_fixture() -> 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)); @@ -15169,30 +16006,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("