Skip to content

Commit 5c4be1b

Browse files
DemchaAVDemchaAV
authored
fix(engine): let the keepWithNext lookahead see keepTogether (#464)
* fix(engine): let the keepWithNext lookahead see keepTogether Two pagination rules disagreed. keepWithNext decides by asking whether the heading plus the FIRST LINE of the next block fits the remaining page. A keepTogether block has no first line to break after — it relocates whole. leadingUnitHeight measured the first line anyway, the lookahead concluded it fit and left the heading in place, and compileNode then moved the entire block. The heading was stranded by the very rule meant to protect it. leadingUnitHeight now returns the whole outer height for a node whose keep-together request will be honoured. The condition mirrors keepWhole in compileNode, so a block taller than the page — whose request the compiler ignores — keeps being estimated by its first slice, which is what lets a heading above a page-spanning paragraph or table still relocate. The method needed the page's inner height to make that test; it takes it as a parameter rather than the CompilerState, so a measurement helper stays free of the mutable page cursor. Reproduced before fixing: at 300x400 with 20pt margins and a 200pt body, the heading stranded for every filler height from 120 to 210pt, and never once when the same body could split. The new test sweeps that range rather than sampling it — a single sample passes against the old compiler for most filler heights. It fails on the unfixed engine at filler=120 and passes on the fixed one; the other three cases hold in both directions, so they guard the behaviour this change must not alter. * test(engine): make the page-spanning case actually exercise keepWithNext The page-spanning test pinned a 300pt filler, where the body's first line still fits under the header. Both land on the same page whether or not anything relocates them, so the assertion held against an engine with no keepWithNext at all — it proved nothing about the branch it names. It now builds at 310pt, inside the window where the rule decides the outcome, and asserts both directions: glued with the opt-in, stranded without it. Removing the page-height guard from the fix — the mutation this test exists to catch — turns it red, where the 300pt version passed. The best-effort boundary test asserts an absence and so reads the same as "the opt-in was never applied". It cannot fail for the right reason on its own; its javadoc now says that rather than implying more coverage than it carries. * test(engine): cover the nested keep-together block, correct the changelog scope The lookahead recurses into a wrapper's first child, and nothing checked that the page height reaches that frame: hand the recursive call a bogus height and every existing test stayed green. Only a page-spanning nested child tells "estimate the whole block" apart from "estimate its first slice", so the page-spanning case now runs in both flat and nested form off one helper. That nested shape is what keepEntriesTogether() builds — a plain timeline wrapper whose entries are the atomic ones — so it is a combination users can reach, not a synthetic one. The changelog named surfaces that do not opt into keepTogether: no template preset calls it, and neither does the markdown renderer. Narrowed to what the code supports, with the one shipped opt-in named.
1 parent 843a9bd commit 5c4be1b

3 files changed

Lines changed: 313 additions & 3 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,21 @@
33
All notable changes to GraphCompose are documented here. Versions
44
follow semantic versioning; release dates are ISO 8601.
55

6+
## v2.1.1 — Planned
7+
8+
### Fixed
9+
10+
- **A heading no longer strands above a block that was asked to stay whole.**
11+
`keepWithNext()` decides by asking whether the heading plus the *first line* of
12+
the next block fits, but a `keepTogether()` block has no first line to break
13+
after — it relocates entire. The lookahead measured the first line anyway,
14+
concluded it fit, left the heading in place, and the compiler then moved the
15+
whole block to the next page. It affected any heading above a block that opts
16+
into `keepTogether()` — including a timeline built with
17+
`keepEntriesTogether()`, whose entries are kept whole one level down. A block
18+
taller than a page still anchors by its first line, because its keep-together
19+
request is ignored anyway.
20+
621
## v2.1.0 — 2026-07-26
722

823
### Highlights

‎core/src/main/java/com/demcha/compose/document/layout/LayoutCompiler.java‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,8 @@ private void compileComposite(PreparedNode<DocumentNode> prepared,
327327
+ toMargin(children.get(k).margin()).vertical()
328328
+ layoutSpec.spacing();
329329
}
330-
needed += leadingUnitHeight(children.get(runEnd), childRegionWidth, prepareContext);
330+
needed += leadingUnitHeight(children.get(runEnd), childRegionWidth, prepareContext,
331+
state.activeInnerHeight());
331332
// Relocate only when the run + first line genuinely fits on a fresh
332333
// page (EPS, not CAPACITY_TOLERANCE): a run that would still overflow
333334
// the next page by a hair should stay put rather than strand there and
@@ -1045,14 +1046,29 @@ private double childAvailableWidth(double regionWidth, DocumentNode node) {
10451046
* @param node the following sibling whose first line anchors the heading
10461047
* @param regionWidth the content-region width the sibling is measured at
10471048
* @param prepareContext measurement context
1049+
* @param pageInnerHeight the active page's inner height, used to tell a honoured
1050+
* keep-together request from one the compiler will ignore
10481051
* @return the height consumed down to the bottom of the node's first flow line
10491052
*/
1050-
private double leadingUnitHeight(DocumentNode node, double regionWidth, PrepareContext prepareContext) {
1053+
private double leadingUnitHeight(DocumentNode node, double regionWidth, PrepareContext prepareContext,
1054+
double pageInnerHeight) {
10511055
Margin margin = toMargin(node.margin());
10521056
Padding padding = toPadding(node.padding());
10531057
PreparedNode<DocumentNode> prepared = prepareForRegionWidth(prepareContext, node, regionWidth);
10541058
double topReservation = margin.top() + padding.top();
10551059

1060+
// A node that asked to be kept whole has no first line to seam after: when
1061+
// the request will actually be honoured, its whole outer height IS its
1062+
// leading unit, because it relocates entire rather than splitting. Mirrors
1063+
// the keepWhole test in compileNode — a node taller than the page has its
1064+
// keep-together ignored and flows, so the first-slice estimate stays right
1065+
// there. Without this a heading above a boxed callout is told "one line
1066+
// fits", stays put, and is stranded when the whole box moves.
1067+
double outerHeight = prepared.measureResult().height() + margin.vertical();
1068+
if (node.keepTogether() && outerHeight <= pageInnerHeight + CAPACITY_TOLERANCE) {
1069+
return outerHeight;
1070+
}
1071+
10561072
if (prepared.isComposite()) {
10571073
CompositeLayoutSpec layoutSpec = prepared.requireCompositeLayout();
10581074
@SuppressWarnings("unchecked")
@@ -1065,7 +1081,8 @@ private double leadingUnitHeight(DocumentNode node, double regionWidth, PrepareC
10651081
}
10661082
double availableWidth = childAvailableWidth(regionWidth, node);
10671083
double innerRegionWidth = Math.max(0.0, availableWidth - padding.horizontal());
1068-
return topReservation + leadingUnitHeight(children.get(0), innerRegionWidth, prepareContext);
1084+
return topReservation + leadingUnitHeight(children.get(0), innerRegionWidth, prepareContext,
1085+
pageInnerHeight);
10691086
}
10701087

10711088
// The leading unit of a splittable leaf is its first slice: a paragraph's

‎qa/src/test/java/com/demcha/compose/document/dsl/SectionKeepWithNextKeepTogetherTest.java‎

Lines changed: 278 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,278 @@
1+
package com.demcha.compose.document.dsl;
2+
3+
import com.demcha.compose.GraphCompose;
4+
import com.demcha.compose.document.api.DocumentSession;
5+
import com.demcha.compose.document.layout.LayoutGraph;
6+
import com.demcha.compose.document.layout.PlacedNode;
7+
import com.demcha.compose.document.style.DocumentColor;
8+
import com.demcha.compose.document.style.DocumentInsets;
9+
import org.junit.jupiter.api.Test;
10+
11+
import static org.assertj.core.api.Assertions.assertThat;
12+
13+
/**
14+
* Verifies that the {@code keepWithNext()} lookahead accounts for a following
15+
* sibling that asked to be kept whole.
16+
*
17+
* <p>The two rules are independent mechanisms that used to disagree. The
18+
* lookahead asks "does the heading plus the next block's <em>first line</em>
19+
* fit?", while a {@code keepTogether()} block has no first line to seam after —
20+
* it relocates entire. So the lookahead answered "yes, one line fits", left the
21+
* heading in place, and the compiler then moved the whole block to the next
22+
* page, stranding the heading it was supposed to protect.</p>
23+
*
24+
* <p>The window is narrow and depends on the filler height, which is why this
25+
* sweeps rather than pinning a single case: at 300&times;400 with 20pt margins
26+
* and a 200pt body the orphan appeared for every filler from 120 to 210pt and
27+
* nowhere else. A single sample would pass against the buggy compiler for most
28+
* choices of filler.</p>
29+
*
30+
* <p>Assertions read the page of the inner content <em>leaf</em>, not the
31+
* section wrapper — see {@link SectionKeepWithNextTest} for why the wrapper's
32+
* start page does not reflect where its content lands.</p>
33+
*/
34+
class SectionKeepWithNextKeepTogetherTest {
35+
36+
private static final DocumentColor GREY = DocumentColor.rgb(220, 220, 220);
37+
private static final DocumentColor INK = DocumentColor.rgb(20, 80, 95);
38+
39+
/** Page 300x400 with 20pt margins leaves 360pt of inner height. */
40+
private static final double PAGE_WIDTH = 300;
41+
private static final double PAGE_HEIGHT = 400;
42+
private static final double INNER_HEIGHT = 360;
43+
44+
private static int page(LayoutGraph graph, String name) {
45+
PlacedNode node = graph.nodes().stream()
46+
.filter(n -> name.equals(n.semanticName()))
47+
.findFirst().orElseThrow();
48+
return node.startPage();
49+
}
50+
51+
/**
52+
* A {@code keepWithNext()} header above a two-part body that fits on a page.
53+
*
54+
* @param bodyKeepTogether whether the body relocates whole instead of splitting
55+
* @param fillerHeight height of the block that pushes the header down the page
56+
* @return the compiled layout graph
57+
*/
58+
private static LayoutGraph build(boolean bodyKeepTogether, double fillerHeight) {
59+
try (DocumentSession document = GraphCompose.document()
60+
.pageSize(PAGE_WIDTH, PAGE_HEIGHT)
61+
.margin(DocumentInsets.of(20))
62+
.create()) {
63+
document.pageFlow().name("Flow").spacing(6)
64+
.addSection("Filler", s -> s.addShape(260, fillerHeight, GREY))
65+
.addSection("Header", s -> s.keepWithNext()
66+
.addShape(shape -> shape.name("HeaderMark")
67+
.size(260, 30).fillColor(INK)))
68+
.addSection("Body", s -> s
69+
.keepTogether(bodyKeepTogether)
70+
.spacing(0)
71+
.addShape(shape -> shape.name("BodyMark")
72+
.size(260, 100).fillColor(INK))
73+
.addShape(shape -> shape.name("BodyTail")
74+
.size(260, 100).fillColor(GREY)))
75+
.build();
76+
return document.layoutGraph();
77+
} catch (Exception e) {
78+
throw new RuntimeException(e);
79+
}
80+
}
81+
82+
/**
83+
* The regression. Before the fix the header stranded for every filler height
84+
* from 120 to 210pt: the lookahead counted only the body's first 100pt shape,
85+
* decided one line fit, and the compiler then moved all 200pt of the body.
86+
*/
87+
@Test
88+
void headerStaysWithAKeepTogetherBodyAtEveryFillerHeight() {
89+
for (double filler = 10; filler <= 330; filler += 10) {
90+
LayoutGraph graph = build(true, filler);
91+
assertThat(page(graph, "HeaderMark"))
92+
.describedAs("filler=%.0f: a keepTogether body relocates whole, "
93+
+ "so the header must travel with it", filler)
94+
.isEqualTo(page(graph, "BodyMark"));
95+
}
96+
}
97+
98+
/**
99+
* The same sweep with a splittable body, which never regressed. Pins that the
100+
* fix did not buy the atomic case at the cost of the ordinary one: a body that
101+
* can split still anchors the header by its first slice alone.
102+
*/
103+
@Test
104+
void headerStaysWithASplittableBodyAtEveryFillerHeight() {
105+
for (double filler = 10; filler <= 330; filler += 10) {
106+
LayoutGraph graph = build(false, filler);
107+
assertThat(page(graph, "HeaderMark"))
108+
.describedAs("filler=%.0f: a splittable body's first slice "
109+
+ "travels with the header", filler)
110+
.isEqualTo(page(graph, "BodyMark"));
111+
}
112+
}
113+
114+
/**
115+
* A body that is taller than a page has its keep-together ignored by the
116+
* compiler, so the lookahead must keep estimating it by its first slice. This
117+
* is the case the fix must NOT change — sizing it by the whole block would
118+
* exceed the page, fail the {@code needed <= activeInnerHeight} guard, and
119+
* strand headers that relocate correctly today.
120+
*
121+
* <p>Asserted in both directions at a filler height where the rule actually
122+
* fires. It has to be: with a 300pt filler the body's first line still fits
123+
* under the header, so the two land together whether or not anything relocates
124+
* them, and the test would pass against an engine with no {@code keepWithNext}
125+
* at all. The live window here is 310–320pt.</p>
126+
*/
127+
@Test
128+
void pageSpanningKeepTogetherBodyStillAnchorsByItsFirstSlice() {
129+
assertPageSpanningBodyAnchorsByItsFirstSlice(false);
130+
}
131+
132+
/**
133+
* The same, with the keep-together block one level down — the shape
134+
* {@code TimelineBuilder.keepEntriesTogether()} produces. The lookahead
135+
* recurses into the wrapper's first child, so the page height has to travel
136+
* down with it; hand the recursive frame a wrong value and this is the case
137+
* that notices, because only a page-spanning child distinguishes "estimate
138+
* the whole block" from "estimate its first slice".
139+
*/
140+
@Test
141+
void nestedPageSpanningKeepTogetherBodyStillAnchorsByItsFirstSlice() {
142+
assertPageSpanningBodyAnchorsByItsFirstSlice(true);
143+
}
144+
145+
private static void assertPageSpanningBodyAnchorsByItsFirstSlice(boolean nested) {
146+
LayoutGraph on = pageSpanningBody(true, nested);
147+
assertThat(page(on, "HeaderMark"))
148+
.describedAs("keep-together is inert above a page-spanning body, "
149+
+ "so the first line still anchors the header")
150+
.isEqualTo(page(on, "BodyMark"));
151+
152+
LayoutGraph off = pageSpanningBody(false, nested);
153+
assertThat(page(off, "HeaderMark"))
154+
.describedAs("without the opt-in the same header strands — which is "
155+
+ "what makes the assertion above meaningful")
156+
.isLessThan(page(off, "BodyMark"));
157+
}
158+
159+
/**
160+
* A header above a body far taller than a page, at a filler height inside the
161+
* window where {@code keepWithNext} decides the outcome.
162+
*
163+
* @param keepWithNext whether the header opts into staying with its body
164+
* @param nested whether the keep-together block sits one level down,
165+
* inside a plain wrapper
166+
* @return the compiled layout graph
167+
*/
168+
private static LayoutGraph pageSpanningBody(boolean keepWithNext, boolean nested) {
169+
try (DocumentSession document = GraphCompose.document()
170+
.pageSize(PAGE_WIDTH, PAGE_HEIGHT)
171+
.margin(DocumentInsets.of(20))
172+
.create()) {
173+
document.pageFlow().name("Flow").spacing(6)
174+
.addSection("Filler", s -> s.addShape(260, 310, GREY))
175+
.addSection("Header", s -> {
176+
if (keepWithNext) {
177+
s.keepWithNext();
178+
}
179+
s.addShape(shape -> shape.name("HeaderMark")
180+
.size(260, 30).fillColor(INK));
181+
})
182+
.addSection("Body", body -> {
183+
if (nested) {
184+
body.spacing(0).addSection("Entry",
185+
entry -> pageSpanningBlock(entry.keepTogether(true)));
186+
} else {
187+
pageSpanningBlock(body.keepTogether(true));
188+
}
189+
})
190+
.build();
191+
return document.layoutGraph();
192+
} catch (Exception e) {
193+
throw new RuntimeException(e);
194+
}
195+
}
196+
197+
/**
198+
* The keep-together block one level down, which is the shape
199+
* {@code TimelineBuilder.keepEntriesTogether()} produces: a plain wrapper
200+
* whose first child is the atomic one. The lookahead recurses into that
201+
* child, so the page height has to travel down with it — pass the wrong
202+
* value there and a header above a timeline strands exactly as it did
203+
* before the fix.
204+
*/
205+
@Test
206+
void headerStaysWithAKeepTogetherBlockNestedInsideAPlainWrapper() {
207+
for (double filler = 10; filler <= 330; filler += 10) {
208+
final double f = filler;
209+
try (DocumentSession document = GraphCompose.document()
210+
.pageSize(PAGE_WIDTH, PAGE_HEIGHT)
211+
.margin(DocumentInsets.of(20))
212+
.create()) {
213+
document.pageFlow().name("Flow").spacing(6)
214+
.addSection("Filler", s -> s.addShape(260, f, GREY))
215+
.addSection("Header", s -> s.keepWithNext()
216+
.addShape(shape -> shape.name("HeaderMark")
217+
.size(260, 30).fillColor(INK)))
218+
.addSection("Wrapper", wrapper -> wrapper.spacing(0)
219+
.addSection("Entry", entry -> entry.keepTogether(true).spacing(0)
220+
.addShape(shape -> shape.name("BodyMark")
221+
.size(260, 100).fillColor(INK))
222+
.addShape(shape -> shape.name("BodyTail")
223+
.size(260, 100).fillColor(GREY))))
224+
.build();
225+
LayoutGraph graph = document.layoutGraph();
226+
assertThat(page(graph, "HeaderMark"))
227+
.describedAs("filler=%.0f: the wrapper's first child relocates "
228+
+ "whole, so the header must travel with it", f)
229+
.isEqualTo(page(graph, "BodyMark"));
230+
} catch (Exception e) {
231+
throw new RuntimeException(e);
232+
}
233+
}
234+
}
235+
236+
/** Fills a section with a paragraph several pages tall. */
237+
private static void pageSpanningBlock(SectionBuilder section) {
238+
section.spacing(0).addParagraph(p -> p.name("BodyMark")
239+
.text("A page-spanning paragraph. ".repeat(220))
240+
.margin(DocumentInsets.zero()));
241+
}
242+
243+
/**
244+
* Best-effort boundary: when the header and the whole block cannot share even
245+
* an empty page, {@code keepWithNext} declines to relocate rather than strand
246+
* the header on a page of its own. Documents the limit instead of leaving it
247+
* to be discovered.
248+
*
249+
* <p>This one asserts an <em>absence</em>, so it reads the same as "the opt-in
250+
* was never applied" and cannot fail for the right reason on its own. It pins
251+
* the documented boundary; the sweeps above are what drive the behaviour.</p>
252+
*/
253+
@Test
254+
void headerDoesNotRelocateWhenItCannotShareAPageWithTheWholeBody() {
255+
double bodyHeight = INNER_HEIGHT - 20;
256+
try (DocumentSession document = GraphCompose.document()
257+
.pageSize(PAGE_WIDTH, PAGE_HEIGHT)
258+
.margin(DocumentInsets.of(20))
259+
.create()) {
260+
document.pageFlow().name("Flow").spacing(6)
261+
.addSection("Filler", s -> s.addShape(260, 100, GREY))
262+
.addSection("Header", s -> s.keepWithNext()
263+
.addShape(shape -> shape.name("HeaderMark")
264+
.size(260, 30).fillColor(INK)))
265+
.addSection("Body", s -> s.keepTogether(true).spacing(0)
266+
.addShape(shape -> shape.name("BodyMark")
267+
.size(260, bodyHeight).fillColor(INK)))
268+
.build();
269+
LayoutGraph graph = document.layoutGraph();
270+
assertThat(page(graph, "HeaderMark"))
271+
.describedAs("header + body exceed a full page, so the rule stays "
272+
+ "best-effort and the header holds its place")
273+
.isLessThan(page(graph, "BodyMark"));
274+
} catch (Exception e) {
275+
throw new RuntimeException(e);
276+
}
277+
}
278+
}

0 commit comments

Comments
 (0)