refactor: polish issue #9 fix (DRY, xml:space guard, focused unit test)

Addresses the three LOW items the previous reviewer pass flagged but
that had been deferred:

* Extract the "concat all TEXT_NODE children" pattern into a single
  module-level helper `xml_editor.get_text_node_data`. Replace the
  four inline copies (build_text_map, add_xml_space_to_t,
  _parse_comment, RevisionManager._get_node_text) with calls into
  it. The method on RevisionManager stays as a thin delegate so
  call sites and any external subclasses remain unchanged.

* `_get_element_text` now treats a whitespace-only TEXT_NODE child
  as content when its parent carries `xml:space="preserve"`, not
  only when it sits alongside a meaningful-text sibling. Closes the
  edge case where a lone-space `<w:t xml:space="preserve"> </w:t>`
  was incorrectly returned as the empty string by `contains=` search.

* New focused unit test for `_set_node_text` asserts the contract
  directly: starting from a multi-TEXT_NODE state, ends with exactly
  one TEXT_NODE carrying the new content. Catches future
  "simplifications" that re-introduce the `firstChild.data =` pattern
  without an integration regression.

* New test covering the `xml:space="preserve"` branch of
  `_get_element_text`.

Author: pablospe

docx_editor/comments.py

@@ -10,7 +10,7 @@
 from pathlib import Path
 
 from .exceptions import CommentError, TextNotFoundError
-from .xml_editor import DocxXMLEditor, _generate_hex_id
+from .xml_editor import DocxXMLEditor, _generate_hex_id, get_text_node_data
 
 # Path to template files
 TEMPLATE_DIR = Path(__file__).parent / "ooxml" / "templates"
@@ -412,12 +412,7 @@ def _parse_comment(self, comment_elem) -> Comment | None:
         except ValueError:
             date = None
 
-        # Extract text content from w:t elements. Concatenate all TEXT_NODE
-        # children — minidom can split a single <w:t> across multiple text
-        # nodes (issue #9), so firstChild.data alone would truncate.
-        text_parts = []
-        for t_elem in comment_elem.getElementsByTagName("w:t"):
-            text_parts.append("".join(c.data for c in t_elem.childNodes if c.nodeType == c.TEXT_NODE))
+        text_parts = [get_text_node_data(t_elem) for t_elem in comment_elem.getElementsByTagName("w:t")]
 
         return Comment(
             id=int(comment_id),

docx_editor/track_changes.py

@@ -27,6 +27,7 @@
     build_text_map,
     compute_paragraph_hash,
     find_in_text_map,
+    get_text_node_data,
 )
 
 
@@ -737,16 +738,10 @@ def _get_non_text_children_xml(self, run) -> str:
     def _get_node_text(self, node) -> str:
         """Get text content of a w:t node by concatenating ALL child text nodes.
 
-        minidom may store the text of a single ``<w:t>`` element across
-        multiple TEXT_NODE children — e.g. when Word documents contain
-        smart quotes (U+2018/U+2019). Reading only ``firstChild.data``
-        would return just the first fragment (issue #9).
+        Thin wrapper around :func:`get_text_node_data` — kept as a method so
+        existing call sites (and external subclasses) don't have to change.
         """
