dtourolle/pyWebLayout
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Update coverage badges [skip ci]

Browse Source
This commit is contained in:
Gitea Action
2026-08-08 20:35:15 +00:00
commit 735face593
312 changed files with 91102 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/ARCHITECTURE_REVIEW.md
+595
View File
@@ -0,0 +1,595 @@
# Architecture Review
Independent review of the codebase at `c5c61a3` (2026-08-06), answering one
question: **is this a well-architected library or an over-complex mess?**
It is a well-architected library with one rotten subsystem inside it. The core
design holds up; roughly a fifth of the code is speculative or non-functional,
and it is concentrated in the ereader pagination/buffering layer.
This document records the verdict, the evidence, and the findings **not already
covered** by [LAYOUT_REMEDIATION_SPEC.md](LAYOUT_REMEDIATION_SPEC.md). Where a
finding is already specced, it is cross-referenced rather than restated.
## Contents
| ID | Finding | Severity | Status |
|----|---------|----------|--------|
| [R1](#r1--the-process-pool-crashes-on-python-314) | The process pool crashes on Python 3.14 | Critical | New; raises priority of S12 |
| [R2](#r2--the-test-suite-hangs-at-interpreter-exit) | Test suite hangs at interpreter exit | High | New; same root cause as R1 |
| [R3](#r3--font-scaling-destroys-hyperlinks) | Font scaling destroys hyperlinks | High | New |
| [R4](#r4--three-packaging-configs-that-disagree) | Three packaging configs that disagree | Medium | New (corrected) |
| [R5](#r5--monkey-patched-page-methods-with-a-conflicting-signature) | Monkey-patched `Page` methods with a conflicting signature | Medium | New |
| [R6](#r6--dead-duck-typing-cluster-in-pagepy) | Dead duck-typing cluster in `page.py` | Low | New; extends S10.3 |
| [R7](#r7--two-orphaned-subsystems) | Two orphaned subsystems | Low | New |
| [R8](#r8--backward-pagination-is-guesswork) | Backward pagination is guesswork | Medium | Resolved by S16 |
| [R9](#r9--query_points-hit-region-is-offset-from-the-glyphs) | `query_point`'s hit region is offset from the glyphs | Medium | New, open |
---
## Verdict
**Well architected.** The concerns that usually decide this question are all on
the right side of the line:
- **The abstract/concrete split is real, not aspirational.** Verified
empirically: `abstract/` never imports `concrete/`; `core/` imports nothing but
itself. The single crossing ([document.py:7](../pyWebLayout/abstract/document.py#L7))
is into `style/`, which the dependency rules permit. Most codebases claiming
this layering have leaked it within a year.
- **The layouter contract is the right abstraction.** `paragraph_layouter`
returning `(fit, failed_word_index, remaining_pretext)` is what makes
pagination resumable, and the shape is consistent across content types. Layout
engines that return `List[Line]` cannot paginate without a second pass.
- **[core/cache.py](../pyWebLayout/core/cache.py) is exemplary.** Usage-ranked
eviction with periodic aging, sampled eviction instead of a maintained heap,
O(1) hit path with no reordering — every choice justified by a measurement in
the docstring.
- **[concrete/text.py](../pyWebLayout/concrete/text.py) is the strongest file.**
The glyph cache ([:581-647](../pyWebLayout/concrete/text.py#L581-L647))
reimplements PIL's internals to skip per-call setup, with a permanent graceful
fallback when the private API is absent. Alignment is a clean strategy pattern,
and `render_alignment_handler` handles last-line-of-paragraph correctly.
- **[html_extraction.py](../pyWebLayout/io/readers/html_extraction.py) is
textbook.** An immutable `StyleContext` threaded down the tree plus a handler
dispatch table — no giant if/elif, no mutable parser state.
**The complexity that is not earned** is concentrated in three places, all in the
same band of the code:
1. `layout/page_buffer.py` — 520 lines of multiprocess prefetch that has never
worked (S12, plus R1/R2 below).
2. `BidirectionalLayouter.render_page_backward` — ~100 lines of convergence
heuristics standing in for an anchor list that already exists (R8).
3. The second block dispatcher in `ereader_layout.py`, which silently drops
tables and lists (S4, S8).
The pattern is visible in the git history: work from `e000068` onward (caching,
alignment, page geometry) is markedly better than the ereader scaffolding it sits
on. This is not a mess. It is a solid library with an early prototype still
embedded in it.
**Sizing the cleanup:** R1–R7 plus S12 and S10.3 remove roughly 800–900 lines and
fix four user-visible defects. None of it requires redesigning anything.
## Test baseline
At `c5c61a3`, in a clean venv on Python 3.14.6:
```
833 passed, 2 skipped, 24 subtests passed in 11.51s
```
(The 2 skips were environmental — the review venv lacked `requests`, so the URL
image tests skipped. With the `test` extra from R4 installed the suite reports
`853 passed, 24 subtests passed in 13.53s`.)
The suite then **hangs indefinitely** rather than exiting. See R2.
## Status of the existing remediation spec
| Spec | Subject | State |
|------|---------|-------|
| S1 | Inline content in non-paragraph containers | Done (`284d521`) |
| S2 | Page geometry: origin and content rect | Done (`f18cec2`) |
| S3 | Draw/canvas lifecycle | Done (`202dacf`) |
| S11 | Partial-block progress discarded | Done (`a57da80`) |
| S13 | Word spacing and alignment | Done (`1262be6`) |
| S14 | Vertical centring in buttons and fields | Done (`c5c61a3`) |
| S4–S10, S12 | Dispatch, cells, table grid, pagination, hygiene, background rendering | Outstanding |
The spec's analysis is sound and in places sharper than this review — S12 caught
that `_render_page_worker` omits `page_size` entirely, which this review missed.
Nothing below supersedes it.
---
## R1 — The process pool crashes on Python 3.14
**Severity: critical. Raises S12 from "useless" to "fatal".**
### Problem
`PageBuffer` submits to a `ProcessPoolExecutor` from inside
`BufferedPageRenderer.render_page`
([page_buffer.py:431](../pyWebLayout/layout/page_buffer.py#L431)). Python 3.14
changed the default multiprocessing start method on Linux from `fork` to
`forkserver`. Under a non-`fork` start method, `submit()` reaches
`_check_not_importing_main()`, which raises unless the caller sits inside an
`if __name__ == "__main__":` guard — and the child re-imports the caller's main
module, re-executing it.
S12 documents this subsystem as delivering no benefit. On 3.14 it is worse than
that: `EreaderLayoutManager.get_current_page()` **raises** when called from
module-level script code.
### Evidence
A plain script calling `manager.get_current_page()` at module level, Python
3.14.6:
```
RuntimeError:
An attempt has been made to start a new process before the
current process has finished its bootstrapping phase.
...
File "pyWebLayout/layout/page_buffer.py", line 221, in _queue_forward_renders
future = self.executor.submit(_render_page_worker, args)
ConnectionResetError: [Errno 104] Connection reset by peer
```
With a `__main__` guard added it does not raise, and instead confirms S12's
finding on every job:
```
Background render failed for position RenderingPosition(...): cannot pickle 'ImagingCore' object
Background render failed for position RenderingPosition(...): cannot pickle 'ImagingCore' object
```
### Action
Fold into **S12**, and treat S12 as unblocked and urgent rather than phase 4. The
recommended resolution there — delete the pool, keep the LRU buffers and position
maps, replace prefetch with synchronous readahead — resolves R1 and R2 as a side
effect. S12's measurement gate still applies to the *readahead* decision; it does
not need to gate deletion of the pool, because the pool's contribution is
provably zero.
### Files
`pyWebLayout/layout/page_buffer.py`
---
## R2 — The test suite hangs at interpreter exit
**Severity: high.** Same root cause as R1.
### Problem
`PageBuffer.__del__` calls `shutdown()`, which calls
`executor.shutdown(wait=True)` ([page_buffer.py:342](../pyWebLayout/layout/page_buffer.py#L342)).
`EreaderLayoutManager.__del__` does the same via `renderer.shutdown()`. Running
`__del__` at interpreter shutdown and blocking on a process pool inside it
deadlocks.
### Evidence
```
833 passed, 2 skipped, 24 subtests passed in 11.51s
```
...then the process sat at ~0% CPU with idle forkserver children for 13 minutes
before being killed. Reproduced twice; both runs completed the tests in under
12s and neither exited.
This is why CI wall-clock does not resemble the 11.5s the tests actually take.
### Action
Resolved by S12's deletion of the executor. If for any reason the pool is
retained, `__del__` must not block: register an `atexit` handler or require
explicit `shutdown()`, and never `wait=True` from a finaliser.
### Acceptance criteria
- `pytest` returns to the shell within a second of printing its summary line.
- No `multiprocessing` child processes outlive the test session.
### Files
`pyWebLayout/layout/page_buffer.py`, `pyWebLayout/layout/ereader_manager.py`
---
## R3 — Font scaling destroys hyperlinks
**Severity: high. User-visible, silent, and trivially reproducible.**
### Problem
`BidirectionalLayouter._scale_block_fonts`
([ereader_layout.py:474-498](../pyWebLayout/layout/ereader_layout.py#L474-L498))
rebuilds a scaled block by constructing plain `Word(word.text, scaled_style)` for
every word. `LinkedWord` is a `Word` subclass
([inline.py:288](../pyWebLayout/abstract/inline.py#L288)), so the reconstruction
downgrades it and the link target is discarded.
The function returns the block unchanged when `font_scale == 1.0` and no family
override is set, which is why no test has caught this: the defect only appears
once the reader changes font size.
Two further gaps in the same function:
1. It handles only `Paragraph` and `Heading`. Every other block type is returned
unscaled, so a font-size change leaves images, tables and lists at their
original size while the text around them reflows.
2. It allocates a new `Paragraph` and a new `Word` per word on **every page
render** at any scale ≠ 1.0 — directly against the caching work in
`concrete/text.py`, and on the hot path.
### Evidence
At `c5c61a3`, parsing `<p>Go to <a href="http://x">this link</a> now.</p>`:
```
scale=1.0: LinkedWords = 2
scale=1.5: LinkedWords = 0
```
### Design
Stop reconstructing abstract blocks at layout time. Font scale and family are
*rendering context*, not document content — carrying them in a copied document
violates the "abstract content is not mutated by layout" principle in
[ARCHITECTURE.md](../ARCHITECTURE.md) in spirit, even though it copies rather
than mutates.
Preferred: thread the scale/override into the layouter and resolve fonts at
`Text` construction, where `Font` objects are already deduplicated by
`FontRegistry`. `paragraph_layouter` already accepts an `alignment_override`;
`font_scale` and `font_family_override` belong in the same place.
Minimum viable fix if the larger change is deferred: reconstruct via
`type(word)` and copy subclass state, and extend coverage to every block type.
This is strictly a stopgap — it keeps the per-page allocation cost.
### Acceptance criteria
- A document containing `<a href>` retains every `LinkedWord` after
`set_font_scale(1.5)`, and `query_point` over the rendered page still returns
`object_type="link"` with the correct target.
- An image block's rendered size is unaffected by `set_font_scale`, or scales
deliberately — not left inconsistent with the text around it.
- No new `Word`/`Paragraph` objects are allocated per page render at scale ≠ 1.0.
### Files
`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/document_layouter.py`
---
## R4 — Three packaging configs that disagree
**Severity: medium.** *Corrected: the original review claimed a clean install
fails on first import. It does not — see below.*
### Problem
The project carries **three** sets of packaging metadata:
| File | Declares |
|------|----------|
| `pyproject.toml` `[project]` | Pillow, numpy, pyphen, beautifulsoup4, flask, ebooklib, requests |
| `setup.cfg` `[options]` | Pillow, numpy |
| `setup.py` `setup(...)` kwargs | Pillow, numpy |
`pyproject.toml`'s `[project]` table wins under any modern build backend, so the
shipped wheel is correct and `pip install pyWebLayout` works. The `setup.cfg` and
`setup.py` copies are dead, contradictory, and actively misleading — reading
either one gives the wrong answer about what the library needs.
The authoritative list is itself wrong in the other direction:
- **`flask` is a runtime dependency.** It is imported only by
`tests/abstract/test_abstract_blocks.py`, as a fixture HTTP server. Every user
installs Flask, Jinja2, Werkzeug, click, itsdangerous and blinker for nothing.
- **`ebooklib` is a runtime dependency and is never imported by the library.**
`epub_reader.py` uses `zipfile` + `xml.etree` directly. Only the *tests* use
ebooklib, to build EPUB fixtures.
- **`requests` is declared required but is optional.** `concrete/image.py:100-111`
imports it lazily and degrades to an error message on the image when absent.
- **`requires-python = ">=3.6"` is false.** The package uses dataclasses (3.7+)
and `from __future__ import annotations` (3.7+); CI tests 3.10, 3.12 and 3.13.
Net effect: a runtime install pulls 7 direct dependencies where 4 are needed.
### Action
- Consolidate on `pyproject.toml`. Reduce `setup.cfg` to its `[flake8]` section
and `setup.py` to a `setup()` shim, each with a comment saying where metadata
lives.
- Runtime deps: Pillow, numpy, pyphen, beautifulsoup4. Move flask, werkzeug,
ebooklib and requests into a `test` extra; add a `remote-images` extra for
requests; add a `dev` extra composing them.
- Set `requires-python = ">=3.10"` to match the CI matrix, and add version
classifiers.
- Add a CI step that installs the package into an empty venv with **only**
declared runtime deps and imports every top-level subpackage. This class of
defect is only caught by installing what you ship — and it is what would have
caught the original misreading.
### Files
`pyproject.toml`, `setup.cfg`, `setup.py`, `.gitea/workflows/ci.yml`
---
## R5 — Monkey-patched `Page` methods with a conflicting signature
**Severity: medium.** Currently inert; a landmine if `Page` is ever refactored.
### Problem
[ereader_layout.py:741-761](../pyWebLayout/layout/ereader_layout.py#L741-L761)
defines `_add_page_methods()` and calls it at import time. It attaches
`can_fit_line` and `available_width` to the `Page` class if they are absent.
`Page` defines both ([page.py:59](../pyWebLayout/concrete/page.py#L59),
[page.py:147](../pyWebLayout/concrete/page.py#L147)), so the patch never fires.
But the two definitions of `can_fit_line` **do not agree**:
| Source | Signature |
|--------|-----------|
| `Page` | `can_fit_line(baseline_spacing, ascent=0, descent=0)` |
| monkey patch | `can_fit_line(line_height)` |
The patched version also ignores descenders entirely — the exact bug S2 fixed. If
`Page.can_fit_line` were ever renamed or moved, this would silently reinstate
pre-S2 clipping behaviour, from an import side effect in a different package.
### Action
Delete `_add_page_methods` and its call site. Import-time monkey-patching of a
class in another module has no place here; if `Page` is missing something the
layout engine needs, it belongs on `Page`.
### Files
`pyWebLayout/layout/ereader_layout.py`
---
## R6 — Dead duck-typing cluster in `page.py`
**Severity: low.** Extends S10.3.
### Problem
[page.py:261-491](../pyWebLayout/concrete/page.py#L261-L491) contains a closed
cluster with no external callers:
- `_get_child_property` (:261) — called only by the four below
- `_get_child_height` (:301) — called by nothing
- `_get_child_position` (:382) — called only by `_point_in_child`
- `_point_in_child` (:435) — called by nothing
- `_get_child_size` (:466) — called only by `_point_in_child`
Verified by grep across `pyWebLayout/`, `tests/`, `examples/` and `scripts/`:
zero references outside the cluster. About 90 lines.
It exists because `Renderable` declares neither `size` nor `origin`, so the code
probes `_size`, `size`, `_height`, `height`, `_origin` and `position` in turn with
`hasattr`. `query_point` (:399) already does the right thing instead — it relies
on the `Queriable` interface.
### Action
- Delete all five methods.
- Add `origin` and `size` to the `Renderable`/`Geometric` contract in
`core/base.py` so the duck-typing cannot grow back. This is the same concern as
S10.1's render contract and can ship with it.
### Files
`pyWebLayout/concrete/page.py`, `pyWebLayout/core/base.py`
---
## R7 — Two orphaned subsystems
**Severity: low**, but they are a large share of the "is this over-complex?"
impression: 559 lines that nothing in the library reaches.
### Problem
**`concrete/interaction_handler.py` (310 lines).** `InteractionHandler` and
`InteractionStateManager` are referenced only by
`examples/07_pressed_state_demo.py`. No library code, no ereader path, no tests.
**`core/highlight.py` (249 lines).** `Highlight`, `HighlightColor` and
`HighlightManager` have tests (`tests/core/test_highlight.py`) but are not wired
into `EreaderLayoutManager` at all. Highlighting is not reachable through the
library's own top-level interface.
`HighlightManager` also duplicates `BookmarkManager`'s JSON persistence
(directory, `_save`, `_load`, per-document file naming) with no shared base.
### Action
Decide per subsystem, and record the decision:
- **Wire it up** — `EreaderLayoutManager` grows `add_highlight` / `highlights_for_page`
and the persistence merges with `BookmarkManager` into one document-state store.
- **Or move it out** — relocate to `examples/` or delete, and drop the tests with it.
Either is fine. Leaving a tested, documented, unreachable subsystem in `core/` is
what makes the library look larger and less coherent than it is.
### Files
`pyWebLayout/concrete/interaction_handler.py`, `pyWebLayout/core/highlight.py`,
`pyWebLayout/layout/ereader_manager.py`
---
## R8 — Backward pagination is guesswork
**Severity: medium.** S11's closing note already flags this for audit; this
records what the audit found.
### Problem
`render_page_backward`
([ereader_layout.py:372-472](../pyWebLayout/layout/ereader_layout.py#L372-L472))
finds the previous page by estimating a start position, rendering forward,
comparing the end against the target, and adjusting — **up to 10 times**. It then
has a fallback that jumps back up to 5 blocks and renders again, and a fallback
for *that* which renders from the start of the document.
Worst case: one "previous page" tap costs up to 12 full page layouts.
The estimator it converges from is `max(1, int(10 / font_scale))` blocks
([:684](../pyWebLayout/layout/ereader_layout.py#L684)) — a constant with no
relationship to page size, block length or font metrics.
The correct answer is usually already known.
`EreaderLayoutManager._page_history` ([ereader_manager.py:210](../pyWebLayout/layout/ereader_manager.py#L210))
records real page-start positions and serves them instantly; the refinement loop
only runs when history misses — after a jump, a bookmark, a font change, or
beyond 50 entries.
S11's note asks whether these fallbacks were compensating for the
discarded-progress bug it fixed. They were, in part: the "failed to move
backward" branch at [:446](../pyWebLayout/layout/ereader_layout.py#L446) is
reachable precisely when forward rendering fails to advance, which S11 addressed.
### Design
Replace convergence with anchors. Maintain a sorted list of known page-start
positions — chapter starts from `ChapterNavigator` (free, already built) plus
every position visited. To go back from position P: binary-search the largest
anchor A < P, render forward from A collecting page starts until reaching P, and
return the last one. Cost is bounded by the anchor spacing, and every page start
discovered on the way is itself a new anchor, so the second traversal of any
region is free.
This subsumes `_page_history`, so the two mechanisms become one.
**Sequencing:** do this after S12, and after S8 — table and list pagination
changes what a page start can be, and re-deriving anchors is cheap only once
positions round-trip through tables correctly (S8 already notes this dependency).
### Acceptance criteria
- `previous_page()` from any position issues at most *k* page layouts, where *k*
is the anchor spacing, with no iteration count and no fallback ladder.
- Forward-then-backward round-trips exactly, from a cold cache, after a chapter
jump, and after a bookmark restore.
- `_estimate_page_start`, `_adjust_start_estimate` and the three-tier fallback
are deleted, not retained alongside.
### Files
`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/ereader_manager.py`
---
## Status
All findings in this document are resolved. What remains is the existing
remediation spec: **S4 → S5 → S6 → S7 → S8 → S9**, plus **S10.1**, unchanged.
| ID | Resolution | Commit |
|----|-----------|--------|
| R1 | Fixed with S12 — the pool that raised is gone | `1924cc2` |
| R2 | Fixed with S12 — no executor, no blocking finaliser | `1924cc2` |
| R3 | `Word.with_style` keeps subclasses; all container blocks scale | `f0dc675` |
| R4 | Consolidated on `pyproject.toml`; 7 runtime deps → 4 | `767e4c1` |
| R5 | Monkey patch deleted | `62ca151` |
| R6 | 138 dead lines deleted; contract hardening deferred to S10.1 | `e81ba48` |
| R7 | Both subsystems wired into `EreaderLayoutManager` | `8746d3f`, `0ce1aea` |
| R8 | Superseded by S16 (anchor replay); dead estimators removed | `bcae45a` |
| R9 | Open — see below | — |
Two things worth carrying forward:
- **S12's measurement stands as the argument against prefetch.** A page render
is 9–56 ms. Any future proposal to render ahead should have to beat that
number first.
- **Wiring an orphan found a bug.** R7's interaction handler had a crash on
every hovered or pressed link (`0ce1aea`). Unreachable code is not
neutral — it is untested code that looks tested.
---
## R9 — query_point's hit region is offset from the glyphs
**Severity: medium.** Found while verifying R3; not part of the original review.
### Problem
The region `Page.query_point` reports for a text object does not line up with
where that object says it is. Probing a `LinkText` at the centre of its own
`origin`/`size` box returns `object_type="empty"`.
### Evidence
A single-link page at 400×600, default scale:
```
'this' origin=(68.3, 35.0) size=(29.2, 19.0) centre=(82, 44) -> empty
'link' origin=(102.5, 35.0) size=(28.3, 19.0) centre=(116, 44) -> empty
grid scan: link is detected across y≈20–39
LinkText claims: y≈35–54
```
The two bands overlap by about four pixels. The offset is close to the font
ascent, which points at a baseline-versus-top mismatch between the coordinates
`Text` stores and the ones `in_object` tests.
This reproduces identically at scale 1.0 and 1.5, so it predates the R3 fix.
### Why it matters
Taps land through the grid because the region is only shifted, not absent — but
it is shifted by most of a line height. Near the top or bottom of a page, or
between tightly spaced lines, a tap can hit the neighbouring line instead of the
one under the finger. It also makes `LinkText.origin`/`size` unusable for
drawing selection or highlight overlays, which is what R7's highlighting now
depends on.
### Action
Establish which of the two is authoritative — almost certainly the drawn
position — and make the other agree. This sits close to S2 (page geometry) and
S3 (draw/canvas lifecycle), both already landed, so the conventions to match
are in place.
### Acceptance criteria
- `page.query_point(centre_of(obj))` returns `obj` for every text object on a
rendered page, at scales 0.8, 1.0, 1.5 and 2.0.
- The end-to-end test in `tests/layout/test_font_scaling.py` probes the centre
directly instead of scanning a grid.
### Files
`pyWebLayout/concrete/text.py`, `pyWebLayout/concrete/page.py`,
`pyWebLayout/core/base.py`
## Reproducing the findings
Reviewed at `c5c61a3` on Python 3.14.6, in a venv containing
`pytest pyphen Pillow numpy beautifulsoup4 lxml ebooklib`.
- **R1**: call `EreaderLayoutManager(...).get_current_page()` from module-level
script code (no `__main__` guard).
- **R2**: `python -m pytest -q`; observe the summary line, then the hang.
- **R3**: parse HTML containing `<a href>`, call
`BidirectionalLayouter._scale_block_fonts(block, 1.5)`, count `LinkedWord`
instances in the result.
- **R5**: compare `inspect.signature(Page.can_fit_line)` against the patch body.
- **R6**: grep the five method names across `pyWebLayout/ tests/ examples/ scripts/`.
- **R8**: read the loop; no execution needed.