pyWebLayout/docs/ARCHITECTURE_REVIEW.md

# 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.