From 49645c3aadd09337ae3ef2a66151eef194061543 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Tue, 14 Jul 2026 16:26:10 +0200 Subject: [PATCH 1/2] fix: preserve run-internal child order in tracked edits (ISSUES.md #20) Non-text run children (w:tab, w:br, w:drawing, field chars) were front-hoisted into a single run emitted before all text parts whenever a tracked replace/delete/insert rebuilt a run, reordering document content. - Add rebuild_run_fragments() to xml_editor: walks a run's direct children in document order, preserves non-text children in place, and delegates each w:t to a caller-supplied renderer. - Route all rebuild sites through it: _replace_same_context, _delete_regular_segment, _delete_same_context, _insert_near_match, and the comment-marker rebuild in comments.py. - _split_foreign_ins_at keeps its own ordered walk (children must land on the correct side of the split) instead of front-hoisting. - Drop the now-unused _get_non_text_children_xml helper. - Side effect: w:t nested inside a drawing's text box is no longer duplicated as a top-level run. Adds tests/test_run_child_order.py (19 ordered-token tests) plus two split-side tests in test_foreign_ins_edits.py. --- docx_editor/comments.py | 47 ++---- docx_editor/track_changes.py | 180 ++++++++++---------- docx_editor/xml_editor.py | 25 +++ tests/test_foreign_ins_edits.py | 52 +++++- tests/test_run_child_order.py | 283 ++++++++++++++++++++++++++++++++ 5 files changed, 463 insertions(+), 124 deletions(-) create mode 100644 tests/test_run_child_order.py diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 3b3a4f6..cc22551 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -26,6 +26,7 @@ compute_paragraph_hash, find_in_text_map, get_text_node_data, + rebuild_run_fragments, ) # Path to template files @@ -309,59 +310,47 @@ def _rebuild_run_with_markers( text box are left untouched. """ rPr_xml = _get_rPr_xml(run) - xml_parts: list[str] = [] - for child in run.childNodes: - if child.nodeType != child.ELEMENT_NODE: - continue - tag = getattr(child, "tagName", "") - if tag == "w:rPr": - continue # Already captured by rPr_xml - if tag != "w:t": - # Non-text content (w:tab, w:br, w:drawing, …) — preserve in - # its own run so document order is maintained. - xml_parts.append(f"{rPr_xml}{child.toxml()}") - continue - - wt = child + def render_wt(wt) -> list[str]: wt_text = get_text_node_data(wt) is_start_here = bool(start_marker) and wt is start_node is_end_here = bool(end_marker) and wt is end_node + parts: list[str] = [] if not is_start_here and not is_end_here: if wt_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") - continue - - if is_start_here and is_end_here: + parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") + elif is_start_here and is_end_here: before = wt_text[:start_offset] matched = wt_text[start_offset : end_offset + 1] after = wt_text[end_offset + 1 :] if before: - xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") - xml_parts.append(start_marker) + parts.append(f"{rPr_xml}{_escape_xml(before)}") + parts.append(start_marker) if matched: - xml_parts.append(f"{rPr_xml}{_escape_xml(matched)}") - xml_parts.append(end_marker) + parts.append(f"{rPr_xml}{_escape_xml(matched)}") + parts.append(end_marker) if after: - xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + parts.append(f"{rPr_xml}{_escape_xml(after)}") elif is_start_here: before = wt_text[:start_offset] after = wt_text[start_offset:] if before: - xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") - xml_parts.append(start_marker) + parts.append(f"{rPr_xml}{_escape_xml(before)}") + parts.append(start_marker) if after: - xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + parts.append(f"{rPr_xml}{_escape_xml(after)}") else: # is_end_here only before = wt_text[: end_offset + 1] after = wt_text[end_offset + 1 :] if before: - xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") - xml_parts.append(end_marker) + parts.append(f"{rPr_xml}{_escape_xml(before)}") + parts.append(end_marker) if after: - xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + parts.append(f"{rPr_xml}{_escape_xml(after)}") + return parts + xml_parts = rebuild_run_fragments(run, rPr_xml, render_wt) self.document_editor.replace_node(run, "".join(xml_parts)) def reply_to_comment(self, parent_comment_id: int, reply_text: str) -> int: diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index c92871b..e73daba 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -28,6 +28,7 @@ compute_paragraph_hash, find_in_text_map, get_text_node_data, + rebuild_run_fragments, ) @@ -831,16 +832,6 @@ def _get_run_info(self, node) -> tuple[Element | None, str]: rPr_xml = rPr_elems[0].toxml() return run, rPr_xml - def _get_non_text_children_xml(self, run) -> str: - """Serialize non-text, non-rPr children (w:tab, w:br, w:drawing, etc.) of a run.""" - parts = [] - for child in run.childNodes: - if child.nodeType == child.ELEMENT_NODE: - tag = getattr(child, "tagName", "") - if tag not in ("w:t", "w:rPr"): - parts.append(child.toxml()) - return "".join(parts) - def _get_node_text(self, node) -> str: """Get text content of a w:t node by concatenating ALL child text nodes. @@ -1012,38 +1003,38 @@ def _replace_same_context(self, match: TextMapMatch, replace_with: str) -> int: # Build deterministic node-to-part mapping using node ids from parts node_to_part = {nid: (before, matched, after) for before, matched, after, nid in info["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}") - - # Iterate all w:t in document order, emitting unmatched as preserved - all_wt = list(run.getElementsByTagName("w:t")) parts_emitted = 0 - for wt in all_wt: + + # Keyword-only defaults bind this iteration's state (B023) + def render_wt(wt, *, node_to_part=node_to_part, run_rPr=rPr_xml, base_idx=part_idx) -> list[str]: + nonlocal parts_emitted + fragments: list[str] = [] if id(wt) in node_to_part: before, matched, after = node_to_part[id(wt)] parts_emitted += 1 if before: - xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") - xml_parts.append( - f"{rPr_xml}{_escape_xml(matched)}" + fragments.append(f"{run_rPr}{_escape_xml(before)}") + fragments.append( + f"{run_rPr}{_escape_xml(matched)}" ) # Insert replacement after the last deletion - if part_idx + parts_emitted == total_parts: - xml_parts.append(f"{first_rPr}{_escape_xml(replace_with)}") + if base_idx + parts_emitted == total_parts: + fragments.append(f"{first_rPr}{_escape_xml(replace_with)}") if after: - xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + fragments.append(f"{run_rPr}{_escape_xml(after)}") else: # Unmatched sibling — preserve wt_text = self._get_node_text(wt) if wt_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") + fragments.append(f"{run_rPr}{_escape_xml(wt_text)}") + return fragments + # Emit the run's children in document order (w:t split around the + # match; w:tab/w:br/w:drawing/… preserved in place) + xml_parts.extend(rebuild_run_fragments(run, rPr_xml, render_wt)) part_idx += len(node_to_part) # Replace all affected runs: insert new XML before first run, remove all runs @@ -1251,20 +1242,38 @@ def _split_foreign_ins_at(self, edge_node, offset: int) -> Element | None: return None node_text = self._get_node_text(edge_node) - run_wts = list(run.getElementsByTagName("w:t")) - edge_idx = next(i for i, wt in enumerate(run_wts) if wt is edge_node) - left_texts = [self._get_node_text(wt) for wt in run_wts[:edge_idx]] - if node_text[:offset]: - left_texts.append(node_text[:offset]) - right_texts = [] - if node_text[offset:]: - right_texts.append(node_text[offset:]) - right_texts += [self._get_node_text(wt) for wt in run_wts[edge_idx + 1 :]] - - if not right_texts: + # This site splits a run into left/right halves rather than rendering + # per-w:t, and must know which side each child lands on, so it keeps + # its own direct-children walk instead of rebuild_run_fragments. + # Non-text children (w:tab, w:br, w:drawing, …) stay in document + # order on whichever side of the split point they fall. + left_parts: list[str] = [] + right_parts: list[str] = [] + side = left_parts + for child in run.childNodes: + if child.nodeType != child.ELEMENT_NODE: + continue + tag = getattr(child, "tagName", "") + if tag == "w:rPr": + continue + if child is edge_node: + if node_text[:offset]: + left_parts.append(f"{rPr_xml}{_escape_xml(node_text[:offset])}") + if node_text[offset:]: + right_parts.append(f"{rPr_xml}{_escape_xml(node_text[offset:])}") + side = right_parts + continue + if tag == "w:t": + wt_text = self._get_node_text(child) + if wt_text: + side.append(f"{rPr_xml}{_escape_xml(wt_text)}") + continue + side.append(f"{rPr_xml}{child.toxml()}") + + if not right_parts: # Split point is at the end of this run — run boundary already return run - if not left_texts: + if not left_parts: # Split point is immediately before this run # (isinstance so ty narrows minidom's sibling union — the usual # nodeType comparison doesn't) @@ -1275,19 +1284,9 @@ def _split_foreign_ins_at(self, edge_node, offset: int) -> Element | None: prev = prev.previousSibling return None - # Mid-run: rebuild as left/right runs (non-text children front-hoisted, - # matching the existing sites — interleaving is issue #20's follow-up) - xml_parts = [] - non_text_xml = self._get_non_text_children_xml(run) - if non_text_xml: - xml_parts.append(f"{rPr_xml}{non_text_xml}") - xml_parts += [f"{rPr_xml}{_escape_xml(t)}" for t in left_texts] - left_len = len(xml_parts) - xml_parts += [f"{rPr_xml}{_escape_xml(t)}" for t in right_texts] - - new_nodes = self.editor.replace_node(run, "".join(xml_parts)) + new_nodes = self.editor.replace_node(run, "".join(left_parts + right_parts)) elements = [n for n in new_nodes if n.nodeType == n.ELEMENT_NODE] - return elements[left_len - 1] + return elements[len(left_parts) - 1] def _insert_own_ins_within_foreign_ins(self, ins_elem, edge_node, offset: int, text: str, rPr_xml: str) -> int: """Insert our own at (edge_node, offset) inside a foreign ins. @@ -1477,24 +1476,18 @@ def _delete_regular_segment(self, positions: list) -> tuple[int, Element | None] if rid in processed_runs: continue - # Build xml_parts for ALL w:t nodes in this run, preserving unmatched ones - matched_node_ids = {id(ng["node"]) for ng in run_info["nodes"].values()} node_items = list(run_info["nodes"].values()) - all_wt_nodes = list(run.getElementsByTagName("w:t")) - xml_parts: list[str] = [] - - # 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_node in all_wt_nodes: - if id(wt_node) not in matched_node_ids: + # Render ALL w:t nodes in this run, preserving unmatched ones. + # Keyword-only defaults bind this iteration's state (B023). + def render_wt(wt_node, *, run_info=run_info, run_rPr=rPr_xml, node_items=node_items, rid=rid) -> list[str]: + fragments: list[str] = [] + if id(wt_node) not in run_info["nodes"]: # Unmatched sibling — preserve as-is wt_text = self._get_node_text(wt_node) if wt_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") - continue + fragments.append(f"{run_rPr}{_escape_xml(wt_text)}") + return fragments ng = run_info["nodes"][id(wt_node)] node_text = self._get_node_text(ng["node"]) @@ -1519,12 +1512,15 @@ def _delete_regular_segment(self, positions: list) -> tuple[int, Element | None] matched = node_text[first_offset : last_offset + 1] if before: - xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") - xml_parts.append(f"{rPr_xml}{_escape_xml(matched)}") + fragments.append(f"{run_rPr}{_escape_xml(before)}") + fragments.append(f"{run_rPr}{_escape_xml(matched)}") if after: - xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + fragments.append(f"{run_rPr}{_escape_xml(after)}") + return fragments - new_xml = "".join(xml_parts) + # Emit the run's children in document order (w:tab/w:br/w:drawing/… + # preserved in place) + new_xml = "".join(rebuild_run_fragments(run, rPr_xml, render_wt)) nodes = self.editor.insert_before(run, new_xml) if run.parentNode: run.parentNode.removeChild(run) @@ -1557,7 +1553,6 @@ def _delete_same_context(self, match: TextMapMatch) -> int: return first_id # Group parts by run, using node ids from _build_cross_boundary_parts - matched_node_ids = {nid for _, _, _, _, _, nid in parts} run_parts: OrderedDict[int, list] = OrderedDict() for part in parts: rid = id(part[0]) @@ -1573,26 +1568,26 @@ def _delete_same_context(self, match: TextMapMatch) -> int: # Build deterministic node-to-part mapping using node ids from parts node_to_part = {nid: (rp_xml, before, matched, after) for _, rp_xml, before, matched, after, nid in rparts} - # 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}") - - # Emit w:t nodes in document order, matched as , unmatched preserved - all_wt = list(run.getElementsByTagName("w:t")) - for wt in all_wt: + # Keyword-only defaults bind this iteration's state (B023) + def render_wt(wt, *, node_to_part=node_to_part, run_rPr=rPr_xml) -> list[str]: + fragments: list[str] = [] if id(wt) in node_to_part: rp_xml, before, matched, after = node_to_part[id(wt)] if before: - xml_parts.append(f"{rp_xml}{_escape_xml(before)}") - xml_parts.append(f"{rp_xml}{_escape_xml(matched)}") + fragments.append(f"{rp_xml}{_escape_xml(before)}") + fragments.append(f"{rp_xml}{_escape_xml(matched)}") if after: - xml_parts.append(f"{rp_xml}{_escape_xml(after)}") - elif id(wt) not in matched_node_ids: + fragments.append(f"{rp_xml}{_escape_xml(after)}") + else: # Unmatched sibling — preserve wt_text = self._get_node_text(wt) if wt_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") + fragments.append(f"{run_rPr}{_escape_xml(wt_text)}") + return fragments + + # Emit the run's children in document order (matched w:t as + # ; w:tab/w:br/w:drawing/… preserved in place) + xml_parts.extend(rebuild_run_fragments(run, rPr_xml, render_wt)) first_run = parts[0][0] new_xml = "".join(xml_parts) @@ -1722,32 +1717,29 @@ def _insert_near_match(self, match: TextMapMatch, text: str, position: Literal[" return -1 return self._insert_own_ins_within_foreign_ins(ins_ancestor, edge.node, offset, text, rPr_xml) - # Rebuild the edge run: split its w:t at the offset and wrap text in + # Rebuild the edge run: split its w:t at the offset and wrap text in + # ; non-text children (w:tab, w:br, w:drawing, …) stay in place 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"): + def render_wt(wt) -> list[str]: + fragments: list[str] = [] 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)}") - xml_parts.append(ins_xml) + fragments.append(f"{rPr_xml}{_escape_xml(before_text)}") + fragments.append(ins_xml) if after_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(after_text)}") + fragments.append(f"{rPr_xml}{_escape_xml(after_text)}") else: # Preserve unmatched sibling w:t wt_text = self._get_node_text(wt) if wt_text: - xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") + fragments.append(f"{rPr_xml}{_escape_xml(wt_text)}") + return fragments - nodes = self.editor.replace_node(run, "".join(xml_parts)) + nodes = self.editor.replace_node(run, "".join(rebuild_run_fragments(run, rPr_xml, render_wt))) for node in nodes: if node.nodeType == node.ELEMENT_NODE and node.tagName == "w:ins": diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index 48579c2..5bf1967 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -612,6 +612,31 @@ def get_text_node_data(elem) -> str: return "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE) +def rebuild_run_fragments(run, rPr_xml: str, render_wt) -> list[str]: + """Rebuild ``run``'s children as XML fragments, preserving document order. + + Iterates *direct* children of ``run`` (not descendants, so ``w:t`` nodes + nested inside a drawing's text box stay inside the drawing): + + - ``w:rPr`` is skipped — callers bake ``rPr_xml`` into every fragment. + - Non-text children (``w:tab``, ``w:br``, ``w:drawing``, field chars, …) + are preserved in place, each wrapped in its own run carrying ``rPr_xml``. + - Each direct ``w:t`` child is replaced by ``render_wt(wt)``'s fragments. + """ + fragments: list[str] = [] + for child in run.childNodes: + if child.nodeType != child.ELEMENT_NODE: + continue + tag = getattr(child, "tagName", "") + if tag == "w:rPr": + continue + if tag != "w:t": + fragments.append(f"{rPr_xml}{child.toxml()}") + continue + fragments.extend(render_wt(child)) + return fragments + + def build_text_map(paragraph, view: Literal["accepted", "original"] = "accepted") -> TextMap: """Build a text map for a paragraph element. diff --git a/tests/test_foreign_ins_edits.py b/tests/test_foreign_ins_edits.py index 59755be..005aef1 100644 --- a/tests/test_foreign_ins_edits.py +++ b/tests/test_foreign_ins_edits.py @@ -376,7 +376,7 @@ def test_insert_before_at_run_boundary_inside_foreign_ins(self, temp_xml): assert _ins_visible_text(a_ins[1]) == "world" def test_insert_mid_run_with_non_text_child(self, temp_xml): - """Mid-run split of a run that also carries a w:tab (front-hoisted, see #20).""" + """Mid-run split of a run that also carries a leading w:tab.""" xml_path = temp_xml(f"{_foreign_ins('Hello world')}") manager = _make_manager(xml_path) @@ -388,6 +388,56 @@ def test_insert_mid_run_with_non_text_child(self, temp_xml): assert len(a_ins) == 2 assert len(a_ins[0].getElementsByTagName("w:tab")) == 1 + def test_insert_mid_run_tab_between_texts_stays_in_order(self, temp_xml): + """A w:tab between two w:t nodes lands on the correct side of the split. + + Splitting A's ins after "Hello" must leave the tab (which follows the + split point) in A's *second* ins, before "world" — not front-hoisted + into the first (issue #20). + """ + xml_path = temp_xml(f"{_foreign_ins('Helloworld')}") + manager = _make_manager(xml_path) + + manager.insert_text_after("Hello", "X") + + _assert_no_nested_ins(manager) + assert _visible_text(manager) == "HelloXworld" + ins_elems = _ins_elems(manager) + a_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_A] + b_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_B] + assert len(a_ins) == 2 + assert len(b_ins) == 1 + # B's ins sits between the two A halves in document order + assert [e.getAttribute("w:author") for e in ins_elems] == [AUTHOR_A, AUTHOR_B, AUTHOR_A] + # First A half: just "Hello", no tab + assert _ins_visible_text(a_ins[0]) == "Hello" + assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 + # Second A half: the tab followed by "world" + assert _ins_visible_text(a_ins[1]) == "world" + assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 + assert _ins_visible_text(b_ins[0]) == "X" + + def test_insert_mid_run_split_skips_rpr_and_empty_wt(self, temp_xml): + """The split walk skips w:rPr, whitespace text nodes, and empty w:t + children while keeping formatting on both rebuilt halves.""" + run_xml = "Hello world" + xml_path = temp_xml(f"{_foreign_ins(run_xml)}") + manager = _make_manager(xml_path) + + manager.insert_text_after("Hello", "X") + + _assert_no_nested_ins(manager) + assert _visible_text(manager) == "HelloXworld" + a_ins = [e for e in _ins_elems(manager) if e.getAttribute("w:author") == AUTHOR_A] + assert len(a_ins) == 2 + assert _ins_visible_text(a_ins[0]) == "Hello" + assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 + assert _ins_visible_text(a_ins[1]) == "world" + assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 + # Formatting survives the rebuild on both sides of the split + for ins in a_ins: + assert ins.getElementsByTagName("w:b") + def test_insert_before_at_ins_start_no_split(self, temp_xml): xml_path = temp_xml(f"{_foreign_ins('Hello')}") manager = _make_manager(xml_path) diff --git a/tests/test_run_child_order.py b/tests/test_run_child_order.py new file mode 100644 index 0000000..7e47b1a --- /dev/null +++ b/tests/test_run_child_order.py @@ -0,0 +1,283 @@ +"""Tests that tracked edits preserve run-internal child order (ISSUES.md #20). + +A run like ``foobar`` must keep its +tab between "foo" and "bar" when a tracked replace/delete/insert rebuilds the +run. Previously every non-text child (w:tab, w:br, w:drawing, field chars, …) +was hoisted into a single run emitted before all text parts. +""" + +from pathlib import Path + +import pytest + +from docx_editor.track_changes import RevisionManager +from docx_editor.xml_editor import DocxXMLEditor + +NS = 'xmlns:w="http://schemas.openxmlformats.org/wordprocessingml/2006/main"' +AUTHOR = "Test Author" +DATE = "2024-01-01T00:00:00Z" + + +@pytest.fixture +def temp_xml(tmp_path): + def _create_xml(body_xml: str) -> Path: + xml = f'{body_xml}' + xml_path = tmp_path / "test_doc.xml" + xml_path.write_text(xml) + return xml_path + + return _create_xml + + +def _make_manager(xml_path: Path) -> RevisionManager: + editor = DocxXMLEditor(xml_path, rsid="00000000", author=AUTHOR) + return RevisionManager(editor) + + +def _tokens_for_run(run, wrapper: str | None = None) -> list[str]: + """Tokenize a run's direct children in document order.""" + + def wrap(token: str) -> str: + return f"{wrapper}({token})" if wrapper else token + + tokens: list[str] = [] + for child in run.childNodes: + if child.nodeType != child.ELEMENT_NODE: + continue + tag = child.tagName + if tag == "w:rPr": + continue + if tag in ("w:t", "w:delText"): + text = "".join(c.data for c in child.childNodes if c.nodeType == c.TEXT_NODE) + if text: + tokens.append(wrap(text)) + elif tag == "w:tab": + tokens.append(wrap("TAB")) + elif tag == "w:br": + tokens.append(wrap("BR")) + elif tag == "w:fldChar": + tokens.append(wrap(f"FLD({child.getAttribute('w:fldCharType')})")) + elif tag == "w:instrText": + tokens.append(wrap("INSTR")) + elif tag == "w:drawing": + tokens.append(wrap("DRAWING")) + else: # pragma: no cover - no other run content used in these fixtures + tokens.append(wrap(tag)) + return tokens + + +def paragraph_tokens(manager: RevisionManager) -> list[str]: + """Ordered content tokens of the document's first paragraph. + + Direct runs yield their tokens bare; runs inside / + wrappers yield tokens wrapped as INS(...)/DEL(...). Exact list + comparison makes ordering assertions precise (no substring checks). + """ + paragraph = manager.editor.dom.getElementsByTagName("w:p")[0] + tokens: list[str] = [] + for child in paragraph.childNodes: + if child.nodeType != child.ELEMENT_NODE: + continue + if child.tagName == "w:r": + tokens.extend(_tokens_for_run(child)) + elif child.tagName in ("w:ins", "w:del"): + wrapper = "INS" if child.tagName == "w:ins" else "DEL" + for run in child.childNodes: + if run.nodeType == run.ELEMENT_NODE and run.tagName == "w:r": + tokens.extend(_tokens_for_run(run, wrapper)) + return tokens + + +MID_RUN_FIXTURES = [("", "TAB"), ("", "BR")] + + +class TestReplaceMidRunOrder: + """Tracked replace keeps a mid-run tab/br between the surrounding texts.""" + + @pytest.mark.parametrize(("child_xml", "token"), MID_RUN_FIXTURES) + def test_pending_and_accepted(self, temp_xml, child_xml, token): + xml_path = temp_xml(f"foo{child_xml}bar baz") + mgr = _make_manager(xml_path) + + mgr.replace_text("bar", "qux") + assert paragraph_tokens(mgr) == ["foo", token, "DEL(bar)", "INS(qux)", " baz"] + + mgr.accept_all() + assert paragraph_tokens(mgr) == ["foo", token, "qux", " baz"] + + @pytest.mark.parametrize(("child_xml", "token"), MID_RUN_FIXTURES) + def test_rejected(self, temp_xml, child_xml, token): + xml_path = temp_xml(f"foo{child_xml}bar baz") + mgr = _make_manager(xml_path) + + mgr.replace_text("bar", "qux") + mgr.reject_all() + + tokens = paragraph_tokens(mgr) + assert tokens[:2] == ["foo", token] + assert "".join(tokens[2:]) == "bar baz" + + +class TestDeleteMidRunOrder: + """Tracked delete keeps a mid-run tab/br between the surrounding texts.""" + + @pytest.mark.parametrize(("child_xml", "token"), MID_RUN_FIXTURES) + def test_pending_and_accepted(self, temp_xml, child_xml, token): + xml_path = temp_xml(f"foo{child_xml}bar baz") + mgr = _make_manager(xml_path) + + mgr.suggest_deletion("bar") + assert paragraph_tokens(mgr) == ["foo", token, "DEL(bar)", " baz"] + + mgr.accept_all() + assert paragraph_tokens(mgr) == ["foo", token, " baz"] + + @pytest.mark.parametrize(("child_xml", "token"), MID_RUN_FIXTURES) + def test_rejected(self, temp_xml, child_xml, token): + xml_path = temp_xml(f"foo{child_xml}bar baz") + mgr = _make_manager(xml_path) + + mgr.suggest_deletion("bar") + mgr.reject_all() + + tokens = paragraph_tokens(mgr) + assert tokens[:2] == ["foo", token] + assert "".join(tokens[2:]) == "bar baz" + + +class TestCrossRunReplaceOrder: + """Cross-run replace (multi-run per-run loop) keeps a trailing tab in place.""" + + def test_tab_stays_between_replacement_and_trailing_text(self, temp_xml): + xml_path = temp_xml("alpha betagamma") + mgr = _make_manager(xml_path) + + mgr.replace_text("alpha beta", "X") + assert paragraph_tokens(mgr) == ["DEL(alpha )", "DEL(beta)", "INS(X)", "TAB", "gamma"] + + mgr.accept_all() + assert paragraph_tokens(mgr) == ["X", "TAB", "gamma"] + + +class TestMixedStateDeleteOrder: + """A delete spanning regular text and an own routes through + _delete_regular_segment; the tab must stay between "foo" and the del.""" + + def _fixture(self, temp_xml) -> Path: + return temp_xml( + "foobar" + f'' + "qux" + ) + + def test_pending_and_accepted(self, temp_xml): + mgr = _make_manager(self._fixture(temp_xml)) + + mgr.suggest_deletion("barqux") + assert paragraph_tokens(mgr) == ["foo", "TAB", "DEL(bar)"] + + mgr.accept_all() + assert paragraph_tokens(mgr) == ["foo", "TAB"] + + def test_rejected(self, temp_xml): + mgr = _make_manager(self._fixture(temp_xml)) + + mgr.suggest_deletion("barqux") + mgr.reject_all() + # The own pending "qux" insertion was consumed in place; rejecting + # restores only the tracked deletion of "bar". + assert paragraph_tokens(mgr) == ["foo", "TAB", "bar"] + + def test_multi_wt_regular_segment_keeps_node_order(self, temp_xml): + """A regular segment spanning three w:t nodes (plus an empty sibling) + wraps each node's matched text in its own , in order.""" + xml_path = temp_xml( + "abcdef" + f'' + "gh" + ) + mgr = _make_manager(xml_path) + + mgr.suggest_deletion("bcdefgh") + assert paragraph_tokens(mgr) == ["a", "DEL(b)", "DEL(cd)", "DEL(ef)"] + + mgr.accept_all() + assert paragraph_tokens(mgr) == ["a"] + + +class TestInsertAroundTab: + """Tracked insert lands on the correct side of a mid-run tab.""" + + def test_insert_after_text_precedes_tab(self, temp_xml): + xml_path = temp_xml("foobar") + mgr = _make_manager(xml_path) + + mgr.insert_text_after("foo", "X") + assert paragraph_tokens(mgr) == ["foo", "INS(X)", "TAB", "bar"] + + def test_insert_before_text_follows_tab(self, temp_xml): + xml_path = temp_xml("foobar") + mgr = _make_manager(xml_path) + + mgr.insert_text_before("bar", "X") + assert paragraph_tokens(mgr) == ["foo", "TAB", "INS(X)", "bar"] + + +class TestEmptySiblingWt: + """Empty sibling w:t nodes are dropped from rebuilt runs, never emitted + as empty runs, while the surrounding order is preserved.""" + + FIXTURE = "foobar" + + def test_replace_skips_empty_sibling(self, temp_xml): + mgr = _make_manager(temp_xml(self.FIXTURE)) + mgr.replace_text("bar", "qux") + assert paragraph_tokens(mgr) == ["foo", "TAB", "DEL(bar)", "INS(qux)"] + + def test_delete_skips_empty_sibling(self, temp_xml): + mgr = _make_manager(temp_xml(self.FIXTURE)) + mgr.suggest_deletion("bar") + assert paragraph_tokens(mgr) == ["foo", "TAB", "DEL(bar)"] + + def test_insert_skips_empty_sibling(self, temp_xml): + mgr = _make_manager(temp_xml(self.FIXTURE)) + mgr.insert_text_after("foo", "X") + assert paragraph_tokens(mgr) == ["foo", "INS(X)", "TAB", "bar"] + + +class TestFieldCharOrder: + """Field characters (highest-risk reordering per ISSUES.md) keep their + begin/instr/end sequence intact and in place.""" + + def test_field_children_stay_in_order(self, temp_xml): + xml_path = temp_xml( + "a" + 'PAGE' + 'b' + ) + mgr = _make_manager(xml_path) + + mgr.replace_text("a", "A") + assert paragraph_tokens(mgr) == ["DEL(a)", "INS(A)", "FLD(begin)", "INSTR", "FLD(end)", "b"] + + +class TestTextboxNoDuplication: + """w:t nodes nested inside a drawing's text box must stay inside the + drawing — not be re-emitted as top-level runs (text duplication).""" + + def test_replace_outside_textbox_keeps_boxed_text_once(self, temp_xml): + xml_path = temp_xml( + "x" + "boxed" + "y" + ) + mgr = _make_manager(xml_path) + + mgr.replace_text("x", "X") + + paragraph = mgr.editor.dom.getElementsByTagName("w:p")[0] + assert paragraph.toxml().count("boxed") == 1 + assert paragraph_tokens(mgr) == ["DEL(x)", "INS(X)", "DRAWING", "y"] + drawings = mgr.editor.dom.getElementsByTagName("w:drawing") + assert len(drawings) == 1 + assert "boxed" in drawings[0].toxml() From 08aed7fb7c11babcef6182779b0e59fb33a78d34 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Tue, 14 Jul 2026 16:46:32 +0200 Subject: [PATCH 2/2] =?UTF-8?q?style:=20review=20fixes=20=E2=80=94=20type?= =?UTF-8?q?=20render=5Fwt=20callback,=20unify=20closure=20param=20name,=20?= =?UTF-8?q?fixture=20docstring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-reviewer pass verdicts: correctness/dead-code/codex CLEAN; three cosmetic findings confirmed by red-team meta-review and applied: - annotate rebuild_run_fragments' callback as Callable[[Element], list[str]] - rename the _delete_regular_segment closure param wt_node -> wt to match the four sibling render_wt closures - add the conventional temp_xml fixture docstring in test_run_child_order.py --- docx_editor/track_changes.py | 8 ++++---- docx_editor/xml_editor.py | 2 +- tests/test_run_child_order.py | 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index e73daba..f6c891c 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -1480,16 +1480,16 @@ def _delete_regular_segment(self, positions: list) -> tuple[int, Element | None] # Render ALL w:t nodes in this run, preserving unmatched ones. # Keyword-only defaults bind this iteration's state (B023). - def render_wt(wt_node, *, run_info=run_info, run_rPr=rPr_xml, node_items=node_items, rid=rid) -> list[str]: + def render_wt(wt, *, run_info=run_info, run_rPr=rPr_xml, node_items=node_items, rid=rid) -> list[str]: fragments: list[str] = [] - if id(wt_node) not in run_info["nodes"]: + if id(wt) not in run_info["nodes"]: # Unmatched sibling — preserve as-is - wt_text = self._get_node_text(wt_node) + wt_text = self._get_node_text(wt) if wt_text: fragments.append(f"{run_rPr}{_escape_xml(wt_text)}") return fragments - ng = run_info["nodes"][id(wt_node)] + ng = run_info["nodes"][id(wt)] node_text = self._get_node_text(ng["node"]) first_offset = ng["first"] last_offset = ng["last"] diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index 5bf1967..6366448 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -612,7 +612,7 @@ def get_text_node_data(elem) -> str: return "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE) -def rebuild_run_fragments(run, rPr_xml: str, render_wt) -> list[str]: +def rebuild_run_fragments(run, rPr_xml: str, render_wt: Callable[[Element], list[str]]) -> list[str]: """Rebuild ``run``'s children as XML fragments, preserving document order. Iterates *direct* children of ``run`` (not descendants, so ``w:t`` nodes diff --git a/tests/test_run_child_order.py b/tests/test_run_child_order.py index 7e47b1a..2e2c459 100644 --- a/tests/test_run_child_order.py +++ b/tests/test_run_child_order.py @@ -20,6 +20,8 @@ @pytest.fixture def temp_xml(tmp_path): + """Fixture that returns a function to create temp XML files.""" + def _create_xml(body_xml: str) -> Path: xml = f'{body_xml}' xml_path = tmp_path / "test_doc.xml"