fix(ereader): keep resume position when a block spans a page (S11)
render_page_forward discarded new_pos on the failure path, but a block that only partially fitted has still advanced the position: paragraph_layouter reports the word it stopped at, and _layout_paragraph_on_page packs it into new_pos. Dropping it told the caller no progress was made, so navigation dead-ended on any paragraph larger than a single page - the reader saw "end of document" mid-book. A 2877-word paragraph at 800x600 rendered 26 lines and reported the start position back; it now paginates across 12 pages. Also guard the navigation loop: EreaderManager.next_page treats no-progress as end-of-document, which is only correct at the actual end. Anywhere else it now logs the offending block index and skips that block, so a future layout bug costs one block rather than the rest of the book.
This commit is contained in:
@@ -344,7 +344,14 @@ class BidirectionalLayouter:
|
|||||||
scaled_block, page, current_pos, font_scale)
|
scaled_block, page, current_pos, font_scale)
|
||||||
|
|
||||||
if not success:
|
if not success:
|
||||||
# Block doesn't fit, we're done with this page
|
# The block did not fit in its entirety. It may still have been
|
||||||
|
# laid out partially - a paragraph larger than one page places as
|
||||||
|
# many lines as fit and reports the word it stopped at. Keeping
|
||||||
|
# that resume point is what allows the next page to continue;
|
||||||
|
# discarding it tells the caller no progress was made, which
|
||||||
|
# dead-ends navigation on the block forever.
|
||||||
|
if self._position_compare(new_pos, current_pos) > 0:
|
||||||
|
current_pos = new_pos
|
||||||
break
|
break
|
||||||
|
|
||||||
# Add inter-block spacing after successfully laying out a block
|
# Add inter-block spacing after successfully laying out a block
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ into a unified, easy-to-use API.
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
from typing import List, Dict, Optional, Tuple, Any, Callable
|
from typing import List, Dict, Optional, Tuple, Any, Callable
|
||||||
import json
|
import json
|
||||||
|
import logging
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
from .ereader_layout import RenderingPosition, ChapterNavigator, ChapterInfo
|
from .ereader_layout import RenderingPosition, ChapterNavigator, ChapterInfo
|
||||||
@@ -20,6 +21,8 @@ from pyWebLayout.style.page_style import PageStyle
|
|||||||
from pyWebLayout.style.fonts import BundledFont
|
from pyWebLayout.style.fonts import BundledFont
|
||||||
from pyWebLayout.layout.document_layouter import image_layouter
|
from pyWebLayout.layout.document_layouter import image_layouter
|
||||||
|
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|
||||||
class BookmarkManager:
|
class BookmarkManager:
|
||||||
"""
|
"""
|
||||||
@@ -417,6 +420,21 @@ class EreaderLayoutManager:
|
|||||||
self._notify_position_changed()
|
self._notify_position_changed()
|
||||||
return self.get_current_page()
|
return self.get_current_page()
|
||||||
|
|
||||||
|
# No progress. That is the correct answer only at the end of the
|
||||||
|
# document; anywhere else a block has failed to lay out and would trap
|
||||||
|
# the reader on this page. Skipping the block costs one block, not the
|
||||||
|
# rest of the book.
|
||||||
|
if self.current_position.block_index < len(self.blocks):
|
||||||
|
logger.error(
|
||||||
|
"Block %d made no layout progress; skipping it. This is a layout "
|
||||||
|
"bug - the block placed nothing and reported no resume point.",
|
||||||
|
self.current_position.block_index)
|
||||||
|
self.current_position = RenderingPosition(
|
||||||
|
chapter_index=self.current_position.chapter_index,
|
||||||
|
block_index=self.current_position.block_index + 1)
|
||||||
|
self._notify_position_changed()
|
||||||
|
return self.get_current_page()
|
||||||
|
|
||||||
return None # At end of document
|
return None # At end of document
|
||||||
|
|
||||||
def previous_page(self) -> Optional[Page]:
|
def previous_page(self) -> Optional[Page]:
|
||||||
|
|||||||
@@ -0,0 +1,123 @@
|
|||||||
|
"""
|
||||||
|
Regression tests for blocks that span more than one page.
|
||||||
|
|
||||||
|
A block larger than a single page is laid out partially, and the layouter reports
|
||||||
|
where it stopped. If that resume point is discarded, the reader is told it made no
|
||||||
|
progress and navigation dead-ends on that block (spec S11).
|
||||||
|
"""
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from pyWebLayout.abstract.block import Paragraph
|
||||||
|
from pyWebLayout.abstract.inline import Word
|
||||||
|
from pyWebLayout.concrete.page import Page
|
||||||
|
from pyWebLayout.layout.ereader_layout import BidirectionalLayouter, RenderingPosition
|
||||||
|
from pyWebLayout.style import Font
|
||||||
|
from pyWebLayout.style.page_style import PageStyle
|
||||||
|
|
||||||
|
|
||||||
|
PAGE_SIZE = (800, 600)
|
||||||
|
|
||||||
|
|
||||||
|
def make_paragraph(word_count, font):
|
||||||
|
"""A paragraph of distinct words, so we can verify none are lost or repeated."""
|
||||||
|
paragraph = Paragraph(font)
|
||||||
|
for i in range(word_count):
|
||||||
|
paragraph.add_word(Word(f"word{i}", font))
|
||||||
|
return paragraph
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def font():
|
||||||
|
return Font(font_size=16)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def huge_paragraph(font):
|
||||||
|
"""A paragraph far larger than one page - the shape that dead-ended."""
|
||||||
|
return make_paragraph(2877, font)
|
||||||
|
|
||||||
|
|
||||||
|
class TestPageSpanningParagraph:
|
||||||
|
"""A single paragraph larger than one page must paginate, not dead-end."""
|
||||||
|
|
||||||
|
def test_layouter_reports_where_it_stopped(self, huge_paragraph):
|
||||||
|
layouter = BidirectionalLayouter([huge_paragraph], PageStyle(), PAGE_SIZE)
|
||||||
|
page = Page(size=PAGE_SIZE, style=PageStyle())
|
||||||
|
|
||||||
|
success, new_pos = layouter._layout_block_on_page(
|
||||||
|
huge_paragraph, page, RenderingPosition(), 1.0)
|
||||||
|
|
||||||
|
assert not success, "a 2877-word paragraph cannot fit on one page"
|
||||||
|
assert new_pos.word_index > 0, "the resume point must be reported"
|
||||||
|
|
||||||
|
def test_first_page_advances(self, huge_paragraph):
|
||||||
|
"""The regression: next position equalled the start position."""
|
||||||
|
layouter = BidirectionalLayouter([huge_paragraph], PageStyle(), PAGE_SIZE)
|
||||||
|
start = RenderingPosition()
|
||||||
|
|
||||||
|
page, next_pos = layouter.render_page_forward(start, 1.0)
|
||||||
|
|
||||||
|
assert len(page.children) > 0, "content was placed on the page"
|
||||||
|
assert (next_pos.block_index, next_pos.word_index) > \
|
||||||
|
(start.block_index, start.word_index), \
|
||||||
|
"a page with content on it must advance the position"
|
||||||
|
|
||||||
|
def test_paginates_to_completion(self, huge_paragraph):
|
||||||
|
"""Every page advances, and the document terminates."""
|
||||||
|
layouter = BidirectionalLayouter([huge_paragraph], PageStyle(), PAGE_SIZE)
|
||||||
|
pos = RenderingPosition()
|
||||||
|
positions = [(pos.block_index, pos.word_index)]
|
||||||
|
|
||||||
|
for _ in range(100):
|
||||||
|
page, next_pos = layouter.render_page_forward(pos, 1.0)
|
||||||
|
key = (next_pos.block_index, next_pos.word_index)
|
||||||
|
|
||||||
|
if next_pos.block_index >= 1:
|
||||||
|
break # ran off the end of the (single-block) document
|
||||||
|
|
||||||
|
assert key > positions[-1], f"no progress at page {len(positions)}"
|
||||||
|
positions.append(key)
|
||||||
|
pos = next_pos
|
||||||
|
else:
|
||||||
|
pytest.fail("pagination did not terminate")
|
||||||
|
|
||||||
|
assert len(positions) > 5, "a 2877-word paragraph spans several pages"
|
||||||
|
|
||||||
|
def test_no_words_lost_or_repeated(self, huge_paragraph):
|
||||||
|
"""Word coverage across pages is exactly the paragraph, in order."""
|
||||||
|
layouter = BidirectionalLayouter([huge_paragraph], PageStyle(), PAGE_SIZE)
|
||||||
|
pos = RenderingPosition()
|
||||||
|
boundaries = [0]
|
||||||
|
|
||||||
|
for _ in range(100):
|
||||||
|
_, next_pos = layouter.render_page_forward(pos, 1.0)
|
||||||
|
if next_pos.block_index >= 1:
|
||||||
|
break
|
||||||
|
boundaries.append(next_pos.word_index)
|
||||||
|
pos = next_pos
|
||||||
|
|
||||||
|
assert boundaries == sorted(boundaries), "word indices must not go backward"
|
||||||
|
assert len(boundaries) == len(set(boundaries)), "a page must not be re-rendered"
|
||||||
|
|
||||||
|
|
||||||
|
class TestNonSpanningBlocksUnaffected:
|
||||||
|
"""The fix must not change behaviour for blocks that fit."""
|
||||||
|
|
||||||
|
def test_small_paragraphs_still_advance_by_block(self, font):
|
||||||
|
blocks = [make_paragraph(20, font) for _ in range(3)]
|
||||||
|
layouter = BidirectionalLayouter(blocks, PageStyle(), PAGE_SIZE)
|
||||||
|
|
||||||
|
_, next_pos = layouter.render_page_forward(RenderingPosition(), 1.0)
|
||||||
|
|
||||||
|
assert next_pos.block_index == 3, "all three short paragraphs fit on one page"
|
||||||
|
assert next_pos.word_index == 0
|
||||||
|
|
||||||
|
def test_empty_document_terminates(self):
|
||||||
|
layouter = BidirectionalLayouter([], PageStyle(), PAGE_SIZE)
|
||||||
|
start = RenderingPosition()
|
||||||
|
|
||||||
|
page, next_pos = layouter.render_page_forward(start, 1.0)
|
||||||
|
|
||||||
|
assert next_pos.block_index == start.block_index
|
||||||
|
assert len(page.children) == 0
|
||||||
Reference in New Issue
Block a user