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. Where a finding is already specced, it is cross-referenced rather than restated.

Contents

ID	Finding	Severity	Status
R1	The process pool crashes on Python 3.14	Critical	New; raises priority of S12
R2	Test suite hangs at interpreter exit	High	New; same root cause as R1
R3	Font scaling destroys hyperlinks	High	New
R4	Three packaging configs that disagree	Medium	New (corrected)
R5	Monkey-patched Page methods with a conflicting signature	Medium	New
R6	Dead duck-typing cluster in page.py	Low	New; extends S10.3
R7	Two orphaned subsystems	Low	New
R8	Backward pagination is guesswork	Medium	Resolved by S16
R9	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) 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 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 is the strongest file. The glyph cache (:581-647) 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 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). 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). 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) rebuilds a scaled block by constructing plain Word(word.text, scaled_style) for every word. LinkedWord is a Word subclass (inline.py:288), 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 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 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, page.py:147), 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 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) 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) — 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) 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 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.