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"