Skip to content

fix: preserve run-internal order of tabs/breaks/drawings in tracked edits (ISSUES.md #20) #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pablospe merged 2 commits into from
Jul 14, 2026
Merged

fix: preserve run-internal order of tabs/breaks/drawings in tracked edits (ISSUES.md #20) #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49645c3
fix: preserve run-internal child order in tracked edits (ISSUES.md #20)
pablospe Jul 14, 2026
08aed7f
style: review fixes — type render_wt callback, unify closure param na…
pablospe Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 18 additions & 29 deletions docx_editor/comments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
compute_paragraph_hash,
find_in_text_map,
get_text_node_data,
rebuild_run_fragments,
)

# Path to template files
Expand Down Expand Up @@ -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"<w:r>{rPr_xml}{child.toxml()}</w:r>")
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"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
continue

if is_start_here and is_end_here:
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
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"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(start_marker)
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
parts.append(start_marker)
if matched:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(matched)}</w:t></w:r>")
xml_parts.append(end_marker)
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(matched)}</w:t></w:r>")
parts.append(end_marker)
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
elif is_start_here:
before = wt_text[:start_offset]
after = wt_text[start_offset:]
if before:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(start_marker)
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
parts.append(start_marker)
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
else: # is_end_here only
before = wt_text[: end_offset + 1]
after = wt_text[end_offset + 1 :]
if before:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(end_marker)
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
parts.append(end_marker)
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
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:
Expand Down
184 changes: 88 additions & 96 deletions docx_editor/track_changes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
compute_paragraph_hash,
find_in_text_map,
get_text_node_data,
rebuild_run_fragments,
)


Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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"<w:r>{rPr_xml}{non_text_xml}</w:r>")

# 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"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(
f"<w:del><w:r>{rPr_xml}<w:delText>{_escape_xml(matched)}</w:delText></w:r></w:del>"
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(before)}</w:t></w:r>")
fragments.append(
f"<w:del><w:r>{run_rPr}<w:delText>{_escape_xml(matched)}</w:delText></w:r></w:del>"
)

# Insert replacement after the last deletion
if part_idx + parts_emitted == total_parts:
xml_parts.append(f"<w:ins><w:r>{first_rPr}<w:t>{_escape_xml(replace_with)}</w:t></w:r></w:ins>")
if base_idx + parts_emitted == total_parts:
fragments.append(f"<w:ins><w:r>{first_rPr}<w:t>{_escape_xml(replace_with)}</w:t></w:r></w:ins>")

if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(after)}</w:t></w:r>")
else:
# Unmatched sibling — preserve
wt_text = self._get_node_text(wt)
if wt_text:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
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
Expand Down Expand Up @@ -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"<w:r>{rPr_xml}<w:t>{_escape_xml(node_text[:offset])}</w:t></w:r>")
if node_text[offset:]:
right_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(node_text[offset:])}</w:t></w:r>")
side = right_parts
continue
if tag == "w:t":
wt_text = self._get_node_text(child)
if wt_text:
side.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
continue
side.append(f"<w:r>{rPr_xml}{child.toxml()}</w:r>")

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)
Expand All @@ -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"<w:r>{rPr_xml}{non_text_xml}</w:r>")
xml_parts += [f"<w:r>{rPr_xml}<w:t>{_escape_xml(t)}</w:t></w:r>" for t in left_texts]
left_len = len(xml_parts)
xml_parts += [f"<w:r>{rPr_xml}<w:t>{_escape_xml(t)}</w:t></w:r>" 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 <w:ins> at (edge_node, offset) inside a foreign ins.
Expand Down Expand Up @@ -1477,26 +1476,20 @@ 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"<w:r>{rPr_xml}{non_text_xml}</w:r>")

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, *, run_info=run_info, run_rPr=rPr_xml, node_items=node_items, rid=rid) -> list[str]:
fragments: list[str] = []
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:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
continue
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
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"]
Expand All @@ -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"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(f"<w:del><w:r>{rPr_xml}<w:delText>{_escape_xml(matched)}</w:delText></w:r></w:del>")
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(before)}</w:t></w:r>")
fragments.append(f"<w:del><w:r>{run_rPr}<w:delText>{_escape_xml(matched)}</w:delText></w:r></w:del>")
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(after)}</w:t></w:r>")
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)
Expand Down Expand Up @@ -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])
Expand All @@ -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"<w:r>{rPr_xml}{non_text_xml}</w:r>")

# Emit w:t nodes in document order, matched as <w:del>, 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"<w:r>{rp_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(f"<w:del><w:r>{rp_xml}<w:delText>{_escape_xml(matched)}</w:delText></w:r></w:del>")
fragments.append(f"<w:r>{rp_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
fragments.append(f"<w:del><w:r>{rp_xml}<w:delText>{_escape_xml(matched)}</w:delText></w:r></w:del>")
if after:
xml_parts.append(f"<w:r>{rp_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
elif id(wt) not in matched_node_ids:
fragments.append(f"<w:r>{rp_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
else:
# Unmatched sibling — preserve
wt_text = self._get_node_text(wt)
if wt_text:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
fragments.append(f"<w:r>{run_rPr}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
return fragments

# Emit the run's children in document order (matched w:t as
# <w:del>; 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)
Expand Down Expand Up @@ -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 <w:ins>
# Rebuild the edge run: split its w:t at the offset and wrap text in
# <w:ins>; non-text children (w:tab, w:br, w:drawing, …) stay in place
ins_xml = f"<w:ins><w:r>{rPr_xml}<w:t>{_escape_xml(text)}</w:t></w:r></w:ins>"
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"<w:r>{rPr_xml}{non_text_xml}</w:r>")

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"<w:r>{rPr_xml}<w:t>{_escape_xml(before_text)}</w:t></w:r>")
xml_parts.append(ins_xml)
fragments.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before_text)}</w:t></w:r>")
fragments.append(ins_xml)
if after_text:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after_text)}</w:t></w:r>")
fragments.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after_text)}</w:t></w:r>")
else:
# Preserve unmatched sibling w:t
wt_text = self._get_node_text(wt)
if wt_text:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
fragments.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
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":
Expand Down
Loading
Loading