Repository navigation
Conversation
…m XML The Word backend already reads runs inside smart tags, custom XML, tracked insertions and simple fields, and the paragraphs of block-level content controls. It dropped three more wrappers of ordinary content: - w:moveTo, the new position of text moved with track changes on, so moved text disappeared from the output altogether (the old position, w:moveFrom, is correctly left out); - w:dir and w:bdo, bidirectional embeddings and overrides around runs; - block-level w:customXml around body paragraphs and tables. The run recursion now includes moveTo, dir and bdo, and block-level custom XML is walked like the content of a block-level content control. Signed-off-by: L4XB <lukas.buck@e-mail.de>
|
✅ DCO Check Passed Thanks @L4XB, all your commits are properly signed off. 🎉 |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
chrikrah
left a comment
There was a problem hiding this comment.
Simply correct. Here is the falsification evidence the body does not carry.
Your tests pin the change, and they also catch a plausible wrong variant. Reverting production and keeping the tests fails exactly the two new ones:
2 failed, 65 passed, 1 skipped
And adding "moveFrom" to the skip set, which would resurrect moved-away text, fails test_runs_in_moves_and_bidirectional_spans_are_read. That is the half most likely to be got wrong, and it is pinned.
No committed ground truth needs regenerating. No .docx under tests/data contains any affected tag:
customXml -> 0 files moveTo -> 0 files moveFrom -> 0 files
w:bdo -> 0 files smartTag -> 0 files
w:dir -> 1 file: tests/data/docx/sources/textbox.docx [false positive]
That last hit is the attribute w:directFormattingOnRuns="1", not a <w:dir> element.
Seven probes at head, checking for leakage, double emission, nesting, and that deletions stay dropped:
customXmlPr leak -> ['Before', 'In custom XML', 'After'] no placeholder or attribute leak
nested block customXml -> ['Before', 'deep']
moveTo nested in ins -> ['A B']
w:del dropped -> ['keep tail']
empty block customXml -> ['Before', 'After']
One hole left, and it is the same one-line pattern rather than something this introduced. w:customXml wrapping a w:tr or a w:tc still drops the whole row or cell, identically at base and head:
customXml around w:tr -> tables: [['R2C1', 'R2C2']] row R1C1/R1C2 gone
customXml around w:tc -> tables: [['plain cell']] 'CX cell' gone
_walk_linear is what got the fix. _handle_tables iterates w:tr and w:tc directly and never sees through the wrapper. Since the title says block-level content, it is worth deciding whether that belongs here or in its own change.
ruff check and ruff format --check are clean at head.
|
HI, Verified on a patched copy: body moveTo is fixed (a Word-made move, 12_move_word_real.docx, gives the text once). moveTo inside a table cell is not fixed by this PR alone (11_moveto_incell.docx), because plain cells go through _Cell.text and not _get_children_recursive. It is fixed only combined with #4455. 12_move_word_real.docx |
|
@chrikrah thanks for the probes and the mutation check. On the hole you found: you've since filed it as #4382, and #4393 fixes it, row and cell wrappers alike. So I've kept it out of this PR, which stays on run- and paragraph-level content. @el-belgounetos thanks for checking with real Word files. Your reading is right. The two changes touch different code and merge cleanly in either order. Once one of them lands, I'm happy to add a cell-level |
…di-runs Signed-off-by: L4XB <lukas.buck@e-mail.de> # Conflicts: # docling/backend/msword_backend.py
|
Merged The four |
AI triage (advisory)Checked commit The PR makes the Word backend read text in Topic labels added: CI safety: no concern found. A maintainer must still approve CI. 1 related pull request(s) in the same code
The |
There was a problem hiding this comment.
AI first review (advisory): no blocker found
This review covers the full diff at 6b4d22e686. It read the code and did not run it.
The run-level change (moveTo, dir, bdo) is small and its test pins the moveFrom exclusion. The block-level customXml branch skips the textbox guard that sdt and tbl have. A textbox in custom XML is therefore read before the other paragraphs in the same wrapper. Check this point before merge.
Findings outside the diff lines
-
suggestion
docling/backend/msword_backend.py:1188: BlockcustomXmlis not in the textbox skip set, so textbox text can move out of reading order
The comment above this guard says that container elements that walk their own children must be skipped. Otherwise the descendant textbox XPath extracts the textboxes before the container's children are walked. This PR makescustomXmlsuch a container (elif tag_name in ("sdt", "customXml")), but the guard still lists onlytblandsdt.Trigger: a block-level
w:customXmlthat holds a paragraph, then a paragraph with a textbox.Result: the XPath runs on the
customXmlelement first. It emits the textbox content, then the children are walked. The textbox text lands before the earlier paragraph of the wrapper instead of after it. Theprocessed_textbox_elementscheck prevents duplicates, but it does not keep the order.Fix: add
"customXml"to the set at this line. Add a small test with a textbox in custom XML if you want to pin it. The two new tests do not cover textboxes.
A maintainer makes the merge decision. This review never approves the pull request.
chrikrah
left a comment
There was a problem hiding this comment.
@L4XB blocking: block-level customXml now walks its children but is missing from the textbox skip set at msword_backend.py:1188, so a textbox inside the wrapper comes out ahead of the paragraph before it. The github-actions review on this head flagged it, and it reproduces:
# 6b4d22e exported; body = START, [A, paragraph holding a w:txbxContent "TB", C], END
$ PYTHONPATH=$PWD python probe.py plain p_plain.docx
plain ['START', 'A', 'TB', 'C', 'END']
$ PYTHONPATH=$PWD python probe.py cx p_cx.docx # same three wrapped in w:customXml
cx ['START', 'TB', 'A', 'C', 'END']
# with {"tbl", "sdt", "customXml"} at line 1188
cx ['START', 'A', 'TB', 'C', 'END']
# your diff (0e54294..6b4d22e) applied to main d01c9c0
$ PYTHONPATH=$PWD python -m pytest tests/test_backend_msword*.py -q
1 failed, 296 passed in 79.84s
# the failure is test_e2e_docx_conversions on docx_list_east_asian_num_fmt.docx; plain d01c9c0 fails it too
"moveTo" removed from the descend set -> test_runs_in_moves_and_bidirectional_spans_are_read FAILED
Everything else holds, and the diff still applies cleanly after the 20 commits main gained. Could you add "customXml" to that set with a textbox-in-wrapper test? I'd approve on that push, and then dolfim-ibm could take it into the next docx batch.
The Word backend already reads the runs inside smart tags, custom XML, tracked insertions and simple fields (
_iter_paragraph_content), and the paragraphs of block-level content controls (_walk_linear). It still drops three wrappers of ordinary content:w:moveTo: the new position of text that was moved with track changes on. Word writesw:moveFromat the old position andw:moveToat the new one.w:moveFromis correctly left out, but becausew:moveTois not read either, moved text disappears from the output altogether.w:dirandw:bdo: a bidirectional embedding or override around runs, used in right-to-left text.w:customXml: custom XML markup around body paragraphs and tables. Its inline form is already read.A probe document with one marker in each place, through
DocumentConverter:mainw:moveTo)w:moveFrom), should stay outw:dirw:bdow:customXmlChange:
moveTo,dirandbdojoin the set_get_children_recursivelooks through._walk_linearwalks a block-levelcustomXmlthe way it walks a content control'ssdtContent. The branch sits next to thesdtone, before the image branches, for the reason the existing comment gives.Tests (
tests/test_backend_msword.py):test_runs_in_moves_and_bidirectional_spans_are_read: a move-from/move-to pair, aw:dirspan and aw:bdospan. It checks the exact text items, so the moved-away text must stay out.test_paragraphs_and_tables_in_block_custom_xml_are_read: a paragraph and a two-cell table inside block-level custom XML, between two plain paragraphs.Results:
main. With the change, the fourtest_backend_msword*.pyfiles pass: 76 passed, 1 skipped.moveTo,dirorbdodropped from the set;customXmlbranch removed;moveFromadded to the set.tests/datacontainsw:moveTo,w:dir,w:bdoorw:customXml; I checked all 275word/*.xmlparts. So no reference output changes.ruff checkandruff format --check(0.15.12) are clean.ty checkonmsword_backend.pyreports the same 12 diagnostics as onmain.Checklist: