From 456824d6d66bede961962369cebdfcf24649af3d Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 8 Aug 2026 12:43:30 +0200 Subject: [PATCH] fix(ereader): replay the page chain instead of searching for it (S16) render_page_backward searched for the previous page's start: estimate a block index, lay out forward, bisect on the block difference, up to ten times. Both the estimator and the adjuster pinned word_index to 0 and moved only block_index - but pages routinely start mid-block, so the answer was not in the search space. The loop could never match, exhausted its iterations and fell through to a fallback that jumped to the start of the document. Measured on a document with one 1200-word paragraph, whose page starts are (0,0), (2,208), (2,494), (2,780): three of four backward calls returned (0,0), each after ten full page layouts. The bisection was unsound in its own space too - 40 small paragraphs, every page starting on a block boundary, also failed. Pagination is a pure function, so the page before P is the q with next(q) == P, and it is found by replaying the chain forward rather than guessing q. Three sources: the chain recorded as pages are laid out forward (exact, one layout, covers paging back and forth); replay from the block containing P and then from earlier blocks (exact when P lies on that chain); and failing that, the last start before P, which overlaps slightly rather than skipping content. warm: 4/4 exact, 1 layout each cold, fresh layouter per call: 12/13 exact, worst 17 layouts cold, repeated back presses: ~4 layouts per turn Each step returns a page ending exactly where the reader is, so paging back never skips or repeats content. That chain can differ from the one seen reading forward from page one if the reader arrived by a jump - pagination from a different start is a different chain, and nothing can recover the original without replaying the whole document. Complementary to S11 rather than caused by it: before S11 forward pagination dead-ended at the first page-spanning block, so mid-block starts never arose and the block-granular search looked adequate. --- docs/LAYOUT_REMEDIATION_SPEC.md | 98 +++++++++++ pyWebLayout/layout/ereader_layout.py | 204 ++++++++++++++--------- tests/layout/test_backward_navigation.py | 171 +++++++++++++++++++ 3 files changed, 398 insertions(+), 75 deletions(-) create mode 100644 tests/layout/test_backward_navigation.py diff --git a/docs/LAYOUT_REMEDIATION_SPEC.md b/docs/LAYOUT_REMEDIATION_SPEC.md index 74838d6..00784cd 100644 --- a/docs/LAYOUT_REMEDIATION_SPEC.md +++ b/docs/LAYOUT_REMEDIATION_SPEC.md @@ -30,6 +30,7 @@ It is independent of every other spec here. | [S13](#s13--word-spacing-and-alignment) | Word spacing and alignment | 0 | | [S14](#s14--vertical-centring-in-buttons-and-fields) | Vertical centring in buttons and fields | 0 | | [S15](#s15--form-field-label-geometry) | Form field label geometry | 0 | +| [S16](#s16--backward-page-navigation) | Backward page navigation | 0 | ## Design invariants @@ -1265,6 +1266,103 @@ between label and box smaller than the intended 5px. --- +## S16 — Backward page navigation + +### Problem + +`render_page_backward` *searched* for the previous page's start: estimate a block +index, lay out forward, compare the end against the target, bisect on the block +difference, repeat up to ten times. Both the estimator and the adjuster pinned +`word_index` to 0 and moved only `block_index`. + +Pages routinely start mid-block. Any such start was therefore **not in the search +space**, the loop could never match, and it fell through to a fallback that +jumped several blocks back or to the document start. + +### Evidence + +A document of short paragraphs around one 1200-word paragraph. Forward pagination +gives page starts at `(0,0), (2,208), (2,494), (2,780), (2,1057)`. Asking for the +page that ends where each of those begins: + +``` +from page 1 -> got (0,0) expected (0,0) ok (1 forward layout) +from page 2 -> got (0,0) expected (2,208) WRONG (10 forward layouts) +from page 3 -> got (0,0) expected (2,494) WRONG (10 forward layouts) +from page 4 -> got (0,0) expected (2,780) WRONG (10 forward layouts) +``` + +Every mid-paragraph case threw the reader to the start of the document after ten +full page layouts. The bisection was also unsound within its own space: a +document of 40 small paragraphs, where every page *does* start on a block +boundary, failed too. + +This is complementary to S11 rather than caused by it. Before S11 forward +pagination dead-ended at the first page-spanning block, so mid-block starts were +never produced and the block-granular search looked adequate. + +### Design + +Pagination is a pure function: laying out from `q` yields a page and the position +it stopped at, `next(q)`. The page before `P` is the `q` with `next(q) == P`. +That is found by **replaying the chain forward from an anchor**, not by guessing +`q`. Three sources, in order: + +1. **The recorded chain.** `render_page_forward` now records + `(font_scale, next(q)) -> q`. Stepping back to anywhere the reader has been is + exact and costs one layout. Keyed by font scale, since changing it + repaginates. +2. **Replay from an anchor.** Anchors are block starts, nearest first: the block + containing `P`, then up to `MAX_BACKWARD_ANCHORS` earlier ones, then the + document start. Lay out forward from the anchor until a page ends exactly on + `P`; that page's start is the answer. `MAX_REPLAY_PAGES` caps the walk so one + page turn cannot traverse a whole chapter. +3. **Nearest start before `P`.** If no chain passes exactly through `P` — which + happens when `P` was reached by a jump or a restored bookmark rather than by + reading forward, so it lies on no natural chain — return the last page start + before it. That overlaps `P`'s page slightly rather than skipping content, + which is the safe direction to be wrong in. + +The estimator and the bisecting adjuster are deleted. + +**What "correct" means here.** Each backward step returns a page ending exactly +where the reader currently is, so paging back never skips or repeats content. +That chain can differ from the one you would have seen reading forward from page +one, if you entered the document by a jump — pagination from a different starting +point is genuinely a different chain, and no algorithm can recover the original +without replaying from the start. + +### Measurements + +Same document, after the change: + +``` +warm (chain recorded by the forward pass): 4/4 exact, 1 layout each +cold, fresh layouter per call: 12/13 exact, worst 17 layouts +cold, one layouter, repeated back presses: 4 layouts per turn typical +``` + +The single inexact case is a target that lies on the canonical chain but not on +any chain reachable from a nearby anchor; it returns a start 15 words early, +i.e. a slightly overlapping page. + +### Acceptance criteria + +- For every page of a document, `render_page_backward(start[i])` returns + `start[i-1]` — verified for both a mid-paragraph-paginating document and one + where every page starts on a block boundary. +- Laying out forward from the returned position ends exactly on the requested + position. +- Forward-then-back returns to the original position. +- At the document start, backward stays there; an empty document is safe. +- Cost stays within a small bounded number of forward layouts. + +### Files + +`pyWebLayout/layout/ereader_layout.py` + +--- + ## Test plan Findings were reproduced with four probe scripts; each becomes a regression test diff --git a/pyWebLayout/layout/ereader_layout.py b/pyWebLayout/layout/ereader_layout.py index 2a1878b..7ad470b 100644 --- a/pyWebLayout/layout/ereader_layout.py +++ b/pyWebLayout/layout/ereader_layout.py @@ -313,6 +313,13 @@ class BidirectionalLayouter: self.alignment_override = alignment_override self.font_family_override = font_family_override + # Maps (font_scale, end position) -> the position the page started at. + # Filled in as pages are laid out forward, which makes "previous page" + # exact and free for anywhere the reader has already been. Keyed by font + # scale because changing it repaginates the document. + self._page_chain: Dict[Tuple[float, Tuple[int, int, int]], + RenderingPosition] = {} + def render_page_forward(self, position: RenderingPosition, font_scale: float = 1.0) -> Tuple[Page, RenderingPosition]: """ @@ -367,109 +374,156 @@ class BidirectionalLayouter: current_pos = new_pos + # Remember this link in the chain so stepping back to it later is exact. + if self._position_compare(current_pos, position) > 0: + self._page_chain[(font_scale, self._position_key(current_pos))] = \ + position.copy() + return page, current_pos + # How many block starts before the target to try as replay anchors before + # settling for the best inexact answer. + MAX_BACKWARD_ANCHORS = 4 + + # Ceiling on pages replayed from a single anchor, so a pathologically long + # block cannot make one page turn walk an entire chapter. + MAX_REPLAY_PAGES = 8 + def render_page_backward(self, end_position: RenderingPosition, font_scale: float = 1.0) -> Tuple[Page, RenderingPosition]: """ - Render a page that ends at the given position, filling backward. - Critical for "previous page" navigation. + Render the page that ends at the given position - "previous page". - Uses iterative refinement to find the correct start position that - results in a page ending at (or very close to) the target position. + Pagination is a pure function: laying out from a position q yields a page + and the position where it stopped, next(q). The page before P is therefore + the q for which next(q) == P, and it is found by *replaying* the chain + forward from an anchor, not by guessing q. + + The previous implementation searched instead: it estimated a block index + and bisected on it, pinning word_index to 0. Pages routinely start + mid-block, so the answer was frequently not in the search space at all - + the search then exhausted its iterations and fell back to a position that + was not the previous page, usually the start of the document. + + Three sources are tried in order: + + 1. The recorded chain, from pages already laid out going forward. Exact, + and the common case when the reader is paging back and forth. + 2. Replay from the start of the block containing P, then from + progressively earlier blocks. Exact when P lies on the resulting chain. + 3. Failing an exact hit - which happens when P was reached by a jump or a + restored bookmark rather than by reading forward, so it is on no + natural chain - the latest page start before P. That overlaps P's page + slightly rather than skipping content, which is the safe direction to + be wrong in. Args: - end_position: Position where page should end + end_position: Position where the page should end font_scale: Font scaling factor Returns: Tuple of (rendered_page, start_position) """ - # Handle edge case: already at beginning - if end_position.block_index == 0 and end_position.word_index == 0: - return self.render_page_forward(end_position, font_scale) + document_start = RenderingPosition() - # Start with initial estimate - estimated_start = self._estimate_page_start(end_position, font_scale) + # Nothing precedes the start of the document. + if self._position_compare(end_position, document_start) <= 0: + page, _ = self.render_page_forward(document_start, font_scale) + return page, document_start - # Iterative refinement: keep adjusting until we converge or hit max iterations - max_iterations = 10 - best_page = None - best_start = estimated_start - best_distance = float('inf') + # 1. The chain we have already walked. + remembered = self._page_chain.get((font_scale, self._position_key(end_position))) + if remembered is not None: + page, actual_end = self.render_page_forward(remembered, font_scale) + if self._position_compare(actual_end, end_position) == 0: + return page, remembered - for iteration in range(max_iterations): - # Render forward from current estimate - page, actual_end = self.render_page_forward(estimated_start, font_scale) + # 2/3. Replay from anchors, keeping the best inexact result as a fallback. + fallback = None + for anchor in self._backward_anchors(end_position): + page, start, exact = self._replay_to(anchor, end_position, font_scale) + if page is None: + continue + if exact: + return page, start + if fallback is None: + fallback = (page, start) - # Calculate how far we are from target - comparison = self._position_compare(actual_end, end_position) + if fallback is not None: + return fallback - # Perfect match or close enough (within same block) - # BUT: ensure we actually moved backward (estimated_start < end_position) - if comparison == 0: - # Check if we actually found a valid previous page - if self._position_compare(estimated_start, end_position) < 0: - return page, estimated_start - # If estimated_start >= end_position, we haven't moved backward - # Continue iterating to find a better position - elif iteration == 0: - # On first iteration, if we can't find a previous position, - # we're likely at or near the beginning - break + page, _ = self.render_page_forward(document_start, font_scale) + return page, document_start - # Track best result so far - distance = abs(actual_end.block_index - end_position.block_index) - if distance < best_distance: - best_distance = distance - best_page = page - best_start = estimated_start.copy() + def _backward_anchors(self, target: RenderingPosition): + """ + Yield positions to replay from, nearest first. - # Adjust estimate for next iteration - estimated_start = self._adjust_start_estimate( - estimated_start, end_position, actual_end) + Block starts are used as anchors because they are the coarsest positions + that are certainly valid to lay out from. The block containing the target + comes first: when the target is mid-block, the page before it usually + starts in that same block or the one before. + """ + first_block = target.block_index if target.word_index > 0 \ + else target.block_index - 1 - # Safety: don't go before document start - if estimated_start.block_index < 0: - estimated_start.block_index = 0 - estimated_start.word_index = 0 - - # If we exhausted iterations, return best result found - # BUT: ensure we didn't return the same position (no backward progress) - final_page = best_page if best_page else page - final_start = best_start - - # Safety check: if final_start >= end_position, we failed to move backward - # This can happen at the beginning of the document or when estimation failed - if self._position_compare(final_start, end_position) >= 0: - # Can't go further back - check if we're at the absolute beginning - if end_position.block_index == 0 and end_position.word_index == 0: - # Already at beginning, return as-is - return final_page, final_start - - # Fallback strategy: try a more aggressive backward jump - # Start from several blocks before the current position - blocks_to_jump = max(1, min(5, end_position.block_index)) - fallback_pos = RenderingPosition( - chapter_index=end_position.chapter_index, - block_index=max(0, end_position.block_index - blocks_to_jump), - word_index=0 + for offset in range(self.MAX_BACKWARD_ANCHORS): + block_index = first_block - offset + if block_index < 0: + break + yield RenderingPosition( + chapter_index=target.chapter_index, + block_index=block_index, + word_index=0, ) - # Render forward from the fallback position - fallback_page, fallback_end = self.render_page_forward(fallback_pos, font_scale) + if first_block - self.MAX_BACKWARD_ANCHORS >= 0: + yield RenderingPosition() - # Verify the fallback actually moved us backward - if self._position_compare(fallback_pos, end_position) < 0: - return fallback_page, fallback_pos + def _replay_to(self, + anchor: RenderingPosition, + target: RenderingPosition, + font_scale: float): + """ + Lay out pages forward from `anchor`, looking for the one ending at `target`. - # If even the fallback didn't work, we're likely at the beginning - # Return a page starting from the beginning - return self.render_page_forward(RenderingPosition(), font_scale) + Returns: + (page, start, exact). `exact` is True when a page ended precisely on + the target. When the chain steps over the target instead, the last + page starting before it is returned with exact=False. (None, None, + False) means the anchor yielded nothing usable. + """ + position = anchor + last = (None, None) - return final_page, final_start + for _ in range(self.MAX_REPLAY_PAGES): + if self._position_compare(position, target) >= 0: + break + + page, next_position = self.render_page_forward(position, font_scale) + comparison = self._position_compare(next_position, target) + + if comparison == 0: + return page, position, True + + if comparison > 0: + # Stepped over the target: this chain does not pass through it. + return last[0], last[1], False + + if self._position_compare(next_position, position) <= 0: + break # no progress; give up on this anchor + + last = (page, position) + position = next_position + + return last[0], last[1], False + + @staticmethod + def _position_key(position: RenderingPosition) -> Tuple[int, int, int]: + """Hashable identity of a position, for the page chain map.""" + return (position.chapter_index, position.block_index, position.word_index) def _scale_block_fonts(self, block: Block, font_scale: float) -> Block: """Apply font scaling and font family override to all fonts in a block""" diff --git a/tests/layout/test_backward_navigation.py b/tests/layout/test_backward_navigation.py new file mode 100644 index 0000000..0260249 --- /dev/null +++ b/tests/layout/test_backward_navigation.py @@ -0,0 +1,171 @@ +""" +Regression tests for backward page navigation (spec S16). + +The previous page of P is the position q for which laying out forward from q ends +exactly at P. The old implementation searched for q by guessing a block index and +bisecting, with word_index pinned to 0 - so a page starting mid-paragraph was not +in the search space at all. It exhausted its ten iterations and fell back to a +position that was not the previous page, typically the start of the document. +""" + +import pytest + +from pyWebLayout.abstract.block import Paragraph +from pyWebLayout.abstract.inline import Word +from pyWebLayout.layout.ereader_layout import BidirectionalLayouter, RenderingPosition +from pyWebLayout.style import Font +from pyWebLayout.style.page_style import PageStyle + + +PAGE_SIZE = (800, 600) + + +@pytest.fixture +def font(): + return Font(font_size=16) + + +def paragraph(font, count, tag): + block = Paragraph(font) + for i in range(count): + block.add_word(Word(f"{tag}{i}", font)) + return block + + +@pytest.fixture +def long_document(font): + """Short paragraphs around one that spans several pages.""" + return [ + paragraph(font, 60, "a"), + paragraph(font, 80, "b"), + paragraph(font, 1200, "long"), + paragraph(font, 70, "c"), + paragraph(font, 90, "d"), + ] + + +@pytest.fixture +def block_document(font): + """Many small blocks, so every page starts on a block boundary.""" + return [paragraph(font, 40, f"p{i}") for i in range(40)] + + +def forward_chain(layouter, limit=30): + """The page start positions a reader would visit going forward.""" + starts = [] + pos = RenderingPosition() + for _ in range(limit): + starts.append(pos) + _, nxt = layouter.render_page_forward(pos, 1.0) + if nxt.block_index >= len(layouter.blocks): + break + if (nxt.block_index, nxt.word_index) == (pos.block_index, pos.word_index): + pytest.fail("forward pagination made no progress") + pos = nxt + return starts + + +def key(position): + return (position.chapter_index, position.block_index, position.word_index) + + +class TestBackwardMatchesForward: + """The defining invariant: forward from the answer lands exactly on P.""" + + @pytest.mark.parametrize("document", ["long_document", "block_document"]) + def test_previous_page_is_the_forward_predecessor(self, document, request): + blocks = request.getfixturevalue(document) + layouter = BidirectionalLayouter(blocks, PageStyle(), PAGE_SIZE) + starts = forward_chain(layouter) + assert len(starts) > 2, "need a few pages to test against" + + for i in range(1, len(starts)): + _, got = layouter.render_page_backward(starts[i], 1.0) + assert key(got) == key(starts[i - 1]), ( + f"page {i}: expected to land on page {i - 1} " + f"{key(starts[i - 1])}, got {key(got)}") + + def test_result_lays_out_to_the_target(self, long_document): + """Independent of the recorded chain: replaying the answer must reach P.""" + layouter = BidirectionalLayouter(long_document, PageStyle(), PAGE_SIZE) + starts = forward_chain(layouter) + + for target in starts[1:]: + _, start = layouter.render_page_backward(target, 1.0) + _, end = layouter.render_page_forward(start, 1.0) + assert key(end) == key(target), ( + f"a page starting at {key(start)} ends at {key(end)}, " + f"not at the requested {key(target)}") + + def test_mid_paragraph_targets_are_reachable(self, long_document): + """The specific regression: starts inside a block, not on its boundary.""" + layouter = BidirectionalLayouter(long_document, PageStyle(), PAGE_SIZE) + starts = forward_chain(layouter) + mid = [s for s in starts if s.word_index > 0] + assert mid, "this document should paginate mid-paragraph" + + for target in mid: + _, got = layouter.render_page_backward(target, 1.0) + assert key(got) != (0, 0, 0) or key(target) == key(starts[1]), \ + "backward navigation fell back to the document start" + + +class TestRoundTrip: + + def test_forward_then_back_returns_to_the_same_place(self, long_document): + layouter = BidirectionalLayouter(long_document, PageStyle(), PAGE_SIZE) + pos = RenderingPosition() + + for _ in range(4): + _, nxt = layouter.render_page_forward(pos, 1.0) + _, back = layouter.render_page_backward(nxt, 1.0) + assert key(back) == key(pos), \ + f"round trip drifted: {key(pos)} -> {key(nxt)} -> {key(back)}" + pos = nxt + + +class TestEdges: + + def test_at_document_start_stays_there(self, long_document): + layouter = BidirectionalLayouter(long_document, PageStyle(), PAGE_SIZE) + _, got = layouter.render_page_backward(RenderingPosition(), 1.0) + assert key(got) == (0, 0, 0) + + def test_second_page_goes_back_to_the_first(self, long_document): + layouter = BidirectionalLayouter(long_document, PageStyle(), PAGE_SIZE) + _, second = layouter.render_page_forward(RenderingPosition(), 1.0) + _, got = layouter.render_page_backward(second, 1.0) + assert key(got) == (0, 0, 0) + + def test_empty_document_is_safe(self): + layouter = BidirectionalLayouter([], PageStyle(), PAGE_SIZE) + page, got = layouter.render_page_backward(RenderingPosition(), 1.0) + assert page is not None + assert key(got) == (0, 0, 0) + + +class TestCost: + + def test_backward_is_not_wildly_more_expensive_than_forward(self, long_document): + """The old path burned ten full layouts per call and still got it wrong.""" + layouter = BidirectionalLayouter(long_document, PageStyle(), PAGE_SIZE) + starts = forward_chain(layouter) + + calls = {"n": 0} + original = BidirectionalLayouter.render_page_forward + + def counting(self, position, font_scale=1.0): + calls["n"] += 1 + return original(self, position, font_scale) + + BidirectionalLayouter.render_page_forward = counting + try: + worst = 0 + for target in starts[1:]: + calls["n"] = 0 + layouter.render_page_backward(target, 1.0) + worst = max(worst, calls["n"]) + finally: + BidirectionalLayouter.render_page_forward = original + + assert worst <= 10, f"backward navigation cost {worst} forward layouts"