diff --git a/docs/ARCHITECTURE_REVIEW.md b/docs/ARCHITECTURE_REVIEW.md index a20fc99..12114ec 100644 --- a/docs/ARCHITECTURE_REVIEW.md +++ b/docs/ARCHITECTURE_REVIEW.md @@ -22,7 +22,8 @@ finding is already specced, it is cross-referenced rather than restated. | [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 | +| [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 | --- @@ -494,22 +495,89 @@ positions round-trip through tables correctly (S8 already notes this dependency) --- -## Recommended order +## 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: ``` -R4 ── packaging; independent, minutes, unblocks clean CI [done] -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 +'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 ``` -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. +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