diff --git a/docling_core/transforms/serializer/common.py b/docling_core/transforms/serializer/common.py index 59f521cf..b452a70b 100644 --- a/docling_core/transforms/serializer/common.py +++ b/docling_core/transforms/serializer/common.py @@ -44,6 +44,7 @@ FloatingItem, Formatting, FormItem, + GroupItem, InlineGroup, KeyValueItem, ListGroup, @@ -79,6 +80,22 @@ class _PageBreakSerResult(SerializationResult): node: _PageBreakNode +def _page_break_ref(prev_page: int, next_page: int) -> str: + """Build the self_ref identifying a page break by the boundary it closes. + + Keying on the boundary rather than on a running counter is what lets the + ``self_ref`` dedup in ``get_parts()`` collapse the duplicate a group produces + when it spans a boundary: ``get_parts()`` re-enters for every group and shares + ``visited`` with the recursion, so the nested scope and the root scope both see + the same transition. A counter is scope-local -- the nested scope restarts at 0 -- + so the two scopes would disagree on the ref and the duplicate would surface. + + The result satisfies the ``^#(?:/([\\w-]+)(?:/(\\d+))?)?$`` ref validator, since + ``[\\w-]+`` accepts dashes. + """ + return f"#/pb-{prev_page}-{next_page}" + + def _iterate_items( doc: DoclingDocument, layers: Optional[set[ContentLayer]], @@ -89,7 +106,6 @@ def _iterate_items( ) -> Iterable[tuple[NodeItem, int]]: my_visited: set[str] = visited if visited is not None else set() prev_page_nr: Optional[int] = None - page_break_i = 0 for item, lvl in doc.iterate_items( root=node, with_groups=True, @@ -97,7 +113,7 @@ def _iterate_items( traverse_pictures=traverse_pictures, ): if add_page_breaks: - if isinstance(item, ListGroup | InlineGroup) and item.self_ref not in my_visited: + if isinstance(item, GroupItem) and item.self_ref not in my_visited: # if group starts with new page, yield page break before group node my_visited.add(item.self_ref) for it, _ in _iterate_items( @@ -113,12 +129,22 @@ def _iterate_items( if prev_page_nr is not None and page_no > prev_page_nr: yield ( _PageBreakNode( - self_ref=f"#/pb/{page_break_i}", + self_ref=_page_break_ref(prev_page_nr, page_no), prev_page=prev_page_nr, next_page=page_no, ), lvl, ) + # Advance past the boundary we just emitted, as the DocItem + # branch below does. Without this, the group's first child + # re-emits the same boundary and only the self_ref dedup in + # get_parts() hides the duplicate. + # + # Unlike that branch this does not need to seed prev_page_nr + # when it is still None: the group's children are re-yielded + # at this level anyway, so the DocItem branch seeds it from + # the first one. + prev_page_nr = page_no break elif isinstance(item, DocItem) and item.prov: page_no = item.prov[0].page_no @@ -126,13 +152,12 @@ def _iterate_items( if prev_page_nr is not None: # close previous range yield ( _PageBreakNode( - self_ref=f"#/pb/{page_break_i}", + self_ref=_page_break_ref(prev_page_nr, page_no), prev_page=prev_page_nr, next_page=page_no, ), lvl, ) - page_break_i += 1 prev_page_nr = page_no yield item, lvl diff --git a/test/test_serialization.py b/test/test_serialization.py index b6bd4b1d..e8670244 100644 --- a/test/test_serialization.py +++ b/test/test_serialization.py @@ -7,7 +7,12 @@ import pytest -from docling_core.transforms.serializer.common import _DEFAULT_LABELS +from docling_core.transforms.serializer.common import ( + _DEFAULT_LABELS, + _iterate_items, + _PageBreakNode, +) +from docling_core.transforms.serializer.doctags import DocTagsDocSerializer from docling_core.transforms.serializer.html import ( HTMLDocSerializer, HTMLMetaSerializer, @@ -25,13 +30,15 @@ from docling_core.transforms.serializer.webvtt import WebVTTDocSerializer, WebVTTParams from docling_core.transforms.visualizer.layout_visualizer import LayoutVisualizer from docling_core.types.doc import DoclingDocument -from docling_core.types.doc.base import ImageRefMode +from docling_core.types.doc.base import BoundingBox, CoordOrigin, ImageRefMode, Size +from docling_core.types.doc.common.reference import ProvenanceItem from docling_core.types.doc.document import ( BaseMeta, CharSpan, DescriptionAnnotation, EntitiesMetaField, EntityMention, + GroupLabel, LanguageMetaField, PictureClassificationMetaField, PictureClassificationPrediction, @@ -1103,3 +1110,242 @@ def test_html_meta_emits_xhtml_compatible_attributes(): assert 'data-meta-name="entities"' in html_out # Output must be parseable by a strict XML parser. ET.fromstring(html_out) + + +# =============================== +# Page break positioning (#705) +# =============================== + + +def _pb_prov(page_no: int) -> ProvenanceItem: + """Provenance on a given page; the bbox/charspan are irrelevant here.""" + return ProvenanceItem( + page_no=page_no, + bbox=BoundingBox.from_tuple((1, 2, 3, 4), origin=CoordOrigin.BOTTOMLEFT), + charspan=(0, 1), + ) + + +def _pb_doc(n_pages: int) -> DoclingDocument: + doc = DoclingDocument(name="page-breaks") + for page_no in range(1, n_pages + 1): + doc.add_page(page_no=page_no, size=Size(width=100, height=100)) + return doc + + +_PB = "" + + +def _pb_render(doc: DoclingDocument) -> str: + return ( + MarkdownDocSerializer( + doc=doc, + params=MarkdownParams(page_break_placeholder=_PB), + ) + .serialize() + .text + ) + + +def _pb_sections(md: str) -> list[list[str]]: + """Split rendered markdown into per-page sections of non-empty lines.""" + return [[line for line in section.splitlines() if line.strip()] for section in md.split(_PB)] + + +def test_md_page_break_precedes_group_starting_on_new_page(): + """A group whose children start on a new page gets the break *before* it. + + Regression test for #705: the break used to be emitted after the group, so the + group's content was attributed to the previous page. + """ + doc = _pb_doc(2) + doc.add_text(label=DocItemLabel.TEXT, text="Page 1 text", prov=_pb_prov(1)) + group = doc.add_group(label=GroupLabel.KEY_VALUE_AREA, name="kv") + doc.add_text(label=DocItemLabel.TEXT, text="KV child", prov=_pb_prov(2), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="Page 2 tail", prov=_pb_prov(2)) + + sections = _pb_sections(_pb_render(doc)) + + assert len(sections) == 2 + assert sections[0] == ["Page 1 text"] + assert sections[1] == ["KV child", "Page 2 tail"] + + +def test_md_page_break_per_slide_groups(): + """Each slide-like group lands in its own section, with no trailing break. + + Mirrors the PowerPoint backend shape reported in #705, where every slide's + content is wrapped in its own group. The placeholder *count* was already + correct before the fix (N-1); only the positions were wrong, so this asserts + positions rather than a count alone. + """ + n_slides = 5 + doc = _pb_doc(n_slides) + for page_no in range(1, n_slides + 1): + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name=f"slide-{page_no}") + doc.add_text( + label=DocItemLabel.TEXT, + text=f"Slide {page_no}", + prov=_pb_prov(page_no), + parent=group, + ) + + md = _pb_render(doc) + sections = _pb_sections(md) + + assert md.count(_PB) == n_slides - 1 + assert sections == [[f"Slide {page_no}"] for page_no in range(1, n_slides + 1)] + + +@pytest.mark.parametrize("with_group", [False, True]) +@pytest.mark.parametrize("gap_at", ["none", "start", "middle", "end"]) +def test_md_page_break_positions_matrix(with_group: bool, gap_at: str): + """Every page's text must land in its own section, group or not, gap or not. + + The matrix is what catches position bugs that a placeholder count cannot: a + document can carry the right number of breaks while attributing content to the + wrong page. + + Note the gap cases pin today's semantics, where a multi-page gap yields a single + boundary rather than one per page: with pages [1, 2, 4] this expects three + sections, not a ``pb-3-4`` as well. #466 / #472 would change that, and would need + these expectations updated along with them. + """ + pages = [1, 2, 3, 4] + if gap_at == "start": + pages = [2, 3, 4] + elif gap_at == "middle": + pages = [1, 2, 4] + elif gap_at == "end": + pages = [1, 2, 3] + + doc = _pb_doc(4) + for page_no in pages: + if with_group: + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name=f"group-{page_no}") + doc.add_text( + label=DocItemLabel.TEXT, + text=f"Text {page_no}", + prov=_pb_prov(page_no), + parent=group, + ) + else: + doc.add_text(label=DocItemLabel.TEXT, text=f"Text {page_no}", prov=_pb_prov(page_no)) + + sections = _pb_sections(_pb_render(doc)) + + # One section per page that carries content, in order, each holding only its + # own text -- no page's content bleeds into a neighbouring section. + assert sections == [[f"Text {page_no}"] for page_no in pages] + + +@pytest.mark.parametrize("empty_pages", [1, 2]) +def test_md_page_break_group_straddling_empty_pages(empty_pages: int): + """A group whose children straddle empty pages keeps each child in its section.""" + first, second = 1, 2 + empty_pages + doc = _pb_doc(second + 1) + doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(first)) + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name="straddler") + doc.add_text(label=DocItemLabel.TEXT, text="Child A", prov=_pb_prov(first + 1), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="Child B", prov=_pb_prov(second), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="Outro", prov=_pb_prov(second + 1)) + + sections = _pb_sections(_pb_render(doc)) + + assert sections == [["Intro"], ["Child A"], ["Child B"], ["Outro"]] + + +def test_md_page_break_adjacent_groups_with_gap(): + """Two adjacent groups separated by an empty page stay in their own sections.""" + doc = _pb_doc(4) + first = doc.add_group(label=GroupLabel.UNSPECIFIED, name="first") + doc.add_text(label=DocItemLabel.TEXT, text="First group", prov=_pb_prov(1), parent=first) + second = doc.add_group(label=GroupLabel.UNSPECIFIED, name="second") + doc.add_text(label=DocItemLabel.TEXT, text="Second group", prov=_pb_prov(3), parent=second) + + sections = _pb_sections(_pb_render(doc)) + + assert sections == [["First group"], ["Second group"]] + + +def test_page_break_boundary_emitted_once_per_transition(): + """No boundary is emitted twice by the iterator, before any dedup can hide it. + + ``get_parts()`` drops nodes whose ``self_ref`` it has already seen, so a + duplicated boundary is invisible in the rendered output. Asserting at the + iterator level keeps the group branch honest: it must advance ``prev_page_nr`` + past the boundary it emits, otherwise the group's first child emits the same + transition again and the output stays correct only by accident. + + This is also why a document corpus cannot stand in for this test. Quoting + @serboor on #705, who ran one: a corpus is "a good net for regressions and a + bad net for this" -- but the two halves of the defect are not equally hidden, + and only one of them is the reason this test exists. + + The *misplacement* does reach the rendered output: across 25 documents, one + 287-page document rendered 7 pairs of consecutive placeholders on the faulty + build -- sections left empty because the break came out after the group -- + where a page-by-page reference had none. A corpus can catch that. + + The *duplicate* underneath it cannot reach the output at all: the same corpus + emitted 732 boundaries of which 722 were distinct, and the markdown contained + exactly 722 placeholders. ``get_parts()``'s dedup collapses the extra one, so + no markdown-level assertion can fail on it by construction -- including on the + two documents that were byte-identical to the reference while their stream was + wrong. Asserting on the stream is what changes that. + """ + doc = _pb_doc(4) + doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1)) + first = doc.add_group(label=GroupLabel.UNSPECIFIED, name="first") + doc.add_text(label=DocItemLabel.TEXT, text="A", prov=_pb_prov(2), parent=first) + doc.add_text(label=DocItemLabel.TEXT, text="B", prov=_pb_prov(2), parent=first) + doc.add_text(label=DocItemLabel.TEXT, text="Middle", prov=_pb_prov(3)) + second = doc.add_group(label=GroupLabel.UNSPECIFIED, name="second") + doc.add_text(label=DocItemLabel.TEXT, text="C", prov=_pb_prov(4), parent=second) + + breaks = [ + node + for node, _ in _iterate_items(doc=doc, layers=None, add_page_breaks=True) + if isinstance(node, _PageBreakNode) + ] + + assert [(b.prev_page, b.next_page) for b in breaks] == [(1, 2), (2, 3), (3, 4)] + # Distinct transitions must stay distinguishable, or get_parts()'s dedup would + # swallow a legitimate break rather than a duplicate one. + assert len({b.self_ref for b in breaks}) == len(breaks) + + +def test_md_page_break_nested_groups(): + """A boundary inside a nested group is emitted before the innermost group. + + This is the one shape where the recursion in ``_iterate_items`` and the + ``my_visited`` set it shares with its caller actually interact: the outer group's + lookahead walks into the inner group, and both scopes see the same transition. + """ + doc = _pb_doc(3) + doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1)) + outer = doc.add_group(label=GroupLabel.UNSPECIFIED, name="outer") + doc.add_text(label=DocItemLabel.TEXT, text="Outer child", prov=_pb_prov(1), parent=outer) + inner = doc.add_group(label=GroupLabel.UNSPECIFIED, name="inner", parent=outer) + doc.add_text(label=DocItemLabel.TEXT, text="Inner child", prov=_pb_prov(2), parent=inner) + doc.add_text(label=DocItemLabel.TEXT, text="Outro", prov=_pb_prov(3)) + + sections = _pb_sections(_pb_render(doc)) + + assert sections == [["Intro", "Outer child"], ["Inner child"], ["Outro"]] + + +def test_page_break_before_group_across_serializers(): + """The fix lives in the shared iterator, so every serializer benefits. + + ``_iterate_items`` backs markdown, LaTeX, DocTags and DocLang alike; asserting on + DocTags too keeps the other consumers from regressing silently. + """ + doc = _pb_doc(2) + doc.add_text(label=DocItemLabel.TEXT, text="Page 1 text", prov=_pb_prov(1)) + group = doc.add_group(label=GroupLabel.KEY_VALUE_AREA, name="kv") + doc.add_text(label=DocItemLabel.TEXT, text="KV child", prov=_pb_prov(2), parent=group) + + doctags = DocTagsDocSerializer(doc=doc).serialize().text + + assert doctags.index("") < doctags.index("KV child")