fix: emit page break before a group whose children start on a new page - #708
fix: emit page break before a group whose children start on a new page#708maxmilian wants to merge 5 commits into
Conversation
The group branch of _iterate_items() only matched ListGroup | InlineGroup, so a plain GroupItem (KEY_VALUE_AREA, and the per-slide groups the PowerPoint backend produces) matched neither it nor the DocItem branch that follows. The boundary was then emitted by the group's first child, placing the break after the group and attributing the group's content to the previous page. The same branch also yielded its break without advancing prev_page_nr, unlike the DocItem branch, so the group's first child re-emitted the same boundary; only the self_ref dedup in get_parts() kept the duplicate out of the output. Broaden the branch to GroupItem, advance prev_page_nr past the emitted boundary, and key _PageBreakNode.self_ref on the boundary rather than on a running counter. The last part is what makes the second safe: get_parts() re-enters for every group and shares visited with the recursion, so a group spanning a boundary is seen by both the nested and the root scope, and a scope-local counter would make the two disagree on the ref once the group branch starts incrementing it. Add position-asserting tests (the reported symptom has the correct placeholder count throughout) plus an iterator-level test that no boundary is emitted twice, which is the only level at which the duplicate is visible.
|
✅ DCO Check Passed Thanks @maxmilian, all your commits are properly signed off. 🎉 |
Merge Protections🔴 1 of 2 protections blocking · waiting on 👀 reviews
🔴 Require two reviewer for test updatesWaiting for
This rule is failing.When test data is updated, we require two reviewers
Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Add the group-inside-a-group case, which is where the recursion in _iterate_items and the my_visited set it shares with its caller actually interact, and a DocTags assertion so the other consumers of the shared iterator cannot regress silently. Both fail without the GroupItem broadening. Extend the iterator-level test to a document with three distinct boundaries, so its uniqueness assertion is no longer trivially true for a single-element list, and note in the matrix test that the gap expectations pin current semantics that docling-project#466 / docling-project#472 would change.
Records the reasoning from docling-project#705: a corpus is a good net for regressions and a bad net for this defect, because real documents rarely place a group across an empty page, so a faulty stream never meets an input that would render differently. The reporter's own latent gap-expansion defect passed both the markdown-level assertions and a 25-document corpus; asserting the property is what caught it. Comment only — no test or source behaviour changes. Signed-off-by: Max Hsu <maxmilian@gmail.com>
The previous wording said a faulty stream "never meets an input that would render differently". Measurements from @serboor on docling-project#705 refute that: across a 25-document corpus, a 287-page document rendered 7 pairs of consecutive placeholders on the faulty build where a page-by-page reference had none. The misplacement is visible in rendered markdown; a corpus can catch it. What a corpus cannot catch is the duplicate underneath it: 732 boundaries emitted, 722 distinct, and exactly 722 placeholders in the output, because get_parts()'s self_ref dedup collapses the extra one before it can render. Two of the affected documents were byte-identical to the reference while their stream was wrong. Both assertions therefore stay, for two different reasons — which is what the docstring now says instead of overstating the first one. Comment only — no test or source behaviour changes. Signed-off-by: Max Hsu <maxmilian@gmail.com>
|
Pushed @serboor's measurements on #705 refuted a claim I had made in the docstring of What a corpus cannot catch is the duplicate underneath: 732 boundaries emitted, 722 distinct, exactly 722 placeholders in the output, because So both assertions in the test stay, for two different reasons, and the docstring now says that instead of overstating the first one. Same reasoning quoted with attribution, as offered on the issue. The rest of his run is on #705: 4 documents change on this branch and they are the four that motivated the issue; three become byte-identical to the reference and the fourth differs by 2 bytes of whitespace that reproduces identically on Still waiting on a second approving review for the Mergify test-data protection. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Fixes #705
What was wrong
_iterate_items()indocling_core/transforms/serializer/common.pyhas two defects around groups, both visible withadd_page_breaks=True:The group branch only matched
ListGroup | InlineGroup. A plainGroupItem—KEY_VALUE_AREAas reported, and the per-slide groups the PowerPoint backend produces — matched neither that branch nor theelif isinstance(item, DocItem)that follows, sinceGroupItemis not aDocItem. No break was emitted at the group, so the boundary was instead emitted by the group's first child, i.e. after the group, attributing the group's content to the previous page.The group branch yielded its break without advancing
prev_page_nr, unlike theDocItembranch. The group's first child therefore re-emitted the same boundary, and only theself_refdedup inget_parts()kept that duplicate out of the output.The reported
.pptxsymptom follows from (1): with each slide in its own group, an N-slide deck still gets N-1 placeholders — the count looks right — but they sit before slides 3…N with one orphan after the last slide, and slides 1 and 2 share a section.The fix
GroupItem, which coversKEY_VALUE_AREA, the per-slide groups, and the deprecatedOrderedList(all subclassGroupItem).prev_page_nrpast the boundary the group branch emits, mirroring theDocItembranch._PageBreakNode.self_refon the boundary (#/pb-{prev}-{next}) rather than on a running counter.That last point is what makes the second change safe.
get_parts()re-enters for every group and sharesvisitedwith the recursion, so a group spanning a boundary is seen by both the nested and the root scope. A counter is scope-local — the nested scope restarts at 0 — so once the group branch starts incrementing it, the two scopes disagree on the ref and the duplicate stops being deduplicated. Keying on the boundary makes the identity independent of traversal order. The ref still satisfies the^#(?:/([\w-]+)(?:/(\d+))?)?$validator, since[\w-]+accepts dashes.Tests
test/test_serialization.pygains a group of page-break tests that assert positions, not just placeholder counts — the reported symptom has the correct count throughout:test_md_page_break_precedes_group_starting_on_new_page— theKEY_VALUE_AREArepro from the issue.test_md_page_break_per_slide_groups— the 5-slide deck shape; each slide gets its own section and there is no trailing break.test_md_page_break_positions_matrix— (no group / group) × (gap at start / middle / end / none), asserting each page's text lands in its own section.test_md_page_break_group_straddling_empty_pages— a group whose children straddle one or two empty pages.test_md_page_break_adjacent_groups_with_gap— two adjacent groups with an empty page between them.test_md_page_break_nested_groups— a group inside a group, the one shape where the recursion and the sharedmy_visitedactually interact.test_page_break_boundary_emitted_once_per_transition— asserts at the iterator level that no boundary is emitted twice, and that distinct transitions stay distinguishable.test_page_break_before_group_across_serializers— the same repro through DocTags, since_iterate_itemsis shared by markdown, LaTeX, DocTags and DocLang.The iterator-level test earns its place because defect (2) is invisible from rendered output: reverting the
prev_page_nradvance leaves every rendering test green, while the iterator emits['#/pb-1-2', '#/pb-1-2']and the dedup hides the duplicate. Verified fail-before / pass-after for each half of the fix independently, reverting one at a time against the final test set:GroupItembroadeningprev_page_nradvanceChecks
Notes
(prev, next), two genuinely distinct breaks sharing a boundary would collapse into one. This is only reachable with non-monotone provenance (e.g. two sibling groups each crossing 1→2). The counter scheme lost the same case — and lost more besides, since a nested scope's first break always collided with the root's first break regardless of boundary — so this is not a regression. Flagging it rather than leaving it to be discovered.doclang.pystill builds_PageBreakNode.self_reffrom a counter in three places. Those nodes never reachget_parts()'s dedup, so there is no bug today, but they could adopt_page_break_reffor consistency. Worth notingdoclang.pyalready dedups document-level breaks keyed on(prev_page, next_page), so boundary-as-identity is an established idiom here.markdown.py/test_serialization.py; this fix lives incommon.py.