Twelve specs covering the defects found auditing the block/table rendering
path, plus the pagination dead-end and the broken background renderer.
Each spec carries a reproduction of the defect against 2a543d0, a design with
concrete signatures, and acceptance criteria. Five design invariants are stated
up front so future changes can be rejected by reference rather than re-argued.
1124 lines
47 KiB
Markdown
1124 lines
47 KiB
Markdown
# Layout Remediation Spec
|
||
|
||
Remediation plan for the defects found in the block/table rendering path
|
||
(audit of 2026-08-06), plus two outstanding defects in the pagination and
|
||
background-rendering paths (S11, S12). Twelve specs, sequenced into five phases.
|
||
|
||
Every problem statement below was reproduced against the code at `2a543d0`; the
|
||
reproductions are quoted verbatim so each spec has a falsifiable "before" state.
|
||
|
||
**S11 is the most user-visible defect in this document** and the cheapest to
|
||
fix: a single paragraph larger than one page dead-ends the reader permanently.
|
||
It is independent of every other spec here.
|
||
|
||
## Contents
|
||
|
||
| ID | Spec | Phase |
|
||
|----|------|-------|
|
||
| [S1](#s1--inline-content-in-non-paragraph-containers) | Inline content in non-paragraph containers | 0 |
|
||
| [S2](#s2--page-geometry-origin-and-content-rect) | Page geometry: origin and content rect | 1 |
|
||
| [S3](#s3--drawcanvas-lifecycle) | Draw/canvas lifecycle | 1 |
|
||
| [S4](#s4--one-block-dispatch-one-measurement) | One block dispatch, one measurement | 2 |
|
||
| [S5](#s5--cells-as-sub-layouts) | Cells as sub-layouts | 2 |
|
||
| [S6](#s6--table-grid-model) | Table grid model | 3 |
|
||
| [S7](#s7--retained-mode-table-rendering) | Retained-mode table rendering | 3 |
|
||
| [S8](#s8--table-and-list-pagination) | Table and list pagination | 4 |
|
||
| [S9](#s9--interactivity-inside-tables) | Interactivity inside tables | 4 |
|
||
| [S10](#s10--contracts-and-hygiene) | Contracts and hygiene | 5 |
|
||
| [S11](#s11--partial-block-progress-is-discarded) | Partial-block progress is discarded | 0 |
|
||
| [S12](#s12--background-rendering) | Background rendering | 4 |
|
||
|
||
## Design invariants
|
||
|
||
These are the rules the specs exist to establish. After remediation, a reviewer
|
||
should be able to reject a change by pointing at one of these.
|
||
|
||
1. **One dispatch.** There is exactly one function that maps an abstract block to
|
||
concrete objects on a page. Tables, the document layouter and the ereader all
|
||
call it. No component re-implements paragraph layout.
|
||
2. **Retained mode everywhere.** Layout produces children; `Page.render()` draws
|
||
them. Nothing paints during layout. A page can be rendered any number of times
|
||
and produce identical output.
|
||
3. **Measure and render agree by construction.** Reported height comes from the
|
||
same objects that get drawn, never from a parallel estimator.
|
||
4. **A cell is a page.** Any block the layout engine can place on a page can be
|
||
placed in a table cell, a list item or a blockquote, with no per-container
|
||
type switch.
|
||
5. **Style flows from the document.** No layout component invents a font, a size
|
||
or a path. Absolute font paths never appear outside `style/fonts.py`.
|
||
|
||
## Phasing
|
||
|
||
```
|
||
S11 (independent, ship today)
|
||
S1 (independent, ship first)
|
||
S12 (independent; decide delete-vs-fix before touching S8)
|
||
S2 ─┬─ S4 ── S5 ─┬─ S6 ── S7 ─┬─ S8
|
||
S3 ─┘ │ └─ S9
|
||
└──────────────── S10 (opportunistic, any time after S7)
|
||
```
|
||
|
||
S11 is a few lines and unblocks reading the affected books; it ships on its own,
|
||
immediately. S1 is independent of everything else and fixes silent content loss,
|
||
so it ships next regardless of appetite for the rest. S12 is independent but
|
||
should be decided before S8, since table pagination changes the cost model that
|
||
justifies background rendering at all. S2+S3 are small and unlock S4/S5, which
|
||
are the bulk of the work. S6/S7 are where the table actually becomes correct.
|
||
S8/S9 are integration. S10 is cleanup.
|
||
|
||
---
|
||
|
||
## S1 — Inline content in non-paragraph containers
|
||
|
||
### Problem
|
||
|
||
Inline tags (`a`, `b`, `strong`, `em`, `span`, `code`, …) are registered to
|
||
[`ignore_handler`](../pyWebLayout/io/readers/html_extraction.py#L825-L828)
|
||
because they are meant to be consumed by
|
||
[`extract_text_content()`](../pyWebLayout/io/readers/html_extraction.py#L364-L466).
|
||
But only `paragraph_handler` and `heading_handler` ever call that function.
|
||
`div_handler`, `list_item_handler`, `table_cell_handler` and
|
||
`table_header_cell_handler` iterate children and call `process_element` per
|
||
child, so inline tags return `None` and their text is discarded. `div_handler`
|
||
additionally ignores `NavigableString` children outright.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
<p>hello <b>world</b> again</p> -> Paragraph ['hello', 'world', 'again'] ok
|
||
<div>hello <b>world</b> again</div> -> (no blocks at all) lost
|
||
<li>hello <b>world</b> again</li> -> HList with no words lost
|
||
<td>hello <b>world</b> again</td> -> Table with no words lost
|
||
<td><a href="u">link</a> text</td> -> Paragraph ['text'] link lost
|
||
```
|
||
|
||
Bare text nodes that *do* survive (in `td`) each become their own `Paragraph`,
|
||
so `a <b>b</b> c` would fragment onto three lines even once `<b>` is handled.
|
||
|
||
### Design
|
||
|
||
Introduce a single helper that every block container uses to process its
|
||
children, replacing the four hand-rolled loops. It walks children in order,
|
||
accumulating consecutive *inline* children (tags and text) into a run, and
|
||
flushing that run into one `Paragraph` whenever a *block* child interrupts it or
|
||
the children are exhausted.
|
||
|
||
Inline-ness is decided by one predicate, not by each caller:
|
||
|
||
```python
|
||
# html_extraction.py
|
||
|
||
INLINE_TAGS: FrozenSet[str] = frozenset({
|
||
"a", "span", "strong", "b", "em", "i", "u", "s", "del", "ins", "mark",
|
||
"small", "sub", "sup", "code", "q", "cite", "abbr", "time", "br",
|
||
})
|
||
|
||
def is_inline(node) -> bool:
|
||
"""True for text nodes and inline tags; False for block tags."""
|
||
|
||
def process_block_children(element: Tag, context: StyleContext) -> List[Block]:
|
||
"""
|
||
Process an element's children into a block list, coalescing runs of inline
|
||
content into paragraphs.
|
||
|
||
This is the single entry point for any container that may hold a mix of
|
||
inline and block content: div, li, td, th, blockquote, figure, section...
|
||
"""
|
||
```
|
||
|
||
`process_block_children` delegates each inline run to the existing
|
||
`extract_text_content`, which already handles `<a>` → `LinkedWord`, style
|
||
nesting and background inheritance — it is called on a synthetic run rather
|
||
than on the whole element. The simplest correct implementation wraps the run's
|
||
nodes in a detached `Tag` and calls `extract_text_content` on it; that keeps
|
||
one code path for inline styling.
|
||
|
||
`<br>` inside a run terminates the current paragraph and starts a new one,
|
||
replacing the current no-op [`line_break_handler`](../pyWebLayout/io/readers/html_extraction.py#L791-L794).
|
||
|
||
Callers become one line each:
|
||
|
||
```python
|
||
def div_handler(element, context): return process_block_children(element, context)
|
||
def list_item_handler(element, context): item = ListItem(...); item._blocks = process_block_children(...)
|
||
def table_cell_handler(element, context): cell = TableCell(...); for b in process_block_children(...): cell.add_block(b)
|
||
```
|
||
|
||
`paragraph_handler`/`heading_handler` keep their current behaviour (they are
|
||
already correct); their image-splitting logic in
|
||
[html_extraction.py:492-552](../pyWebLayout/io/readers/html_extraction.py#L492-L552)
|
||
is subsumed by `process_block_children` and should be folded in — an `<img>` is
|
||
a block child, so the "text before, image after" split falls out of the general
|
||
algorithm for free.
|
||
|
||
### Acceptance criteria
|
||
|
||
- All four markup samples in *Evidence* produce the same words as the `<p>`
|
||
control, in document order, with `LinkedWord` preserved for `<a href>`.
|
||
- `<div>a <b>b</b> c</div>` yields **one** `Paragraph` of three words, not three
|
||
paragraphs.
|
||
- `<td>text<p>para</p>more</td>` yields three blocks in order: Paragraph, Paragraph,
|
||
Paragraph — inline runs on either side of a block child are not merged.
|
||
- `<td>a<br>b</td>` yields two paragraphs.
|
||
- `<p>text<img src=x>more</p>` behaves as it does today (regression).
|
||
- Round-trip test over `tests/io_tests/` fixtures shows no block-count or
|
||
word-count regressions.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/io/readers/html_extraction.py`, `tests/io_tests/test_html_extraction.py`
|
||
|
||
### Risk
|
||
|
||
Low, and contained to the reader. The change *adds* content where there was
|
||
none, so existing assertions on parsed structure may need their expected counts
|
||
raised. Grep for tests asserting `len(blocks) == N` before starting.
|
||
|
||
---
|
||
|
||
## S2 — Page geometry: origin and content rect
|
||
|
||
### Problem
|
||
|
||
Two issues, one cause: `Page` assumes it is rooted at (0,0) and has no notion of
|
||
a content rectangle.
|
||
|
||
1. **Horizontal padding is ignored.** `paragraph_layouter` sets
|
||
`x_cursor = page.border_size` ([document_layouter.py:154](../pyWebLayout/layout/document_layouter.py#L154))
|
||
while line width is `page.available_width` (which *does* subtract both
|
||
paddings). With `border_width=2, padding=(40,40,40,40)` on a 400px page, the
|
||
line lands at `origin.x = 2, width = 316`: text hugs the border and the whole
|
||
80px padding budget accumulates on the right. Vertical padding is honoured
|
||
([page.py:34](../pyWebLayout/concrete/page.py#L34)), so the asymmetry is
|
||
horizontal only.
|
||
2. **A page cannot be placed inside another page**, which is the prerequisite for
|
||
S5 (cells as sub-layouts).
|
||
|
||
### Design
|
||
|
||
Give `Page` an origin and derive a content rect from it. Default `(0, 0)` keeps
|
||
every existing caller behaviourally identical apart from the padding fix.
|
||
|
||
```python
|
||
class Page(Renderable, Queriable):
|
||
def __init__(self, size, style=None, origin: Tuple[int, int] = (0, 0)): ...
|
||
|
||
@property
|
||
def origin(self) -> np.ndarray:
|
||
"""Absolute top-left of the page box."""
|
||
|
||
@property
|
||
def content_origin(self) -> Tuple[int, int]:
|
||
"""Absolute top-left of the content box (origin + border + padding)."""
|
||
|
||
@property
|
||
def content_rect(self) -> Tuple[int, int, int, int]:
|
||
"""(x, y, w, h) of the content box in absolute coordinates."""
|
||
|
||
@property
|
||
def remaining_height(self) -> int:
|
||
"""Content-box height still available below _current_y_offset."""
|
||
```
|
||
|
||
`remaining_height` replaces the ad-hoc
|
||
`page.size[1] - page._current_y_offset - page.border_size` computed inline by
|
||
`image_layouter`, `table_layouter`, `button_layouter` and `form_layouter` — all
|
||
four of which subtract the border but not the bottom padding, so every block
|
||
type may currently be placed up to `padding_bottom` past its boundary.
|
||
|
||
The existing [`free_space()`](../pyWebLayout/concrete/page.py#L41-L43) has the
|
||
same defect (it returns full page width, and height ignoring bottom border and
|
||
padding) and has no callers; delete it in favour of `content_rect` /
|
||
`remaining_height`.
|
||
|
||
All layouters switch from `page.border_size` to `page.content_origin[0]` for the
|
||
x cursor, and `_current_y_offset` is initialised to `content_origin[1]`.
|
||
`can_fit_line`'s `max_y` becomes `content_rect.y + content_rect.h`.
|
||
|
||
`_current_y_offset` stays absolute, so no arithmetic elsewhere changes sign.
|
||
|
||
### Acceptance criteria
|
||
|
||
- With `size=(400,300), border_width=2, padding=(40,40,40,40)`: the first line
|
||
has `origin == (42, 42)` and `size[0] == 316`; rendered ink starts at
|
||
x ≥ 42 and ends at x ≤ 358.
|
||
- No block is placed within `padding_bottom` of the page bottom, for every block
|
||
type (paragraph, image, table, button, form).
|
||
- A `Page(size=(100,50), origin=(200,300))` places its first line at
|
||
`(200 + border + padding_left, 300 + border + padding_top)`.
|
||
- Existing rendering tests with default `padding=(20,20,20,20)` shift right by
|
||
20px — golden images in `docs/images/` and `test_output/` must be regenerated
|
||
and eyeballed once, deliberately, as part of this spec.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/page.py`, `pyWebLayout/concrete/dynamic_page.py`,
|
||
`pyWebLayout/layout/document_layouter.py`
|
||
|
||
### Risk
|
||
|
||
Medium — it moves every existing rendering by `padding_left`. That is the point,
|
||
but it invalidates every golden image at once. Do it in its own commit, separate
|
||
from anything else, so the diff of regenerated images is reviewable.
|
||
|
||
---
|
||
|
||
## S3 — Draw/canvas lifecycle
|
||
|
||
### Problem
|
||
|
||
[`Page.add_child`](../pyWebLayout/concrete/page.py#L140-L154) sets
|
||
`self._canvas = None` but leaves `self._draw` bound to the discarded canvas, and
|
||
the [`draw` property](../pyWebLayout/concrete/page.py#L131-L138) only rebuilds
|
||
when `_draw is None`. So after the first `add_child`, `page.draw` hands out a
|
||
draw context pointing at an orphaned image while `page._canvas` stays `None`.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
after first .draw: _canvas set? True
|
||
after add_child: _canvas set? False _draw is stale? True
|
||
page.draw returns same stale object? True page._canvas still None? True
|
||
=> a table laid out now receives canvas = None
|
||
```
|
||
|
||
Downstream: `table_layouter` passes `canvas=None` into `TableRenderer`, so every
|
||
image inside a cell silently degrades to a grey `[Image: WxH]` placeholder
|
||
([table.py:297-320](../pyWebLayout/concrete/table.py#L297-L320)).
|
||
|
||
Note the trap: naively fixing the property so it rebuilds whenever
|
||
`_canvas is None` makes layout allocate a full-page RGBA canvas on *every*
|
||
`add_child`, because layout calls `page.draw` to measure text. The fix has to
|
||
separate the two uses.
|
||
|
||
### Design
|
||
|
||
Split measurement from rendering.
|
||
|
||
```python
|
||
class Page:
|
||
@property
|
||
def measurement_draw(self) -> ImageDraw.ImageDraw:
|
||
"""
|
||
Persistent 1x1 scratch draw context used for text metrics during layout.
|
||
Never invalidated; matches the render canvas's mode so that
|
||
Text width caching keys stay consistent.
|
||
"""
|
||
|
||
@property
|
||
def draw(self) -> ImageDraw.ImageDraw:
|
||
"""Draw context bound to the live render canvas, rebuilt if invalidated."""
|
||
```
|
||
|
||
- Layouters construct `Line`/`Text` with `page.measurement_draw`.
|
||
- `Page.render_children` already rebinds `child._draw = self._draw` and
|
||
`child._canvas = self._canvas` before rendering
|
||
([page.py:256-269](../pyWebLayout/concrete/page.py#L256-L269)), so children
|
||
built against the scratch context draw onto the real canvas at render time.
|
||
That rebinding becomes load-bearing rather than incidental — document it as
|
||
such, and extend it to recurse into child pages (S5).
|
||
- `draw` rebuilds when `self._draw is None or self._canvas is None`.
|
||
|
||
Because `Text._calculate_dimensions` keys its width cache on `self._draw.mode`
|
||
([text.py](../pyWebLayout/concrete/text.py)), the scratch image must be created
|
||
in the same mode as the render canvas (`RGBA`) or the cache will hold two
|
||
entries per word.
|
||
|
||
### Acceptance criteria
|
||
|
||
- Laying out 100 paragraphs allocates **zero** full-page canvases (assert via a
|
||
counter patched onto `_create_canvas`).
|
||
- After any sequence of `add_child` calls, `page.draw.im` is the same image
|
||
object as `page._canvas`.
|
||
- A table containing an image, laid out after a paragraph, renders the real
|
||
image and not the placeholder.
|
||
- `page.render()` called twice returns pixel-identical images.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/page.py`, `pyWebLayout/layout/document_layouter.py`
|
||
|
||
---
|
||
|
||
## S4 — One block dispatch, one measurement
|
||
|
||
### Problem
|
||
|
||
There are three implementations of "lay out content in a box", which must agree
|
||
and do not:
|
||
|
||
| Purpose | Location | Word spacing | Hyphenation |
|
||
|---|---|---|---|
|
||
| Rendering | [`_render_cell_content`](../pyWebLayout/concrete/table.py#L110-L242) | `0.25–0.5 × font_size` | real, via `Line.add_word` |
|
||
| Height | [`_estimate_wrapped_lines`](../pyWebLayout/concrete/table.py#L578-L639) | `0.25 × font_size` | "assume one line" |
|
||
| Width | [`layout_cell_content`](../pyWebLayout/layout/table_optimizer.py#L117-L186) | hardcoded `(3, 6)` | none |
|
||
|
||
`layout_cell_content` additionally appends to `line._text_objects` directly,
|
||
bypassing the fitting logic it is trying to predict.
|
||
|
||
Separately, block *dispatch* is duplicated three times:
|
||
[`DocumentLayouter.layout_document`](../pyWebLayout/layout/document_layouter.py#L683-L723),
|
||
[`EreaderLayout._layout_block_on_page`](../pyWebLayout/layout/ereader_layout.py#L493-L519),
|
||
and the `isinstance` chain in `_render_cell_content`. They support different
|
||
block types, which is why tables render paragraphs but not lists, and the
|
||
ereader renders neither.
|
||
|
||
### Design
|
||
|
||
Two new public functions, one module each.
|
||
|
||
**Dispatch** — `pyWebLayout/layout/block_layouter.py`:
|
||
|
||
```python
|
||
@dataclass
|
||
class LayoutResult:
|
||
complete: bool # block fully placed
|
||
next_word: Optional[int] = None # resume index if not complete
|
||
pretext: Optional[Text] = None # hyphenated remainder
|
||
next_item: Optional[int] = None # resume index for lists
|
||
next_row: Optional[int] = None # resume index for tables
|
||
|
||
def layout_block(block: Block,
|
||
page: Page,
|
||
start_word: int = 0,
|
||
pretext: Optional[Text] = None,
|
||
**resume) -> LayoutResult:
|
||
"""
|
||
Place one abstract block onto a page, starting from a resume point.
|
||
The single dispatch point for Paragraph, Heading, Image, Table, HList,
|
||
Quote, CodeBlock, HorizontalRule, PageBreak, Button, Form.
|
||
"""
|
||
```
|
||
|
||
The existing `paragraph_layouter`, `image_layouter`, `table_layouter`,
|
||
`button_layouter`, `form_layouter` stay as the per-type implementations;
|
||
`layout_block` is the registry in front of them. `DocumentLayouter`,
|
||
`EreaderLayout` and the cell layout of S5 all call it, and gaining a block type
|
||
means adding one entry, once.
|
||
|
||
**Measurement** — `pyWebLayout/layout/measure.py`:
|
||
|
||
```python
|
||
@dataclass
|
||
class BlockMeasure:
|
||
min_width: int # narrowest the block can be without overflow
|
||
max_width: int # width at which the block never wraps
|
||
|
||
def measure_block(block: Block, style_ctx: RenderingContext) -> BlockMeasure:
|
||
"""Intrinsic width demands of a block, per the CSS min-content/max-content model."""
|
||
```
|
||
|
||
Rules per type:
|
||
- `Paragraph`/`Heading`: `min_width` = widest unbreakable run — a word wider
|
||
than its font's `min_hyphenation_width` contributes its longest hyphenation
|
||
fragment, not its full width; `max_width` = Σ word widths + (n−1) × min spacing.
|
||
- `Image`: intrinsic width for both.
|
||
- `Table`: recurse (nested tables measure through the S6 grid).
|
||
- `HList`: item measure + marker indent.
|
||
|
||
Measurement uses each word's *own* style, so it is correct for mixed formatting
|
||
— unlike all three current implementations, which assume one font for the box.
|
||
|
||
Heights are **not** part of measurement. Height comes from running
|
||
`layout_block` at the final assigned width and reading `_current_y_offset`.
|
||
`_estimate_wrapped_lines` and `layout_cell_content` are deleted, and
|
||
`DynamicPage.get_min_width`/`get_preferred_width` are reimplemented in terms of
|
||
`measure_block` (they currently walk `Line._text_objects`, which only works if
|
||
someone has pre-poked the lines).
|
||
|
||
### Acceptance criteria
|
||
|
||
- `_estimate_wrapped_lines` and `layout_cell_content` no longer exist.
|
||
- Property test: for a corpus of paragraphs and widths, laying out at
|
||
`measure_block(b).max_width` produces exactly one line, and laying out at
|
||
`min_width` produces no line that overflows its box.
|
||
- `layout_block` handles every type in the union above; adding a type to the
|
||
registry makes it work in documents, cells and the ereader simultaneously
|
||
(assert with one test that exercises all three call sites).
|
||
- Mixed-font paragraph: a paragraph whose words have different sizes measures
|
||
wider than the same word count at the smallest size.
|
||
|
||
### Files
|
||
|
||
new `pyWebLayout/layout/block_layouter.py`, new `pyWebLayout/layout/measure.py`,
|
||
`pyWebLayout/layout/document_layouter.py`, `pyWebLayout/layout/table_optimizer.py`,
|
||
`pyWebLayout/concrete/dynamic_page.py`, `pyWebLayout/concrete/table.py`
|
||
|
||
### Risk
|
||
|
||
Largest spec by volume, but mostly deletion. Land `measure.py` first with tests
|
||
against the current optimizer's outputs to establish a baseline, then swap the
|
||
optimizer over, then delete.
|
||
|
||
---
|
||
|
||
## S5 — Cells as sub-layouts
|
||
|
||
### Problem
|
||
|
||
[`TableCellRenderer._render_cell_content`](../pyWebLayout/concrete/table.py#L110-L242)
|
||
hand-rolls line breaking and handles only `Paragraph`, `Heading` and `Image`
|
||
([table.py:141-146](../pyWebLayout/concrete/table.py#L141-L146)); lists, nested
|
||
tables, quotes, code blocks, buttons and forms are silently dropped. It also
|
||
discards styling: it hardcodes
|
||
`/usr/share/fonts/truetype/dejavu/DejaVuSans.ttf` (a Debian path — **it does not
|
||
exist on the dev machine**; 6 such literals across the package) and
|
||
`font_size = 12`, then rebuilds every word as a plain `Word(text, font)`
|
||
([table.py:156-171](../pyWebLayout/concrete/table.py#L156-L171)), throwing away
|
||
bold/italic/colour/size, the bundled-font system, the font-family override, and
|
||
`LinkedWord` link targets.
|
||
|
||
`DynamicPage` — the abstraction that exists precisely for this — is used only
|
||
for measurement, despite `3bcd1bf` claiming cells can hold anything.
|
||
|
||
### Design
|
||
|
||
A cell owns a `DynamicPage` positioned at the cell's content origin, sharing the
|
||
parent's canvas, filled by `layout_block` (S4).
|
||
|
||
```python
|
||
class TableCellRenderer(Box):
|
||
def __init__(self, cell, origin, size, style, is_header_section=False):
|
||
self._page = DynamicPage(
|
||
style=PageStyle(padding=style.cell_padding, border_width=0,
|
||
background_color=...),
|
||
origin=origin, # S2
|
||
)
|
||
self._page.layout(size)
|
||
for block in cell.blocks():
|
||
layout_block(block, self._page) # S4
|
||
|
||
def render(self):
|
||
self._draw_background_and_border()
|
||
self._page.render_children() # children already positioned absolutely
|
||
```
|
||
|
||
Consequences that fall out rather than being coded:
|
||
|
||
- **Arbitrary content works.** Nested tables, lists, buttons — anything in the
|
||
S4 registry. Invariant 4 is satisfied structurally, not by a type switch.
|
||
- **Styling is preserved**, because `layout_block` uses each word's own style.
|
||
Header cells get bold via `get_or_create_font(weight=FontWeight.BOLD)`
|
||
([core/base.py:186](../pyWebLayout/core/base.py#L186)) rather than by
|
||
substituting a font *file path*. No absolute paths outside `style/fonts.py`.
|
||
- **Cell height is the laid-out height** (`_page._current_y_offset - origin.y`),
|
||
so S6's row heights are exact by construction (invariant 3).
|
||
- **Overflow is bounded**: `can_fit_line` against the cell page's content rect
|
||
stops content at the cell edge instead of painting over neighbours.
|
||
|
||
`TableCellRenderer` keeps its constructor shape so `TableRowRenderer` is
|
||
unaffected, but loses the `draw`/`canvas` parameters — the canvas arrives at
|
||
render time via the S3 rebinding, which must now recurse into child pages:
|
||
|
||
```python
|
||
def render_children(self):
|
||
for child in self._children:
|
||
if isinstance(child, Page):
|
||
child.attach_surface(self._canvas, self._draw) # recurse
|
||
...
|
||
```
|
||
|
||
### Acceptance criteria
|
||
|
||
- A cell containing an `HList`, a nested `Table`, a `Quote` and a `Button`
|
||
renders all four (today: a bare list and a nested table draw nothing but the
|
||
cell border).
|
||
- A cell whose words carry bold/italic/24px/red styling renders with those
|
||
attributes; assert on the `Text` objects' `_style`, not on pixels.
|
||
- `grep -rn "/usr/share/fonts" pyWebLayout/` returns 0 hits.
|
||
- A cell containing a `LinkedWord` produces a `LinkText` concrete object
|
||
(prerequisite for S9).
|
||
- Cell content that exceeds the cell's height is clipped at the cell boundary
|
||
and does not overpaint the next row.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/table.py`, `pyWebLayout/concrete/dynamic_page.py`,
|
||
`pyWebLayout/concrete/page.py`
|
||
|
||
---
|
||
|
||
## S6 — Table grid model
|
||
|
||
### Problem
|
||
|
||
Four separate geometry defects:
|
||
|
||
1. **`size` is wrong.** [table.py:445-446](../pyWebLayout/concrete/table.py#L445-L446)
|
||
computes total height as `sum(self._row_heights.values())`, but `_row_heights`
|
||
is a **three-entry dict** (`header`/`body`/`footer`), not one entry per row.
|
||
A 10-row table reports height **44px**; its bottom-most drawn pixel is at
|
||
**y=409**. The caption is drawn but not counted either.
|
||
2. **Uniform row heights.** [`_calculate_row_height_for_section`](../pyWebLayout/concrete/table.py#L499-L576)
|
||
takes the max across all rows in a section, so one verbose cell inflates every
|
||
row in the table.
|
||
3. **colspan is not counted.** [`get_column_count`](../pyWebLayout/layout/table_optimizer.py#L189-L205)
|
||
returns `first_row.cell_count`. For `<tr><td colspan=2>` followed by
|
||
`<tr><td><td><td>` it returns **1**, and
|
||
[`TableRowRenderer.render`](../pyWebLayout/concrete/table.py#L383) silently
|
||
drops every cell past `len(column_widths)` — two of three cells never render.
|
||
4. **rowspan is parsed and stored but never read** by any renderer or measurer;
|
||
spanned rows just shift left.
|
||
|
||
### Design
|
||
|
||
A resolved grid, built once, that every other stage consumes.
|
||
|
||
```python
|
||
# pyWebLayout/layout/table_grid.py
|
||
|
||
@dataclass(frozen=True)
|
||
class GridCell:
|
||
cell: TableCell
|
||
row: int # resolved row index across all sections
|
||
col: int # resolved column index
|
||
colspan: int
|
||
rowspan: int
|
||
section: str # "header" | "body" | "footer"
|
||
|
||
class TableGrid:
|
||
"""
|
||
Occupancy-resolved view of an abstract Table.
|
||
|
||
Built by the standard HTML table algorithm: walk rows in order, maintaining a
|
||
set of slots occupied by open rowspans, placing each cell at the first free
|
||
column and marking its span.
|
||
"""
|
||
n_cols: int # max over rows of sum(colspan), not first row's cell count
|
||
n_rows: int
|
||
def cells(self) -> Iterator[GridCell]: ...
|
||
def row(self, index: int) -> List[GridCell]: ...
|
||
def cell_at(self, row: int, col: int) -> Optional[GridCell]: ...
|
||
```
|
||
|
||
**Column widths.** `optimize_table_layout` takes a `TableGrid` and per-cell
|
||
`measure_block` results (S4). Span distribution follows CSS: a cell spanning *k*
|
||
columns imposes its demand on the *sum* of those columns, and only widens them
|
||
(proportionally to their current demand) if the sum falls short. Single-column
|
||
demands are applied first so spanning cells cannot dominate.
|
||
|
||
**Row heights.** Per row, not per section: lay out every cell in the row at its
|
||
assigned width (S5) and take the max of the resulting content heights. A cell
|
||
with `rowspan = k` contributes to row *r+k−1* only if the accumulated height of
|
||
rows *r…r+k−1* is less than the cell needs, and the shortfall is distributed
|
||
across those rows.
|
||
|
||
**Size.** `TableRenderer._size` = caption height + Σ per-row heights + borders,
|
||
computed from the same per-row heights that `render()` walks. The two must come
|
||
from one list; a test asserts they cannot diverge.
|
||
|
||
### Acceptance criteria
|
||
|
||
- `<tr><td colspan=2></tr><tr><td><td><td></tr>` → `grid.n_cols == 3`, and all
|
||
four cells render.
|
||
- `<td rowspan=2>` spans two rows vertically; the cell below it in the next row
|
||
is not shifted left.
|
||
- 10-row table: `renderer.size[1]` equals `last_drawn_pixel_y - origin_y + border`
|
||
(±1 for border rounding). This is the direct regression test for defect 1 —
|
||
today it is 44 vs 409.
|
||
- A table with one tall row and nine short ones is shorter than 10 × tall row.
|
||
- A captioned table's `size[1]` includes the caption.
|
||
- Fuzz: for randomly generated colspan/rowspan tables, no two rendered cells'
|
||
rectangles overlap, and every cell in the abstract table appears exactly once
|
||
in the grid.
|
||
|
||
### Files
|
||
|
||
new `pyWebLayout/layout/table_grid.py`, `pyWebLayout/layout/table_optimizer.py`,
|
||
`pyWebLayout/concrete/table.py`
|
||
|
||
---
|
||
|
||
## S7 — Retained-mode table rendering
|
||
|
||
### Problem
|
||
|
||
Every other block type is retained-mode: layouters build objects, `add_child`
|
||
them, and `Page.render()` draws them onto a fresh canvas. Tables are
|
||
immediate-mode: [`table_layouter`](../pyWebLayout/layout/document_layouter.py#L351-L402)
|
||
grabs `page.draw`, paints directly, bumps `_current_y_offset`, and never adds
|
||
anything to `page._children`.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
layout_table -> True
|
||
pixels on canvas right after layout: 28328
|
||
pixels after page.render(): 0
|
||
children on page: 0
|
||
```
|
||
|
||
`Page.render()` rebuilds the canvas ([page.py:279](../pyWebLayout/concrete/page.py#L279))
|
||
and the table is not a child, so it is erased. Lay out a table *and* a
|
||
paragraph, render, and only the paragraph survives. The examples appear to work
|
||
only because they read `page._canvas` directly instead of calling `render()`.
|
||
|
||
Because `size` lies (S6), the fit check
|
||
`if table_height > available_height: return False` is also meaningless: a ~950px
|
||
table "fits" a 300px page, draws past the border, and leaves
|
||
`_current_y_offset = 64` so the next paragraph overlaps it.
|
||
|
||
### Design
|
||
|
||
- `TableRenderer.__init__` measures only — it builds the grid, resolves widths,
|
||
lays out cells (S5) and computes `size`. It draws nothing and takes no
|
||
`draw`/`canvas` parameters.
|
||
- `TableRenderer.render()` draws, and is called by `Page.render_children()`.
|
||
- `table_layouter` becomes structurally identical to `image_layouter`:
|
||
|
||
```python
|
||
def table_layouter(table, page, style=None) -> bool:
|
||
renderer = TableRenderer(table, origin=(page.content_origin[0], page._current_y_offset),
|
||
available_width=page.available_width, style=style)
|
||
if renderer.size[1] > page.remaining_height:
|
||
return False # honest check, honest size (S6)
|
||
page.add_child(renderer) # retained mode
|
||
return True
|
||
```
|
||
|
||
- `_row_renderers` / `_cell_renderers` are rebuilt per `render()` call, not
|
||
appended to (today they grow without bound on re-render).
|
||
|
||
### Acceptance criteria
|
||
|
||
- `layout_table(...)` then `page.render()` yields a canvas containing the table.
|
||
- `page.render()` twice → pixel-identical images.
|
||
- Table + following paragraph: both present, non-overlapping, paragraph starts
|
||
below `table.size[1]`.
|
||
- A table taller than the remaining space returns `False` and adds no child.
|
||
- `len(page.children) == 1` after laying out one table.
|
||
- Rendering the same `TableRenderer` five times leaves
|
||
`len(renderer._row_renderers)` equal to the row count.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/table.py`, `pyWebLayout/layout/document_layouter.py`
|
||
|
||
---
|
||
|
||
## S8 — Table and list pagination
|
||
|
||
### Problem
|
||
|
||
[`EreaderLayout._layout_table_on_page`](../pyWebLayout/layout/ereader_layout.py#L598-L611)
|
||
skips tables outright ("For now, skip tables"), and `_layout_list_on_page` does
|
||
the same for lists. The ereader — the library's main consumer — renders neither.
|
||
`RenderingPosition` already carries `table_row`, `table_col` and
|
||
`list_item_index` ([ereader_layout.py:38-40](../pyWebLayout/layout/ereader_layout.py#L33-L45)),
|
||
so the state model anticipated this; only the layout half is missing.
|
||
`examples/13_table_pagination_demo.py` documents pagination behaviour that the
|
||
library does not implement.
|
||
|
||
### Design
|
||
|
||
With S4 (`layout_block` returning a resume point) and S6/S7 (honest per-row
|
||
geometry), pagination is row-splitting rather than new machinery.
|
||
|
||
```python
|
||
def table_layouter(table, page, style=None, start_row: int = 0) -> LayoutResult:
|
||
"""
|
||
Place as many rows as fit from start_row.
|
||
Returns complete=False with next_row set when the table is split.
|
||
"""
|
||
```
|
||
|
||
Rules:
|
||
- Split only on row boundaries. A single row taller than a full page is placed
|
||
anyway and clipped — with a `log.warning`, not silently.
|
||
- Header rows repeat at the top of each continuation fragment. This is a
|
||
`TableStyle` flag (`repeat_header: bool = True`), because for a two-row table
|
||
it is noise.
|
||
- Column widths are resolved once for the whole table and reused across
|
||
fragments, so columns line up across pages.
|
||
|
||
`EreaderLayout._layout_table_on_page` / `_layout_list_on_page` delegate to
|
||
`layout_block` and map `LayoutResult.next_row` / `next_item` onto
|
||
`RenderingPosition`. The existing bidirectional navigation
|
||
(`render_page_backward`) must round-trip through table positions — that is the
|
||
part most likely to bite, so it gets its own test.
|
||
|
||
### Acceptance criteria
|
||
|
||
- A 60-row table across a 3-page document renders every row exactly once,
|
||
no duplicates, no gaps.
|
||
- Forward then backward navigation across a table returns to the identical
|
||
starting position (extends `tests/layout/test_navigation_consistency.py`).
|
||
- With `repeat_header=True`, every fragment starts with the header row.
|
||
- A serialized `RenderingPosition` inside a table restores to the same page after
|
||
reload.
|
||
- Lists paginate mid-list and resume at the right item, with markers continuing
|
||
their numbering.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/document_layouter.py`,
|
||
`pyWebLayout/concrete/table.py`
|
||
|
||
### Risk
|
||
|
||
The ereader's backward-navigation estimator
|
||
([ereader_layout.py:400-465](../pyWebLayout/layout/ereader_layout.py#L430-L465))
|
||
assumes blocks are cheap to re-lay-out repeatedly. Tables are not. Expect to
|
||
need a per-table layout cache keyed on `(table_id, available_width, font_scale)`
|
||
before this is usable at speed.
|
||
|
||
---
|
||
|
||
## S9 — Interactivity inside tables
|
||
|
||
### Problem
|
||
|
||
Nothing inside a table is hit-testable: `query_point` into a rendered table
|
||
returns `object_type="empty"`. Links, images and buttons in cells are invisible
|
||
to the query/selection/callback system the rest of the library is built on.
|
||
`examples/14_interactive_table.py` works around this by hand-painting buttons
|
||
*on top of* the table and doing its own coordinate maths — the demo fakes the
|
||
feature. Meanwhile `TableCellRenderer` sets bounds on `InteractiveImage`
|
||
([table.py:322-327](../pyWebLayout/concrete/table.py#L322-L327)), a third,
|
||
parallel interaction mechanism.
|
||
|
||
### Design
|
||
|
||
S5 and S7 make this mostly free: cells are pages, tables are children, and
|
||
`Page.query_point` already recurses into children that implement `query_point`
|
||
([page.py:345-358](../pyWebLayout/concrete/page.py#L345-L358)).
|
||
|
||
Required:
|
||
- `TableRenderer` and `TableCellRenderer` implement `Queriable.in_object` (they
|
||
are `Box` subclasses, so bounds already exist) and `query_point`, delegating to
|
||
the cell's `DynamicPage`.
|
||
- Because child pages are positioned in absolute coordinates (S2), **no
|
||
coordinate translation is needed** — a translating implementation would be a
|
||
sign S2 was not applied.
|
||
- Callback registration cascades: a cell page's `CallbackRegistry` merges into
|
||
the owning page's registry at `add_child` time, so `page.callbacks` remains the
|
||
single lookup point.
|
||
- `InteractiveImage.set_rendered_bounds` becomes redundant for the table path;
|
||
keep it for direct users but stop calling it from the cell renderer.
|
||
|
||
### Acceptance criteria
|
||
|
||
- `page.query_point(p)` for a point over a link in a cell returns
|
||
`object_type == "link"` with the correct `link_target`.
|
||
- A `Button` in a cell fires its callback through the normal
|
||
`page.callbacks` path.
|
||
- `examples/14_interactive_table.py` is rewritten to put real `Button` blocks in
|
||
cells and delete its manual overlay maths — the example shrinks substantially,
|
||
which is the acceptance signal.
|
||
- `query_range` selection spanning a table returns the cell text in document
|
||
order.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/table.py`, `pyWebLayout/concrete/page.py`,
|
||
`pyWebLayout/core/callback_registry.py`, `examples/14_interactive_table.py`
|
||
|
||
---
|
||
|
||
## S10 — Contracts and hygiene
|
||
|
||
Small independent items. Each is a one-commit change; none blocks the others.
|
||
|
||
### 10.1 Render contract
|
||
|
||
`TableRenderer.render()`, `TableRowRenderer.render()` and
|
||
`TableCellRenderer.render()` are annotated `-> Image.Image` and all
|
||
`return None`. After S7, decide and enforce one contract: `render()` draws onto
|
||
the bound canvas and returns `None`; only `Page.render()` returns an image.
|
||
Update `Renderable.render`'s docstring in
|
||
[core/base.py:16-22](../pyWebLayout/core/base.py#L16-L22), which currently
|
||
promises a `PIL.Image`, and annotate accordingly.
|
||
|
||
### 10.2 Abstract/concrete leak
|
||
|
||
[concrete/__init__.py:18](../pyWebLayout/concrete/__init__.py) re-exports the
|
||
**abstract** `Table, TableRow as Row, TableCell as Cell` from the concrete
|
||
package, aliased to look concrete, while `TableRenderer` is not exported at all.
|
||
This contradicts `ARCHITECTURE.md`. Remove the re-export, export the renderers,
|
||
and add the `Cell`/`Row` names to a deprecation shim for one release if anything
|
||
external depends on them.
|
||
|
||
### 10.3 Dead code
|
||
|
||
- [table.py:229-242](../pyWebLayout/concrete/table.py#L229-L242): fallback
|
||
reading `self._cell._text_content`, an attribute that exists nowhere.
|
||
- `TableCellRenderer._children`: written never, read never.
|
||
- `dynamic_page.py`: `import numpy as np` unused;
|
||
`render_partial`/`has_more_content`/`reset_pagination` are an unused
|
||
pagination API superseded by S8 — delete or wire up, do not leave both.
|
||
- [document_layouter.py:157-161](../pyWebLayout/layout/document_layouter.py#L157-L161):
|
||
`temp_text.width` computed and discarded; `else: pass`.
|
||
|
||
### 10.4 Exception handling
|
||
|
||
13 bare `except Exception:` / `except BaseException:` in the package. The worst
|
||
is [table.py:331-333](../pyWebLayout/concrete/table.py#L331-L333), which swallows
|
||
every image failure into a 20px gap with no diagnostic. Policy: catch the
|
||
specific exception, log at `warning` with `exc_info=True`, and render a visible
|
||
placeholder for content failures. `except BaseException` (which catches
|
||
`KeyboardInterrupt`) is never correct here.
|
||
|
||
### 10.5 Image source resolution
|
||
|
||
[`_render_image_in_cell`](../pyWebLayout/concrete/table.py#L252-L266) probes six
|
||
attribute names (`source`, `_source`, `path`, `src`, `_path`, `_src`) to find an
|
||
image path. Give `abstract.block.Image` one documented accessor and use it.
|
||
|
||
### Acceptance criteria
|
||
|
||
- `grep -rn "except BaseException" pyWebLayout/` → 0 hits.
|
||
- No `hasattr` chain longer than one alternative anywhere in `concrete/table.py`.
|
||
- `python -W error -c "import pyWebLayout"` clean; flake8 reports no unused
|
||
imports in touched files.
|
||
|
||
---
|
||
|
||
## S11 — Partial-block progress is discarded
|
||
|
||
### Problem
|
||
|
||
`BidirectionalLayouter.render_page_forward` discards the resume position of a
|
||
block that only partially fitted:
|
||
|
||
```python
|
||
success, new_pos = self._layout_block_on_page(scaled_block, page, current_pos, font_scale)
|
||
if not success:
|
||
# Block doesn't fit, we're done with this page
|
||
break # <-- new_pos dropped on the floor
|
||
...
|
||
current_pos = new_pos
|
||
return page, current_pos
|
||
```
|
||
[ereader_layout.py:341-347](../pyWebLayout/layout/ereader_layout.py#L330-L362)
|
||
|
||
`paragraph_layouter` correctly returns the index of the first word that did not
|
||
fit, and `_layout_paragraph_on_page` correctly packs it into `new_pos.word_index`
|
||
([ereader_layout.py:570-582](../pyWebLayout/layout/ereader_layout.py#L570-L582)).
|
||
The information exists and is thrown away one frame up the stack. The returned
|
||
"next position" is therefore the *same* position the page started at.
|
||
|
||
The bug is invisible for a document whose paragraphs each fit on a page — the
|
||
`not success` path is only taken when nothing more fits, which for normal prose
|
||
means the block boundary. It bites exactly when one block spans a page boundary,
|
||
i.e. any paragraph longer than a page.
|
||
|
||
### Evidence
|
||
|
||
A 2877-word paragraph at 800×600:
|
||
|
||
```
|
||
_layout_block_on_page -> success=False, new_pos.word_index=271 (start was 0)
|
||
render_page_forward -> next position (b0, w0) (start was b0, w0)
|
||
|
||
page 0: start(b0,w0) -> next(b0,w0) lines=26 DEAD END
|
||
```
|
||
|
||
The page renders 26 lines of real content, then reports that the reader has made
|
||
no progress. Navigation is stuck on that page permanently; the book is
|
||
unreadable from that block onward.
|
||
|
||
### Design
|
||
|
||
Distinguish *nothing placed* from *something placed*. Only the former should
|
||
leave the position untouched.
|
||
|
||
```python
|
||
if not success:
|
||
if self._position_compare(new_pos, current_pos) > 0:
|
||
# Partially placed: keep the progress, end the page here.
|
||
current_pos = new_pos
|
||
break
|
||
```
|
||
|
||
`_position_compare` already exists and is used by the backward navigation
|
||
([ereader_layout.py:439](../pyWebLayout/layout/ereader_layout.py#L439)), so
|
||
ordering semantics stay in one place.
|
||
|
||
Two supporting changes, because a silent dead-end should not be possible again:
|
||
|
||
1. **A no-progress guard in the navigation loop.** `EreaderManager.next_page`
|
||
asserts that the returned position is strictly greater than the requested
|
||
one; if not, it logs an error naming the block index and force-advances
|
||
`block_index += 1`. A malformed block should cost the reader one block, not
|
||
the rest of the book.
|
||
2. **The same audit for the backward path.** `render_page_backward`'s refinement
|
||
loop ([ereader_layout.py:400-465](../pyWebLayout/layout/ereader_layout.py#L430-L465))
|
||
already has fallbacks for failing to move backward, which is the symmetric
|
||
symptom — check whether those fallbacks were compensating for this bug and
|
||
simplify them if so.
|
||
|
||
### Acceptance criteria
|
||
|
||
- A 2877-word single-paragraph document paginates to completion: 12 pages at
|
||
800×600, every word appearing exactly once, positions strictly increasing.
|
||
(Verified against the patched implementation: pages advance
|
||
w0 → w271 → w531 → … → w2684 → end.)
|
||
- Forward-then-backward across a page-spanning paragraph round-trips to the
|
||
original position.
|
||
- A block that genuinely places nothing (e.g. an image taller than the page on
|
||
an empty page) still returns the unchanged position, and the no-progress guard
|
||
force-advances it with a logged error rather than looping.
|
||
- Regression test asserts `next_position > position` for **every** page of a
|
||
full-book pagination run over the EPUB fixtures.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/ereader_manager.py`
|
||
|
||
### Risk
|
||
|
||
Low, and it makes pagination strictly more correct. Watch for page-count changes
|
||
in `tests/layout/test_navigation_consistency.py` — any fixture with a
|
||
page-spanning paragraph will now produce more pages, which is the fix working.
|
||
|
||
---
|
||
|
||
## S12 — Background rendering
|
||
|
||
### Problem
|
||
|
||
`PageBuffer` starts a `ProcessPoolExecutor(max_workers=4)`
|
||
([page_buffer.py:117](../pyWebLayout/layout/page_buffer.py#L117)) and submits
|
||
page renders to it. **Every job fails.** `_render_page_worker` returns
|
||
`pickle.dumps(page)` ([page_buffer.py:50](../pyWebLayout/layout/page_buffer.py#L50)),
|
||
and a `Page` holds a live PIL canvas, which is not picklable.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
args pickle OK: 877.5 KB in 21 ms # submit succeeds
|
||
real pool round trip -> job FAILED: TypeError cannot pickle 'ImagingCore' object
|
||
```
|
||
|
||
So the cost is paid in full and the benefit is zero: 4 forked processes, the
|
||
entire block list pickled and shipped per job (~880KB for a 200-block document),
|
||
a full page laid out in the worker — then the result is thrown away by
|
||
`check_completed_renders`, which swallows the exception into a bare `print`
|
||
([page_buffer.py:274-276](../pyWebLayout/layout/page_buffer.py#L274-L276)). On a
|
||
4-core Pi with 512MB this is actively harmful: four interpreter copies plus four
|
||
copies of the book, to populate a cache that never populates.
|
||
|
||
Four further defects in the same file, which matter only if the decision is to
|
||
keep it:
|
||
|
||
1. `_render_page_worker` builds `BidirectionalLayouter(blocks, page_style, font_family_override=...)`
|
||
**without `page_size`**, so it silently uses the default `(800, 600)`
|
||
([page_buffer.py:44](../pyWebLayout/layout/page_buffer.py#L44)). Fixing the
|
||
pickling alone would poison the cache with wrong-size pages.
|
||
2. `check_completed_renders` caches every result with `is_backward=False`
|
||
([page_buffer.py:268](../pyWebLayout/layout/page_buffer.py#L268)), so backward
|
||
renders land in the forward buffer.
|
||
3. `_queue_forward_renders` / `_queue_backward_renders` `break` at the end of
|
||
their first loop body, so despite `for i in range(self.buffer_size)` they
|
||
queue at most one page each.
|
||
4. Failures are reported with `print`, not the logger.
|
||
|
||
### Design
|
||
|
||
**Recommendation: delete the process pool.** Replace it with synchronous
|
||
readahead, gated on a measurement.
|
||
|
||
The justification for multiprocessing was sub-second navigation. That premise
|
||
predates the text caches now landing in `concrete/text.py`, whose own
|
||
measurements put a page at ~30ms on desktop once warm, with
|
||
`EreaderManager.prewarm_caches` ([ereader_manager.py:222](../pyWebLayout/layout/ereader_manager.py#L222))
|
||
priming the working set at open time. If a page costs tens of milliseconds, a
|
||
process pool cannot pay for its own IPC, let alone its memory on the target
|
||
device.
|
||
|
||
```python
|
||
class PageBuffer:
|
||
def __init__(self, buffer_size: int = 5):
|
||
"""LRU page cache with synchronous readahead. No worker processes."""
|
||
|
||
def readahead(self, position: RenderingPosition, n: int = 1) -> None:
|
||
"""
|
||
Render and cache the next n pages on the calling thread.
|
||
Called after a navigation completes, when the reader is idle.
|
||
"""
|
||
```
|
||
|
||
This keeps the LRU buffers, the position maps and the invalidation logic — all
|
||
of which are fine — and removes the executor, the worker function, the pickling
|
||
and the `threading.Lock` that only guarded the pending-render dict.
|
||
|
||
**Gate:** measure first, on the Pi, with the caches warm. Record page render time
|
||
at the target page size in the spec's PR description. If p95 is under ~150ms,
|
||
delete the pool. If it is materially worse, the fallback is a **single worker
|
||
thread**, not processes: layout is PIL-bound, PIL releases the GIL for
|
||
rasterisation, and a thread shares the block list instead of copying it. Fixing
|
||
the process pool properly would require making the entire concrete tree
|
||
picklable (Page → Line → Text → Font → `FreeTypeFont`), which is a large amount
|
||
of surface area to maintain for a cache.
|
||
|
||
Whichever way the gate goes, defects 1–4 above are fixed or deleted with the code
|
||
that contains them, and failures are logged with `exc_info=True` rather than
|
||
printed.
|
||
|
||
### Acceptance criteria
|
||
|
||
- No `ProcessPoolExecutor` in the package, or — if the gate says keep it — a test
|
||
that submits a real job through a real pool and asserts it **succeeds**. The
|
||
absence of such a test is why this shipped broken.
|
||
- Measured page-render p95 on the target device recorded in the PR, before and
|
||
after.
|
||
- Peak RSS while paginating a full book drops by roughly the pool's share
|
||
(expect ~4 interpreter copies' worth on a 4-core device).
|
||
- Readahead of *n* pages populates the cache with *n* pages (today: one queued,
|
||
zero cached).
|
||
- Backward-rendered pages land in the backward buffer.
|
||
- Cache invalidation on font-scale and font-family change still clears both
|
||
buffers.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/page_buffer.py`, `pyWebLayout/layout/ereader_manager.py`
|
||
|
||
### Risk
|
||
|
||
Low. The feature currently contributes nothing but overhead, so removing it
|
||
cannot regress rendering; the only risk is navigation latency, which is what the
|
||
gate measures.
|
||
|
||
---
|
||
|
||
## Test plan
|
||
|
||
Findings were reproduced with four probe scripts; each becomes a regression test
|
||
rather than being thrown away.
|
||
|
||
| Regression test | Guards | Currently |
|
||
|---|---|---|
|
||
| `test_table_survives_page_render` | S7 | fails (0 px after render) |
|
||
| `test_table_size_matches_drawn_extent` | S6 | fails (44 vs 409) |
|
||
| `test_table_fit_check_rejects_oversized` | S6/S7 | fails (returns True) |
|
||
| `test_colspan_column_count` | S6 | fails (1 vs 3) |
|
||
| `test_rowspan_occupancy` | S6 | fails (ignored) |
|
||
| `test_inline_content_in_div_li_td` | S1 | fails (content lost) |
|
||
| `test_page_draw_not_stale_after_add_child` | S3 | fails (stale) |
|
||
| `test_horizontal_padding_honoured` | S2 | fails (x=2 vs 42) |
|
||
| `test_arbitrary_blocks_in_cell` | S5 | fails (dropped) |
|
||
| `test_query_point_into_table_cell` | S9 | fails ("empty") |
|
||
| `test_page_spanning_paragraph_advances` | S11 | fails (dead-ends at page 0) |
|
||
| `test_background_render_job_succeeds` | S12 | fails (unpicklable Page) |
|
||
|
||
Note that `tests/concrete/test_table_rendering.py:541-553` currently asserts only
|
||
`height > 0`, which is why the size defect survived. Assertions of that shape
|
||
should be replaced wherever the tests touch geometry.
|
||
|
||
Golden images in `docs/images/` are regenerated **once** under S2 and reviewed
|
||
deliberately; after that they are treated as fixtures and any diff is a
|
||
regression.
|
||
|
||
## Out of scope
|
||
|
||
Called out so their absence is a decision rather than an oversight:
|
||
|
||
- CSS percentage widths in `parse_html_width`
|
||
([table_optimizer.py:283-284](../pyWebLayout/layout/table_optimizer.py#L283-L284)) —
|
||
needs a containing-block model.
|
||
- `border-collapse: separate`, per-cell borders, per-cell background colour.
|
||
- Vertical alignment within cells (`valign`); everything is top-aligned.
|
||
- RTL and vertical writing modes.
|
||
- Floats and absolute positioning.
|