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

docs: add architecture review with findings R1-R8

Browse Source 
Independent review of the codebase at c5c61a3. Records the architectural
verdict and eight findings not covered by LAYOUT_REMEDIATION_SPEC.md,
cross-referencing the existing spec where they overlap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-08 12:21:02 +02:00
co-authored by Claude Opus 5
parent 4d596ce095
commit 7384a32cdd
1 changed files with 507 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/ARCHITECTURE_REVIEW.md
+507
View File
@@ -0,0 +1,507 @@
# 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--declared-dependencies-are-incomplete) | Declared dependencies are incomplete | High | New |
| [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 | Partially noted in S11 |
---
## 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
```
(Two further failures in `test_concrete_image.py` are environmental — the review
venv lacked `requests`. See R4, which is the same underlying problem.)
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 — Declared dependencies are incomplete
**Severity: high. A clean `pip install pyWebLayout` fails on first import.**
### Problem
[setup.cfg](../setup.cfg) declares:
```
install_requires =
Pillow
numpy
```
The library also imports `pyphen` ([abstract/inline.py](../pyWebLayout/abstract/inline.py)),
`bs4` ([io/readers/html_extraction.py](../pyWebLayout/io/readers/html_extraction.py))
and `ebooklib` ([io/readers/epub_reader.py](../pyWebLayout/io/readers/epub_reader.py)).
`import pyWebLayout.concrete` fails without `pyphen`.
There is no declared test extra either; `tests/concrete/test_concrete_image.py`
needs `requests`, which nothing declares.
### Action
- Add `pyphen`, `beautifulsoup4` and `ebooklib` to `install_requires`, with lower
bounds.
- Add an `[options.extras_require] test =` entry covering `pytest` and `requests`.
- Add a CI job that installs the built wheel into an empty environment and runs
`python -c "import pyWebLayout.concrete, pyWebLayout.io.readers.epub_reader"`.
This class of defect is only ever caught by installing what you ship.
### Files
`setup.cfg`, `setup.py`, CI configuration
---
## 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`
---
## Recommended order
```
R4 ── packaging; independent, minutes, unblocks clean CI
S12 ── delete the process pool; resolves R1 and R2 with it
R3 ── font scaling loses links; independent, user-visible
R5 ── delete the monkey patch; minutes
R6 ── delete the dead cluster (with S10.1's render contract)
R7 ── decide the two orphans; no code risk either way
S4 → S5 → S6 → S7 → S8 → S9 (existing spec, unchanged)
R8 ── after S8
```
R4, R5 and R6 are an afternoon and carry no design risk. S12 is the largest
single removal and fixes two defects at once. R3 is the one users would notice
today. Everything after that is the existing spec, which needs no revision.
## 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.