-        text = ""
-        for child in node.childNodes:
-            if child.nodeType == child.TEXT_NODE:
-                text += child.data
-        return text
+        return get_text_node_data(node)
 
     def _set_node_text(self, node, text: str) -> None:
         """Replace all text content of a w:t/w:delText element with ``text``.

docx_editor/xml_editor.py

@@ -131,6 +131,20 @@ def _is_inside_element(node, tag_name: str) -> bool:
     return False
 
 
+def get_text_node_data(elem) -> str:
+    """Concatenate all direct TEXT_NODE children of ``elem`` into one string.
+
+    minidom can split a single ``<w:t>``'s text across multiple TEXT_NODE
+    children — reproducibly when smart quotes (U+2018/U+2019) are present
+    (issue #9). Anywhere code naively read ``elem.firstChild.data`` would
+    only see the first fragment. This helper returns the complete string.
+
+    Does NOT recurse into element children. For recursive extraction across
+    a subtree see ``XMLEditor._get_element_text``.
+    """
+    return "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE)
+
+
 def build_text_map(paragraph) -> TextMap:
     """Build a text map for a paragraph element.
 
@@ -150,11 +164,7 @@ def build_text_map(paragraph) -> TextMap:
             continue
 
         inside_ins = _is_inside_element(node, "w:ins")
-        # Get text content from the text node
-        node_text = ""
-        for child in node.childNodes:
-            if child.nodeType == child.TEXT_NODE:
-                node_text += child.data
+        node_text = get_text_node_data(node)
 
         for i, char in enumerate(node_text):
             text_chars.append(char)
@@ -335,26 +345,28 @@ def find_all_nodes(
     def _get_element_text(self, elem) -> str:
         """Recursively extract all text content from an element.
 
-        Skips whitespace-only TEXT_NODEs only when they sit between element
-        siblings (i.e. pretty-print indentation). When an element holds only
-        TEXT_NODE children — as a ``<w:t>`` split across multiple text nodes
-        by minidom does (issue #9) — every fragment is preserved, including
-        ones that happen to be pure whitespace.
+        A whitespace-only TEXT_NODE child is treated as content (rather than
+        pretty-print indentation) when any of:
+          * ``elem`` carries ``xml:space="preserve"`` — the document explicitly
+            marks its whitespace significant.
+          * Another TEXT_NODE sibling has non-whitespace content — the lone
+            whitespace is a fragment of a split text node (issue #9).
+        Otherwise the whitespace TEXT_NODE is discarded as inter-element
+        formatting.
 
         Args:
             elem: DOM element to extract text from
 
         Returns:
             Concatenated text content
         """
-        # A whitespace-only TEXT_NODE is "real content" when it sits alongside
-        # a non-whitespace TEXT_NODE sibling (the smart-quote split case).
-        # Otherwise it's pretty-print indentation and should be skipped.
-        has_meaningful_text_sibling = any(n.nodeType == n.TEXT_NODE and n.data.strip() for n in elem.childNodes)
+        preserve_whitespace = elem.getAttribute("xml:space") == "preserve" or any(
+            n.nodeType == n.TEXT_NODE and n.data.strip() for n in elem.childNodes
+        )
         text_parts = []
         for node in elem.childNodes:
             if node.nodeType == node.TEXT_NODE:
-                if not node.data.strip() and not has_meaningful_text_sibling:
+                if not node.data.strip() and not preserve_whitespace:
                     continue
                 text_parts.append(node.data)
             elif node.nodeType == node.ELEMENT_NODE:
@@ -618,10 +630,7 @@ def add_comment_extensible_date(elem) -> None:
 
         def add_xml_space_to_t(elem) -> None:
             # Add xml:space="preserve" to w:t if text has leading/trailing whitespace.
-            # Concatenate all TEXT_NODE children — minidom can split a single
-            # <w:t> across multiple text nodes (issue #9), so firstChild.data
-            # alone would miss whitespace that sits in a later fragment.
-            text = "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE)
+            text = get_text_node_data(elem)
             if text and (text[0].isspace() or text[-1].isspace()):
                 if not elem.hasAttribute("xml:space"):
                     elem.setAttribute("xml:space", "preserve")

tests/test_track_changes.py

@@ -1681,6 +1681,23 @@ class TestMultiTextNodeWtElements:
     # When paragraph= is passed, control routes through _replace_across_nodes
     # which uses _get_node_text and avoids the bug.
 
+    def test_set_node_text_consolidates_split_nodes(self, clean_workspace):
+        """Direct contract test for _set_node_text: starts from a multi-TEXT_NODE
+        state, ends with exactly one TEXT_NODE carrying the full new content.
+        Guards against future "simplifications" that would re-introduce the
+        firstChild.data assignment pattern."""
+        doc = Document.open(clean_workspace)
+        wt = _split_wt_text_nodes(doc, "quick brown fox")
+        text_nodes_before = [c for c in wt.childNodes if c.nodeType == c.TEXT_NODE]
+        assert len(text_nodes_before) > 1
+
+        doc._revision_manager._set_node_text(wt, "consolidated")
+
+        text_nodes_after = [c for c in wt.childNodes if c.nodeType == c.TEXT_NODE]
+        assert len(text_nodes_after) == 1
+        assert text_nodes_after[0].data == "consolidated"
+        doc.close()
+
     def test_replace_without_paragraph_arg_succeeds(self, clean_workspace):
         doc = Document.open(clean_workspace)
         wt = _split_wt_text_nodes(doc, "quick brown fox")

tests/test_xml_editor.py

@@ -263,6 +263,22 @@ def test_get_element_text_preserves_split_whitespace(self, tmp_path):
 
         assert editor._get_element_text(item) == "Library Bookshelves"
 
+    def test_get_element_text_honors_xml_space_preserve(self, tmp_path):
+        """A lone whitespace TEXT_NODE inside an element marked
+        xml:space="preserve" is significant content and must be returned.
+
+        Without the attribute it would be treated as pretty-print indent and
+        stripped — matching the pre-existing behavior."""
+        xml_content = '<?xml version="1.0"?><root><keep xml:space="preserve"> </keep><drop> </drop></root>'
+        xml_path = tmp_path / "preserve.xml"
+        xml_path.write_text(xml_content)
+        editor = XMLEditor(xml_path)
+
+        keep = editor.dom.getElementsByTagName("keep")[0]
+        drop = editor.dom.getElementsByTagName("drop")[0]
+        assert editor._get_element_text(keep) == " "
+        assert editor._get_element_text(drop) == ""
+
 
 class TestXMLEditorGetNextRid:
     """Tests for XMLEditor.get_next_rid method."""