refactor(ereader): delete the import-time Page monkey patch (R5)
ereader_layout.py defined _add_page_methods() and called it at import
time, attaching can_fit_line and available_width to the Page class if
they were absent. Page defines both, so the patch never fired - but the
two definitions of can_fit_line disagreed:
Page can_fit_line(baseline_spacing, ascent=0, descent=0)
monkey patch can_fit_line(line_height)
The patched version had no way to express descent, which is exactly the
clipping bug S2 fixed. Had Page.can_fit_line ever been renamed or moved,
this would have silently reinstated pre-S2 behaviour as a side effect of
importing a module in a different package.
Import-time patching of another module's class has no place here. If the
layout engine needs something from Page, it belongs on Page.
Tests pin the outcome: the module exposes no patcher, Page owns both
attributes, and can_fit_line still rejects a line whose descender would
hang past the content box.
894 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -857,27 +857,3 @@ class BidirectionalLayouter:
|
|||||||
if pos1.word_index != pos2.word_index:
|
if pos1.word_index != pos2.word_index:
|
||||||
return 1 if pos1.word_index > pos2.word_index else -1
|
return 1 if pos1.word_index > pos2.word_index else -1
|
||||||
return 0
|
return 0
|
||||||
|
|
||||||
|
|
||||||
# Add can_fit_line method to Page class if it doesn't exist
|
|
||||||
def _add_page_methods():
|
|
||||||
"""Add missing methods to Page class"""
|
|
||||||
if not hasattr(Page, 'can_fit_line'):
|
|
||||||
def can_fit_line(self, line_height: int) -> bool:
|
|
||||||
"""Check if a line of given height can fit on the page"""
|
|
||||||
available_height = self.content_size[1] - self._current_y_offset
|
|
||||||
return available_height >= line_height
|
|
||||||
|
|
||||||
Page.can_fit_line = can_fit_line
|
|
||||||
|
|
||||||
if not hasattr(Page, 'available_width'):
|
|
||||||
@property
|
|
||||||
def available_width(self) -> int:
|
|
||||||
"""Get available width for content"""
|
|
||||||
return self.content_size[0]
|
|
||||||
|
|
||||||
Page.available_width = available_width
|
|
||||||
|
|
||||||
|
|
||||||
# Apply the page methods
|
|
||||||
_add_page_methods()
|
|
||||||
|
|||||||
@@ -899,5 +899,43 @@ class TestBidirectionalLayouter:
|
|||||||
assert next_pos == position # No progress possible
|
assert next_pos == position # No progress possible
|
||||||
|
|
||||||
|
|
||||||
|
class TestNoPageMonkeyPatching:
|
||||||
|
"""
|
||||||
|
R5: importing this module used to run _add_page_methods(), which attached
|
||||||
|
can_fit_line/available_width to Page if they were absent. They are not
|
||||||
|
absent, so it never fired - but its can_fit_line took (line_height) and
|
||||||
|
ignored descenders, while Page's takes (baseline_spacing, ascent, descent).
|
||||||
|
Had Page's ever been renamed, the import would have silently reinstated the
|
||||||
|
pre-S2 clipping bug from another package.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def test_module_does_not_patch_page(self):
|
||||||
|
import pyWebLayout.layout.ereader_layout as ereader_layout
|
||||||
|
|
||||||
|
assert not hasattr(ereader_layout, '_add_page_methods')
|
||||||
|
|
||||||
|
def test_page_owns_its_geometry_methods(self):
|
||||||
|
from pyWebLayout.concrete.page import Page
|
||||||
|
|
||||||
|
assert 'can_fit_line' in vars(Page)
|
||||||
|
assert 'available_width' in vars(Page)
|
||||||
|
|
||||||
|
def test_can_fit_line_still_accounts_for_descenders(self, sample_page_style):
|
||||||
|
"""
|
||||||
|
The patched version took a single line_height and had no way to express
|
||||||
|
descent, so a descender hanging past the content box counted as fitting.
|
||||||
|
"""
|
||||||
|
from pyWebLayout.concrete.page import Page
|
||||||
|
|
||||||
|
page = Page(size=(200, 100), style=sample_page_style)
|
||||||
|
content_y, content_h = page.content_rect[1], page.content_rect[3]
|
||||||
|
available = content_y + content_h - page._current_y_offset
|
||||||
|
|
||||||
|
assert page.can_fit_line(0, ascent=available, descent=0)
|
||||||
|
assert not page.can_fit_line(0, ascent=available, descent=1), \
|
||||||
|
"a descender past the content box must not be reported as fitting"
|
||||||
|
assert page.can_fit_line(0, ascent=available - 1, descent=1)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
pytest.main([__file__, "-v"])
|
pytest.main([__file__, "-v"])
|
||||||
|
|||||||
Reference in New Issue
Block a user