From 2191f5f87f347929ea02a025dbf071a0ffe97998 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Tue, 14 Jul 2026 11:59:00 +0200 Subject: [PATCH 1/2] fix: unify occurrence search through text maps (ISSUES.md #21) Document-wide replace/delete/insert used an element-level w:t search (_get_nth_match) with a text-map fallback, and the two counted occurrences over different sequences: a match split across fragmented runs was invisible to the element-level path, so occurrence=N could target different matches depending on which path won. Route all three methods through a single _find_document_wide text-map lookup (the same sequence count_matches uses), delete the duplicate single-node edit bodies and the drift shim (~250 lines), and rewrite _insert_near_match to split the edge w:t at the match boundary, fixing mid-node anchor placement for the paragraph-scoped and batch_edit insert paths as well ("HelloXX world", not "Hello worldXX"). TextNotFoundError.total_occurrences now matches count_matches, and runs with non-text children (w:tab, w:br) keep them during inserts. --- docx_editor/track_changes.py | 339 +++----------------- tests/test_review_fixes.py | 146 ++++++++- tests/test_track_changes.py | 586 +++++------------------------------ 3 files changed, 266 insertions(+), 805 deletions(-) diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index 7cc7da1..b4a6c4f 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -532,48 +532,20 @@ def count_matches(self, text: str) -> int: local_occ += 1 return count - def _get_nth_match(self, text: str, occurrence: int): - """Get the nth occurrence of text (0-indexed). - - Args: - text: Text to search for - occurrence: Which occurrence to get (0 = first, 1 = second, etc.) - - Returns: - The matching w:t element + def _find_document_wide(self, text: str, occurrence: int) -> TextMapMatch: + """Document-wide nth-occurrence lookup via text maps. Raises: - TextNotFoundError: If not enough occurrences exist + TextNotFoundError: If the text is not found or occurrence doesn't + exist; ``total_occurrences`` matches :meth:`count_matches`. """ - matches = self.editor.find_all_nodes(tag="w:t", contains=text) - if not matches: - raise TextNotFoundError(text) - if occurrence >= len(matches): - raise TextNotFoundError( - text, - occurrence=occurrence, - total_occurrences=len(matches), - ) - return matches[occurrence] - - def _find_match_containing_node(self, text: str, elem) -> TextMapMatch | None: - """Find the TextMapMatch for `text` whose positions include `elem`. - - Searches through all paragraphs to find the match that involves the - specific w:t node, avoiding occurrence-index drift between search methods. - """ - elem_id = id(elem) - for paragraph in self.editor.dom.getElementsByTagName("w:p"): - text_map = build_text_map(paragraph) - local_occ = 0 - while True: - match = find_in_text_map(text_map, text, local_occ) - if match is None: - break - if any(id(pos.node) == elem_id for pos in match.positions): - return match - local_occ += 1 - return None + match = self._find_across_boundaries(text, occurrence) + if match is not None: + return match + total = self.count_matches(text) + if total: + raise TextNotFoundError(text, occurrence=occurrence, total_occurrences=total) + raise TextNotFoundError(text) def _find_across_boundaries(self, text: str, occurrence: int = 0) -> TextMapMatch | None: """Find the nth occurrence of text across element boundaries. @@ -626,81 +598,8 @@ def replace_text(self, find: str, replace_with: str, occurrence: int = 0, paragr ) return self._replace_across_nodes(match, replace_with) - # Find the text element containing the search text - try: - elem = self._get_nth_match(find, occurrence) - except TextNotFoundError: - # Fall back to cross-boundary search - match = self._find_across_boundaries(find, occurrence) - if match is None: - raise - return self._replace_across_nodes(match, replace_with) - - # Get the parent run - run = elem.parentNode - while run and run.nodeName != "w:r": - run = run.parentNode - - if not run: - raise RevisionError("Could not find parent w:r element") - - # Get the full text content - full_text = self._get_node_text(elem) - start_idx = full_text.find(find) - - if start_idx == -1: - raise TextNotFoundError(find) - - # If run has multiple w:t children, delegate to cross-boundary path - # to avoid losing sibling w:t nodes when replacing the whole run. - if len(run.getElementsByTagName("w:t")) > 1: - match = self._find_match_containing_node(find, elem) - if match is not None: - return self._replace_across_nodes(match, replace_with) - - # Build replacement XML - before_text = full_text[:start_idx] - after_text = full_text[start_idx + len(find) :] - - # Site A: If inside , edit text in-place (no del/ins wrappers) - if self._find_ancestor(run, "w:ins"): - self._set_node_text(elem, before_text + replace_with + after_text) - _set_xml_space_preserve(elem) - return -1 - - # Preserve run properties if present - rPr_xml = "" - rPr_elems = run.getElementsByTagName("w:rPr") - if rPr_elems: - rPr_xml = rPr_elems[0].toxml() - - # Build the replacement runs - xml_parts = [] - - # Text before the match (unchanged) - if before_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(before_text)}") - - # Deletion of old text - xml_parts.append(f"{rPr_xml}{_escape_xml(find)}") - - # Insertion of new text - xml_parts.append(f"{rPr_xml}{_escape_xml(replace_with)}") - - # Text after the match (unchanged) - if after_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(after_text)}") - - # Replace the original run - new_xml = "".join(xml_parts) - nodes = self.editor.replace_node(run, new_xml) - - # Find the insertion node to get its ID - for node in nodes: - if node.nodeType == node.ELEMENT_NODE and node.tagName == "w:ins": - return int(node.getAttribute("w:id")) - - return -1 + match = self._find_document_wide(find, occurrence) + return self._replace_across_nodes(match, replace_with) def suggest_deletion(self, text: str, occurrence: int = 0, paragraph: str | None = None) -> int: """Mark text as deleted with tracked changes. @@ -729,88 +628,8 @@ def suggest_deletion(self, text: str, occurrence: int = 0, paragraph: str | None ) return self._delete_across_nodes(match) - # Find the text element containing the search text - try: - elem = self._get_nth_match(text, occurrence) - except TextNotFoundError: - # Fall back to cross-boundary search - match = self._find_across_boundaries(text, occurrence) - if match is None: - raise - return self._delete_across_nodes(match) - - # Get the parent run - run = elem.parentNode - while run and run.nodeName != "w:r": - run = run.parentNode - - if not run: - raise RevisionError("Could not find parent w:r element") - - # Get the full text content - full_text = self._get_node_text(elem) - start_idx = full_text.find(text) - - if start_idx == -1: - raise TextNotFoundError(text) - - # If run has multiple w:t children, delegate to cross-boundary path - if len(run.getElementsByTagName("w:t")) > 1: - match = self._find_match_containing_node(text, elem) - if match is not None: - return self._delete_across_nodes(match) - - # Preserve run properties if present - rPr_xml = "" - rPr_elems = run.getElementsByTagName("w:rPr") - if rPr_elems: - rPr_xml = rPr_elems[0].toxml() - - before_text = full_text[:start_idx] - after_text = full_text[start_idx + len(text) :] - - # Site B: If inside , shrink/remove the insertion (no ) - ins_ancestor = self._find_ancestor(run, "w:ins") - if ins_ancestor: - remaining = before_text + after_text - if remaining: - self._set_node_text(elem, remaining) - _set_xml_space_preserve(elem) - else: - # Entire w:t text removed — check if other w:t nodes exist - if len(self._get_wt_nodes_in_ancestor(ins_ancestor)) == 1: - # Sole w:t — remove entire - if ins_ancestor.parentNode: - ins_ancestor.parentNode.removeChild(ins_ancestor) - else: - # Other w:t nodes exist — remove just this w:t (and run if empty) - self._remove_wt_and_maybe_run(elem) - return -1 - - # Build the replacement runs - xml_parts = [] - - # Text before the match (unchanged) - if before_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(before_text)}") - - # Deletion of the target text - xml_parts.append(f"{rPr_xml}{_escape_xml(text)}") - - # Text after the match (unchanged) - if after_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(after_text)}") - - # Replace the original run - new_xml = "".join(xml_parts) - nodes = self.editor.replace_node(run, new_xml) - - # Find the deletion node to get its ID - for node in nodes: - if node.nodeType == node.ELEMENT_NODE and node.tagName == "w:del": - return int(node.getAttribute("w:id")) - - return -1 + match = self._find_document_wide(text, occurrence) + return self._delete_across_nodes(match) def _get_run_info(self, node) -> tuple[Element | None, str]: """Get the parent w:r element and its rPr XML for a w:t node.""" @@ -1478,62 +1297,51 @@ def _insert_text( ) return self._insert_near_match(match, text, position) - # Find the text element containing the anchor text - try: - elem = self._get_nth_match(anchor, occurrence) - except TextNotFoundError: - # Fall back to cross-boundary search - match = self._find_across_boundaries(anchor, occurrence) - if match is None: - raise TextNotFoundError(anchor) from None - return self._insert_near_match(match, text, position) - - # Get the parent run - run = elem.parentNode - while run and run.nodeName != "w:r": - run = run.parentNode - - if not run: - raise RevisionError("Could not find parent w:r element") - - # Preserve run properties if present - rPr_xml = "" - rPr_elems = run.getElementsByTagName("w:rPr") - if rPr_elems: - rPr_xml = rPr_elems[0].toxml() + match = self._find_document_wide(anchor, occurrence) + return self._insert_near_match(match, text, position) - # Site C: If inside , edit text in-place (safe for multi-w:t) - full_text = self._get_node_text(elem) - anchor_idx = full_text.find(anchor) + def _insert_near_match(self, match: TextMapMatch, text: str, position: Literal["before", "after"]) -> int: + """Insert text before/after a match, splitting the edge w:t at the match boundary.""" + positions = match.positions + if not positions: + return -1 - if anchor_idx == -1: - raise TextNotFoundError(anchor) + if position == "after": + edge = positions[-1] + offset = edge.offset_in_node + 1 + else: + edge = positions[0] + offset = edge.offset_in_node - before_text = full_text[:anchor_idx] - after_text = full_text[anchor_idx + len(anchor) :] + run, rPr_xml = self._get_run_info(edge.node) + if not run: + return -1 + # If the edge run is inside , splice text at the exact offset + # (no wrapper, so no nested ) if self._find_ancestor(run, "w:ins"): - if position == "before": - self._set_node_text(elem, before_text + text + anchor + after_text) - else: - self._set_node_text(elem, before_text + anchor + text + after_text) - _set_xml_space_preserve(elem) + node_text = self._get_node_text(edge.node) + self._set_node_text(edge.node, node_text[:offset] + text + node_text[offset:]) + _set_xml_space_preserve(edge.node) return -1 - # Build split runs + insertion, preserving sibling w:t nodes + # Rebuild the edge run: split its w:t at the offset and wrap text in ins_xml = f"{rPr_xml}{_escape_xml(text)}" xml_parts = [] + + # Preserve non-text children (w:tab, w:br, w:drawing, etc.) + non_text_xml = self._get_non_text_children_xml(run) + if non_text_xml: + xml_parts.append(f"{rPr_xml}{non_text_xml}") + for wt in run.getElementsByTagName("w:t"): - if wt is elem: - # Split this w:t around the anchor + if wt is edge.node: + node_text = self._get_node_text(wt) + before_text = node_text[:offset] + after_text = node_text[offset:] if before_text: xml_parts.append(f"{rPr_xml}{_escape_xml(before_text)}") - if position == "before": - xml_parts.append(ins_xml) - xml_parts.append(f"{rPr_xml}{_escape_xml(anchor)}") - else: - xml_parts.append(f"{rPr_xml}{_escape_xml(anchor)}") - xml_parts.append(ins_xml) + xml_parts.append(ins_xml) if after_text: xml_parts.append(f"{rPr_xml}{_escape_xml(after_text)}") else: @@ -1542,56 +1350,7 @@ def _insert_text( if wt_text: xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") - new_xml = "".join(xml_parts) - nodes = self.editor.replace_node(run, new_xml) - - # Find the insertion node to get its ID - for node in nodes: - if node.nodeType == node.ELEMENT_NODE and node.tagName == "w:ins": - return int(node.getAttribute("w:id")) - - return -1 - - def _insert_near_match(self, match: TextMapMatch, text: str, position: Literal["before", "after"]) -> int: - """Insert text before/after a cross-boundary match.""" - positions = match.positions - if not positions: - return -1 - - # Get rPr from first run - first_run, rPr_xml = self._get_run_info(positions[0].node) - - # Sites H/I: If ref run is inside , insert bare (no wrapper) - if position == "after": - last_run, _ = self._get_run_info(positions[-1].node) - if not last_run: - return -1 - ref_run = last_run - else: - if not first_run: - return -1 - ref_run = first_run - - all_inside_ins = all(p.is_inside_ins for p in positions) - if all_inside_ins and self._find_ancestor(ref_run, "w:ins"): - bare_xml = f"{rPr_xml}{_escape_xml(text)}" - if position == "after": - self.editor.insert_after(ref_run, bare_xml) - else: - self.editor.insert_before(ref_run, bare_xml) - return -1 - - ins_xml = f"{rPr_xml}{_escape_xml(text)}" - - # If ref_run is inside a , insert relative to the ancestor - # to avoid nesting inside . - ins_ancestor = self._find_ancestor(ref_run, "w:ins") - insert_ref = ins_ancestor if ins_ancestor else ref_run - - if position == "after": - nodes = self.editor.insert_after(insert_ref, ins_xml) - else: - nodes = self.editor.insert_before(insert_ref, ins_xml) + nodes = self.editor.replace_node(run, "".join(xml_parts)) for node in nodes: if node.nodeType == node.ELEMENT_NODE and node.tagName == "w:ins": diff --git a/tests/test_review_fixes.py b/tests/test_review_fixes.py index 08194c5..3f69806 100644 --- a/tests/test_review_fixes.py +++ b/tests/test_review_fixes.py @@ -4,8 +4,9 @@ import pytest -from docx_editor.track_changes import RevisionManager -from docx_editor.xml_editor import DocxXMLEditor +from docx_editor.exceptions import TextNotFoundError +from docx_editor.track_changes import EditOperation, RevisionManager +from docx_editor.xml_editor import DocxXMLEditor, build_text_map, compute_paragraph_hash NS = 'xmlns:w="http://schemas.openxmlformats.org/wordprocessingml/2006/main"' @@ -144,9 +145,9 @@ class TestOccurrenceIndexDrift: def test_replace_second_occurrence_in_multi_wt_run(self, temp_xml): # Two paragraphs: first has "cat" in a single w:t, second has "cat" in a - # multi-w:t run (so the fallback triggers). We ask for occurrence=1 (second). - # Bug: _find_across_boundaries with occurrence=1 could return a different - # match than what _get_nth_match found, because the two methods count differently. + # multi-w:t run. We ask for occurrence=1 (second). Historical bug: the + # element-level and text-map searches counted occurrences over different + # sequences, so occurrence=1 could land on a different match. xml_path = temp_xml("catcat dog") mgr = _make_manager(xml_path) mgr.replace_text("cat", "tiger", occurrence=1) @@ -250,10 +251,10 @@ def test_delete_preserves_leading_space_in_after_text(self, temp_xml): class TestOccurrenceFallbackConsistency: - """Initial fallback from _get_nth_match to _find_across_boundaries must find the right match.""" + """Cross-boundary occurrence indexing must be consistent across paragraphs.""" def test_replace_cross_boundary_second_occurrence(self, temp_xml): - # First paragraph: "catdog" split across two w:t (not findable by _get_nth_match) + # First paragraph: "catdog" split across two w:t (invisible to element-level search) # Second paragraph: "catdog" also split across two w:t # Asking for occurrence=1 should replace the SECOND "catdog", not the first. xml_path = temp_xml( @@ -371,3 +372,134 @@ def test_delete_preserves_tab_in_run(self, temp_xml): mgr.suggest_deletion("MATCHEND") tabs = mgr.editor.dom.getElementsByTagName("w:tab") assert tabs.length > 0, "w:tab should be preserved during deletion rebuild" + + +def _paragraph_texts(mgr) -> list[str]: + """Accepted-view visible text of each paragraph (insertions in, deletions out).""" + return [build_text_map(p).text for p in mgr.editor.dom.getElementsByTagName("w:p")] + + +def _pref(mgr, index: int) -> str: + """Build a hash-anchored paragraph ref like 'P1#a7b2' for the nth (1-based) paragraph.""" + p = mgr.editor.dom.getElementsByTagName("w:p")[index - 1] + return f"P{index}#{compute_paragraph_hash(p)}" + + +class TestDocumentWideOccurrenceStability: + """Document-wide occurrence must count text-map matches, not whole w:t nodes (ISSUES.md #21). + + Occurrence 0 of "target" spans two runs in P1; occurrence 1 is a single w:t + in P2. The element-level search saw only P2, so occurrence=0 edited P2. + """ + + BODY = "targettarget" + + def test_replace_occurrence_0_edits_split_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.replace_text("target", "X", occurrence=0) + assert _paragraph_texts(mgr) == ["X", "target"] + + def test_replace_occurrence_1_edits_single_node_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.replace_text("target", "X", occurrence=1) + assert _paragraph_texts(mgr) == ["target", "X"] + + def test_delete_occurrence_0_edits_split_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.suggest_deletion("target", occurrence=0) + assert _paragraph_texts(mgr) == ["", "target"] + + def test_delete_occurrence_1_edits_single_node_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.suggest_deletion("target", occurrence=1) + assert _paragraph_texts(mgr) == ["target", ""] + + def test_insert_after_occurrence_0_edits_split_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.insert_text_after("target", "XX", occurrence=0) + assert _paragraph_texts(mgr) == ["targetXX", "target"] + + def test_insert_after_occurrence_1_edits_single_node_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.insert_text_after("target", "XX", occurrence=1) + assert _paragraph_texts(mgr) == ["target", "targetXX"] + + def test_insert_before_occurrence_0_edits_split_match(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + mgr.insert_text_before("target", "XX", occurrence=0) + assert _paragraph_texts(mgr) == ["XXtarget", "target"] + + def test_error_total_matches_count_matches(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + with pytest.raises(TextNotFoundError) as exc_info: + mgr.replace_text("target", "X", occurrence=5) + assert exc_info.value.total_occurrences == mgr.count_matches("target") == 2 + + def test_no_match_raises_plain_not_found(self, temp_xml): + mgr = _make_manager(temp_xml(self.BODY)) + with pytest.raises(TextNotFoundError, match="Text not found"): + mgr.suggest_deletion("missing") + + +class TestInsertPlacementMidNode: + """_insert_near_match must split the anchor's w:t at the match edge. + + It used to insert relative to whole runs, so a mid-node anchor pushed the + insertion to the run boundary ("Hello worldXX" instead of "HelloXX world"). + """ + + def test_insert_after_mid_node_paragraph_scoped(self, temp_xml): + mgr = _make_manager(temp_xml("Hello world")) + mgr.insert_text_after("Hello", "XX", paragraph=_pref(mgr, 1)) + assert _paragraph_texts(mgr) == ["HelloXX world"] + + def test_insert_before_mid_node_paragraph_scoped(self, temp_xml): + mgr = _make_manager(temp_xml("Hello world")) + mgr.insert_text_before("world", "XX", paragraph=_pref(mgr, 1)) + assert _paragraph_texts(mgr) == ["Hello XXworld"] + + def test_insert_after_mid_node_batch(self, temp_xml): + mgr = _make_manager(temp_xml("Hello world")) + op = EditOperation(action="insert_after", paragraph=_pref(mgr, 1), anchor="Hello", text="XX") + mgr.batch_edit([op]) + assert _paragraph_texts(mgr) == ["HelloXX world"] + + def test_insert_before_mid_node_batch(self, temp_xml): + mgr = _make_manager(temp_xml("Hello world")) + op = EditOperation(action="insert_before", paragraph=_pref(mgr, 1), anchor="world", text="XX") + mgr.batch_edit([op]) + assert _paragraph_texts(mgr) == ["Hello XXworld"] + + def test_insert_after_mid_node_document_wide(self, temp_xml): + mgr = _make_manager(temp_xml("Hello world")) + mgr.insert_text_after("Hello", "XX") + assert _paragraph_texts(mgr) == ["HelloXX world"] + + def test_insert_before_mid_node_document_wide(self, temp_xml): + mgr = _make_manager(temp_xml("Hello world")) + mgr.insert_text_before("world", "XX") + assert _paragraph_texts(mgr) == ["Hello XXworld"] + + def test_insert_after_preserves_sibling_wt(self, temp_xml): + mgr = _make_manager(temp_xml("cat dog")) + mgr.insert_text_after("cat", "XX", paragraph=_pref(mgr, 1)) + assert _paragraph_texts(mgr) == ["catXX dog"] + + def test_insert_after_run_boundary_edge(self, temp_xml): + # Anchor ends exactly at a run boundary — placement was already correct. + mgr = _make_manager(temp_xml("cat dog")) + mgr.insert_text_after("cat", "XX", paragraph=_pref(mgr, 1)) + assert _paragraph_texts(mgr) == ["catXX dog"] + + def test_insert_after_mid_node_inside_ins(self, temp_xml): + # Anchor inside an existing insertion: splice at the exact offset, no nested w:ins. + mgr = _make_manager( + temp_xml( + '' + "Hello world" + ) + ) + mgr.insert_text_after("Hello", "XX", paragraph=_pref(mgr, 1)) + assert _paragraph_texts(mgr) == ["HelloXX world"] + for ins in mgr.editor.dom.getElementsByTagName("w:ins"): + assert len(ins.getElementsByTagName("w:ins")) == 0 diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index bae5437..c1c86d0 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -7,8 +7,8 @@ from conftest import find_ref from docx_editor import Document, TextNotFoundError -from docx_editor.exceptions import RevisionError from docx_editor.track_changes import Revision, RevisionManager, _escape_xml +from docx_editor.xml_editor import DocxXMLEditor, build_text_map def _attach_text_node(mock_elem, text): @@ -676,19 +676,19 @@ def test_escape_no_special_chars(self): class TestRevisionManagerErrorHandling: """Tests for error handling in RevisionManager.""" - def test_get_nth_match_no_matches(self, clean_workspace): - """Test _get_nth_match raises error when no matches found.""" + def test_replace_text_no_matches(self, clean_workspace): + """Test document-wide replace raises error when no matches found.""" doc = Document.open(clean_workspace) with pytest.raises(TextNotFoundError) as exc_info: - doc._revision_manager._get_nth_match("nonexistent_xyz_123", 0) + doc._revision_manager.replace_text("nonexistent_xyz_123", "X") assert "not found" in str(exc_info.value).lower() doc.close() - def test_get_nth_match_occurrence_out_of_range(self, clean_workspace): - """Test _get_nth_match raises error for invalid occurrence.""" + def test_replace_text_occurrence_out_of_range(self, clean_workspace): + """Test document-wide replace raises error for invalid occurrence.""" doc = Document.open(clean_workspace) # "Sample" exists once in the document @@ -698,9 +698,10 @@ def test_get_nth_match_occurrence_out_of_range(self, clean_workspace): pytest.skip("Test text not found") with pytest.raises(TextNotFoundError) as exc_info: - doc._revision_manager._get_nth_match("Sample", occurrence=count + 10) + doc._revision_manager.replace_text("Sample", "X", occurrence=count + 10) assert "occurrence" in str(exc_info.value).lower() + assert exc_info.value.total_occurrences == count doc.close() @@ -708,56 +709,6 @@ def test_get_nth_match_occurrence_out_of_range(self, clean_workspace): class TestRevisionManagerWithMockedEditor: """Tests using mocked editor for edge cases.""" - def test_replace_text_no_parent_run_raises_error(self): - """Test replace_text raises RevisionError when no parent w:r found.""" - # Create mock editor - mock_editor = MagicMock() - - # Create mock element without proper parent hierarchy - mock_elem = MagicMock() - mock_elem.parentNode = None # No parent - - mock_editor.find_all_nodes.return_value = [mock_elem] - - manager = RevisionManager(mock_editor) - - with pytest.raises(RevisionError) as exc_info: - manager.replace_text("test", "TEST") - - assert "parent w:r" in str(exc_info.value).lower() - - def test_suggest_deletion_no_parent_run_raises_error(self): - """Test suggest_deletion raises RevisionError when no parent w:r found.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_elem.parentNode = None - - mock_editor.find_all_nodes.return_value = [mock_elem] - - manager = RevisionManager(mock_editor) - - with pytest.raises(RevisionError) as exc_info: - manager.suggest_deletion("test") - - assert "parent w:r" in str(exc_info.value).lower() - - def test_insert_text_no_parent_run_raises_error(self): - """Test _insert_text raises RevisionError when no parent w:r found.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_elem.parentNode = None - - mock_editor.find_all_nodes.return_value = [mock_elem] - - manager = RevisionManager(mock_editor) - - with pytest.raises(RevisionError) as exc_info: - manager.insert_text_after("test", "new text") - - assert "parent w:r" in str(exc_info.value).lower() - def test_parse_revision_missing_id_returns_none(self): """Test _parse_revision returns None when w:id is missing.""" mock_editor = MagicMock() @@ -949,458 +900,78 @@ def test_reject_all_then_list(self, clean_workspace): doc.close() -class TestMockedEdgeCases: - """Tests for edge cases using mocks to reach uncovered branches.""" - - def test_replace_text_parent_traversal_loop(self): - """Test replace_text when elem.parentNode is not immediately w:r.""" - mock_editor = MagicMock() - - # Create a chain: elem -> intermediate_node -> run (w:r) - mock_elem = MagicMock() - mock_intermediate = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] # No rPr - - mock_intermediate.nodeName = "other" - mock_intermediate.parentNode = mock_run - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_intermediate - _attach_text_node(mock_elem, "hello world") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - # Mock replace_node to return a node with w:ins - mock_ins_node = MagicMock() - mock_ins_node.nodeType = mock_ins_node.ELEMENT_NODE - mock_ins_node.tagName = "w:ins" - mock_ins_node.getAttribute.return_value = "42" - - mock_editor.replace_node.return_value = [mock_ins_node] - - manager = RevisionManager(mock_editor) - result = manager.replace_text("hello", "HELLO") - - assert result == 42 - - def test_suggest_deletion_parent_traversal_loop(self): - """Test suggest_deletion when elem.parentNode is not immediately w:r.""" - mock_editor = MagicMock() - - # Create a chain: elem -> intermediate_node -> run (w:r) - mock_elem = MagicMock() - mock_intermediate = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] # No rPr - - mock_intermediate.nodeName = "other" - mock_intermediate.parentNode = mock_run - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_intermediate - _attach_text_node(mock_elem, "hello world") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - # Mock replace_node to return a node with w:del - mock_del_node = MagicMock() - mock_del_node.nodeType = mock_del_node.ELEMENT_NODE - mock_del_node.tagName = "w:del" - mock_del_node.getAttribute.return_value = "43" - - mock_editor.replace_node.return_value = [mock_del_node] - - manager = RevisionManager(mock_editor) - result = manager.suggest_deletion("hello") - - assert result == 43 - - def test_insert_text_parent_traversal_loop(self): - """Test _insert_text when elem.parentNode is not immediately w:r.""" - mock_editor = MagicMock() - - # Create a chain: elem -> intermediate_node -> run (w:r) - mock_elem = MagicMock() - mock_intermediate = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] # No rPr - - mock_intermediate.nodeName = "other" - mock_intermediate.parentNode = mock_run - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_intermediate - _attach_text_node(mock_elem, "anchor") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - # Mock replace_node to return a node with w:ins - mock_ins_node = MagicMock() - mock_ins_node.nodeType = mock_ins_node.ELEMENT_NODE - mock_ins_node.tagName = "w:ins" - mock_ins_node.getAttribute.return_value = "44" - - mock_editor.replace_node.return_value = [mock_ins_node] - - manager = RevisionManager(mock_editor) - result = manager.insert_text_after("anchor", "new text") - - assert result == 44 - - def test_replace_text_with_rpr(self): - """Test replace_text preserves w:rPr when present.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - - # Mock rPr element - mock_rPr = MagicMock() - mock_rPr.toxml.return_value = "" - mock_run.getElementsByTagName.return_value = [mock_rPr] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "hello world") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - mock_ins_node = MagicMock() - mock_ins_node.nodeType = mock_ins_node.ELEMENT_NODE - mock_ins_node.tagName = "w:ins" - mock_ins_node.getAttribute.return_value = "45" - - mock_editor.replace_node.return_value = [mock_ins_node] - - manager = RevisionManager(mock_editor) - result = manager.replace_text("hello", "HELLO") - - assert result == 45 - # Verify rPr was included in the XML - call_args = mock_editor.replace_node.call_args[0][1] - assert "" in call_args - - def test_suggest_deletion_with_rpr(self): - """Test suggest_deletion preserves w:rPr when present.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - - # Mock rPr element - mock_rPr = MagicMock() - mock_rPr.toxml.return_value = "" - mock_run.getElementsByTagName.return_value = [mock_rPr] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "hello world") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - mock_del_node = MagicMock() - mock_del_node.nodeType = mock_del_node.ELEMENT_NODE - mock_del_node.tagName = "w:del" - mock_del_node.getAttribute.return_value = "46" - - mock_editor.replace_node.return_value = [mock_del_node] - - manager = RevisionManager(mock_editor) - result = manager.suggest_deletion("hello") - - assert result == 46 - # Verify rPr was included in the XML - call_args = mock_editor.replace_node.call_args[0][1] - assert "" in call_args - - def test_insert_text_with_rpr(self): - """Test _insert_text preserves w:rPr when present.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - - # Mock rPr element - mock_rPr = MagicMock() - mock_rPr.toxml.return_value = "" - - def _get_by_tag(tag): - if tag == "w:rPr": - return [mock_rPr] - if tag == "w:t": - return [mock_elem] - return [] - - mock_run.getElementsByTagName.side_effect = _get_by_tag - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "anchor") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - mock_ins_node = MagicMock() - mock_ins_node.nodeType = mock_ins_node.ELEMENT_NODE - mock_ins_node.tagName = "w:ins" - mock_ins_node.getAttribute.return_value = "47" - - mock_editor.replace_node.return_value = [mock_ins_node] - - manager = RevisionManager(mock_editor) - result = manager.insert_text_after("anchor", "new text") - - assert result == 47 - # Verify rPr was included in the XML - call_args = mock_editor.replace_node.call_args[0][1] - assert "" in call_args - - def test_replace_text_returns_minus_one_when_no_ins_found(self): - """Test replace_text returns -1 when w:ins not found in result.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "hello") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - # Return nodes that don't include w:ins - mock_other_node = MagicMock() - mock_other_node.nodeType = mock_other_node.ELEMENT_NODE - mock_other_node.tagName = "w:r" - - mock_editor.replace_node.return_value = [mock_other_node] - - manager = RevisionManager(mock_editor) - result = manager.replace_text("hello", "HELLO") - - assert result == -1 - - def test_suggest_deletion_returns_minus_one_when_no_del_found(self): - """Test suggest_deletion returns -1 when w:del not found in result.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "hello") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - # Return nodes that don't include w:del - mock_other_node = MagicMock() - mock_other_node.nodeType = mock_other_node.ELEMENT_NODE - mock_other_node.tagName = "w:r" - - mock_editor.replace_node.return_value = [mock_other_node] - - manager = RevisionManager(mock_editor) - result = manager.suggest_deletion("hello") - +class TestDocumentWideEditsRealXml: + """Real-XML edge-case coverage for the unified document-wide edit path.""" + + NS = 'xmlns:w="http://schemas.openxmlformats.org/wordprocessingml/2006/main"' + INS_ATTRS = 'w:id="1" w:author="A" w:date="2024-01-01T00:00:00Z"' + + def _manager(self, tmp_path, body_xml): + xml = f'{body_xml}' + xml_path = tmp_path / "doc.xml" + xml_path.write_text(xml) + editor = DocxXMLEditor(xml_path, rsid="00000000", author="Test Author") + return RevisionManager(editor) + + def _accepted_text(self, mgr): + return "".join(build_text_map(p).text for p in mgr.editor.dom.getElementsByTagName("w:p")) + + def test_suggest_deletion_preserves_rpr(self, tmp_path): + """Deleting text from a formatted run keeps the run properties.""" + mgr = self._manager(tmp_path, "Hello world") + mgr.suggest_deletion("Hello") + del_elems = mgr.editor.dom.getElementsByTagName("w:del") + assert len(del_elems) == 1 + assert len(del_elems[0].getElementsByTagName("w:i")) == 1 + # The preserved " world" run keeps its formatting too + for wt in mgr.editor.dom.getElementsByTagName("w:t"): + run = wt.parentNode + assert len(run.getElementsByTagName("w:i")) == 1 + + def test_insert_text_preserves_rpr(self, tmp_path): + """Inserting near a formatted anchor applies the anchor run's properties.""" + mgr = self._manager(tmp_path, "anchor text") + mgr.insert_text_after("anchor", " NEW") + ins_elems = mgr.editor.dom.getElementsByTagName("w:ins") + assert len(ins_elems) == 1 + assert len(ins_elems[0].getElementsByTagName("w:u")) == 1 + + def test_replace_inside_ins_edits_in_place(self, tmp_path): + """Replacing text whose run sits under a w:ins wrapper splices in place.""" + mgr = self._manager(tmp_path, f"hello world") + result = mgr.replace_text("hello", "HELLO") + assert result == -1 # no new revision created + assert self._accepted_text(mgr) == "HELLO world" + assert len(mgr.editor.dom.getElementsByTagName("w:del")) == 0 + + def test_delete_inside_ins_shrinks_insertion(self, tmp_path): + """Deleting text whose run sits under a w:ins wrapper shrinks the insertion.""" + mgr = self._manager(tmp_path, f"hello world") + result = mgr.suggest_deletion("hello ") assert result == -1 + assert self._accepted_text(mgr) == "world" + assert len(mgr.editor.dom.getElementsByTagName("w:del")) == 0 - def test_insert_text_returns_minus_one_when_no_ins_found(self): - """Test _insert_text returns -1 when w:ins not found in result.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "anchor") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - # Return nodes that don't include w:ins - mock_other_node = MagicMock() - mock_other_node.nodeType = mock_other_node.ELEMENT_NODE - mock_other_node.tagName = "w:r" - - mock_editor.insert_after.return_value = [mock_other_node] - - manager = RevisionManager(mock_editor) - result = manager.insert_text_after("anchor", "text") - + def test_insert_inside_ins_splices_without_nesting(self, tmp_path): + """Inserting at an anchor whose run sits under a w:ins wrapper avoids nested w:ins.""" + mgr = self._manager(tmp_path, f"hello world") + result = mgr.insert_text_after("hello", "XX") assert result == -1 + assert self._accepted_text(mgr) == "helloXX world" + ins_elems = mgr.editor.dom.getElementsByTagName("w:ins") + assert len(ins_elems) == 1 - def test_replace_text_text_mismatch_raises_error(self): - """Test replace_text raises error when text found by matcher but not in actual content.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - # The matcher found "world" in this node, but actual content is different - _attach_text_node(mock_elem, "different content") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - manager = RevisionManager(mock_editor) - - with pytest.raises(TextNotFoundError): - manager.replace_text("world", "WORLD") - - def test_suggest_deletion_text_mismatch_raises_error(self): - """Test suggest_deletion raises error when text found by matcher but not in actual content.""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - # The matcher found "world" in this node, but actual content is different - _attach_text_node(mock_elem, "different content") - - mock_editor.find_all_nodes.return_value = [mock_elem] - - manager = RevisionManager(mock_editor) - - with pytest.raises(TextNotFoundError): - manager.suggest_deletion("world") - - def test_replace_text_with_before_text_only(self): - """Test replace_text with text at end (only before_text, no after_text).""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "prefix hello") # "hello" is at end - - mock_editor.find_all_nodes.return_value = [mock_elem] - - mock_ins_node = MagicMock() - mock_ins_node.nodeType = mock_ins_node.ELEMENT_NODE - mock_ins_node.tagName = "w:ins" - mock_ins_node.getAttribute.return_value = "48" - - mock_editor.replace_node.return_value = [mock_ins_node] - - manager = RevisionManager(mock_editor) - result = manager.replace_text("hello", "HELLO") - - assert result == 48 - # Verify before_text run was included - call_args = mock_editor.replace_node.call_args[0][1] - assert "prefix" in call_args - - def test_suggest_deletion_with_before_text_only(self): - """Test suggest_deletion with text at end (only before_text, no after_text).""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "prefix hello") # "hello" is at end - - mock_editor.find_all_nodes.return_value = [mock_elem] - - mock_del_node = MagicMock() - mock_del_node.nodeType = mock_del_node.ELEMENT_NODE - mock_del_node.tagName = "w:del" - mock_del_node.getAttribute.return_value = "49" - - mock_editor.replace_node.return_value = [mock_del_node] - - manager = RevisionManager(mock_editor) - result = manager.suggest_deletion("hello") - - assert result == 49 - # Verify before_text run was included - call_args = mock_editor.replace_node.call_args[0][1] - assert "prefix" in call_args - - def test_suggest_deletion_with_after_text_only(self): - """Test suggest_deletion with text at start (only after_text, no before_text).""" - mock_editor = MagicMock() - - mock_elem = MagicMock() - mock_run = MagicMock() - - mock_run.nodeName = "w:r" - mock_run.parentNode = None - mock_run.getElementsByTagName.return_value = [] - - mock_elem.nodeName = "w:t" - mock_elem.parentNode = mock_run - _attach_text_node(mock_elem, "hello suffix") # "hello" is at start - - mock_editor.find_all_nodes.return_value = [mock_elem] - - mock_del_node = MagicMock() - mock_del_node.nodeType = mock_del_node.ELEMENT_NODE - mock_del_node.tagName = "w:del" - mock_del_node.getAttribute.return_value = "50" - - mock_editor.replace_node.return_value = [mock_del_node] - - manager = RevisionManager(mock_editor) - result = manager.suggest_deletion("hello") + def test_replace_at_end_of_node_preserves_prefix(self, tmp_path): + """Replacing text at the end of a w:t keeps the preceding text.""" + mgr = self._manager(tmp_path, "prefix hello") + mgr.replace_text("hello", "HELLO") + assert self._accepted_text(mgr) == "prefix HELLO" - assert result == 50 - # Verify after_text run was included - call_args = mock_editor.replace_node.call_args[0][1] - assert "suffix" in call_args + def test_delete_at_start_of_node_preserves_suffix(self, tmp_path): + """Deleting text at the start of a w:t keeps the following text.""" + mgr = self._manager(tmp_path, "hello suffix") + mgr.suggest_deletion("hello") + assert self._accepted_text(mgr) == " suffix" class TestListRevisionsEdgeCases: @@ -1849,10 +1420,9 @@ def _split_wt_text_nodes(doc, target_text, tag="w:t"): class TestMultiTextNodeWtElements: """Issue #9: w:t elements with multiple TEXT_NODE children (smart-quote split).""" - # The buggy paths in replace_text/suggest_deletion/_insert_text (lines 547, - # 650, 1389) are only reached when paragraph=None (via _get_nth_match). - # When paragraph= is passed, control routes through _replace_across_nodes - # which uses _get_node_text and avoids the bug. + # Both the document-wide (paragraph=None) and paragraph-scoped paths route + # through the text-map helpers, which read node text via _get_node_text and + # so tolerate w:t elements whose text is split across TEXT_NODE children. def test_set_node_text_consolidates_split_nodes(self, clean_workspace): """Direct contract test for _set_node_text: starts from a multi-TEXT_NODE From 78851c4c103a997c39606a1aaa5f6aa15f350c1d Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Tue, 14 Jul 2026 12:18:36 +0200 Subject: [PATCH 2/2] review: add occurrence default to _find_document_wide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-reviewer pass on PR #39. Fixed: missing `occurrence: int = 0` default, matching the sibling finders (_find_in_paragraph, _find_across_boundaries). Verified as not applicable here and deferred with notes in ISSUES.md: straddle-match insertion into a foreign w:ins (needs #19's ins-splitting machinery; in-place rebuild would nest w:ins) and front-hoisted non-text run children in the insert rebuild (#20, same pattern as the existing replace/delete paths). xml:space on generated w:t was a false positive — the editor's attribute injection adds it. --- docx_editor/track_changes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index b4a6c4f..8e94052 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -532,7 +532,7 @@ def count_matches(self, text: str) -> int: local_occ += 1 return count - def _find_document_wide(self, text: str, occurrence: int) -> TextMapMatch: + def _find_document_wide(self, text: str, occurrence: int = 0) -> TextMapMatch: """Document-wide nth-occurrence lookup via text maps. Raises: