Skip to content

refactor(font): Japanese UI P5 PR C — ResolvedGlyph resolver + P4 deletion + CJK UI fallback - #3

Merged
mtskf merged 8 commits into
masterfrom
refactor/cjk-resolveglyph-migration
Jun 17, 2026
Merged

refactor(font): Japanese UI P5 PR C — ResolvedGlyph resolver + P4 deletion + CJK UI fallback#3
mtskf merged 8 commits into
masterfrom
refactor/cjk-resolveglyph-migration

Conversation

@mtskf

@mtskf mtskf commented Jun 17, 2026

Copy link
Copy Markdown
Owner

Summary

PR C (final) of the Japanese UI Support P5 re-architecture. Replaces the old P4 CJK "synthesizer / ring / sentinel" machinery with CJK-as-an-ordinary-EpdFont via a source-aligned ResolvedGlyph resolver chain, so Japanese (CJK) text renders correctly across the firmware UI (titles, menus, filenames, status bar, chapter list, language picker) with zero CJK-specific branches left in the blitter. The reader body-text path is untouched.

Prerequisite PRs A + B (plan Tasks 1–3) are already merged on master; this PR is plan Tasks 4–8 plus a comment-hygiene cleanup.

Changes

  • feat(font) ResolvedGlyph + EpdFontFamily::resolveGlyph — a {const EpdGlyph* glyph; const EpdFontData* data;} pair that is always source-aligned (glyph and data always from the same EpdFontData), eliminating the P4 owner-mismatch bug structurally. Adds setFallback() + a no-fallback fast path that never double-probes SD/ring-buffer fonts.
  • refactor(font) deletes ALL P4 machinery: CjkUiFallback.h, cjk_ui_font_20.h (209k-line obsolete fork blob), the cjkUiFallback_ flag/enableCjkUiFallback(), EpdFontFamily::hasCjkUiFallback(), and the GfxRenderer isSynthesized/custom-CJK-blitter/getGlyphBitmap guard branches + P4 tests. Migrates every getGlyph/getData pair in GfxRenderer to a single resolveGlyph call; refactors EpdFont::getTextBounds into a family-aware getTextBoundsImpl (minimal seam, all measurement semantics preserved) with a cross-font kern guard.
  • feat(font) family vertical-metric floor via max(primary, fallback) (getMaxAscender/getMaxAdvanceY) — no magic constants, never names "CJK"; Latin-only fonts (no fallback) are structurally unchanged.
  • feat(ui) registers the converted 20px CJK font (cjk_ui_20_font_data, flash/PROGMEM) as the fallback on the three UI families (smallFontFamily, ui10FontFamily, ui12FontFamily) in src/main.cpp; reader/body families are deliberately NOT registered.
  • test(font) new host gtests: font_resolver (resolver + metric floor + measurement consistency) and font_boundary (kern/ligature no-match for CJK, advance-additivity across the Latin↔CJK boundary, converter null-table guard).
  • chore(font) comment hygiene: corrected stale metric-helper docs; dropped a now-dead pgmspace test include.

Flash size (must be confirmed by CI — this machine has no PlatformIO)

  • Target: +210 KB ± 20 KB vs master (the CJK font blob is flash-resident PROGMEM; zero DRAM overhead). cjk_ui_20.cpp is compiled exactly once (single library TU).
  • Canary: ≈ +420 KB would signal ODR duplication (a second copy of the font arrays being linked) — investigate build_src_filter / double-compilation if seen.
  • ≈ 0 would mean the font was not linked.
  • Please record the exact Flash: figures (master clean build vs this branch) from the CI build in this PR before merge.

Test Plan

  • Host gtest (cmake) — 104/104 green on a clean rebuild, including CjkFontParity (3420 codepoints bit-perfect), CjkFontSelf, FontResolver (+metric/measurement), FontBoundary, and the pre-existing differential-rounding suites (EpdFont + Fp4Math, no regression).
  • Zero P4 residue (CjkUiFallback/isSynthesized/hasCjkUiFallback/cjk_ui_font_20 etc. — grep clean across lib/ src/ test/).
  • CI: firmware build (pio run) + flash delta within target (see above).
  • Device (human tester): Japanese book titles render in home/recent/file-browser/chapter-list/status-bar + the 日本語 language-picker entry; pure-English UI letterforms visually unchanged (no blocky 20px Latin); taller UI rows + correct baseline alignment for mixed Latin+CJK in all 4 orientations; dark-mode polarity matches Latin; ESP.getFreeHeap() unchanged (PROGMEM, no new DRAM).

Deferred follow-ups (final whole-branch review — none blocking; all currently inert)

  • Cross-font kern-guard symmetry: getTextBoundsImpl guards cross-font kern with prevData == glyphData; the renderer draw/measure paths rely on the primary's table returning 0 for CJK (true today — CJK font has null kern tables and the primary excludes CJK). Add a symmetric guard/comment only if a future fallback ever carries its own kern table.
  • setFallback precondition asserts the fallback's glyphMissHandler == nullptr but not the primary's (UI primaries are all static built-ins today).
  • Minor cosmetic test-strength / dead-clause notes (resolveGlyph REPLACEMENT_GLYPH symmetry clause, a rotatedAscender alias).

mtskf added 7 commits June 18, 2026 08:09
…op dead pgmspace include

Final-review follow-ups (M6-1, M5-1): the three GfxRenderer metric-function
comments still described getMaxAscender/getMaxAdvanceY as Task-4 stubs, but
Task 6 shipped the max(primary, fallback) bodies — corrected to describe the
shipped behaviour. Also dropped the now-dead test/support (pgmspace.h) include
from the font_resolver test target: after the P4 deletion, none of its TUs
reference PROGMEM. Comment/build-hygiene only; no behaviour change. 104/104 host
tests green.
@mtskf

mtskf commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

Review-cycle (cycle 1, Codex + 5 Claude reviewers + Validator) — outcome

One real regression was found and fixed in 6d143e7:

  • HIGH — GfxRenderer.cpp rotated (Rotated90CW) draw baked the per-glyph resolved-font ascender into the baseline origin, so on a rotated mixed Latin+CJK line, glyphs whose own font ascender < the line ascender were misaligned (CJK vs Latin). The non-rotated path was unaffected (baseline-relative cursorY - top). Host tests can't catch it (GfxRenderer.cpp is not host-compiled). Fix: the rotated path now uses fontFamily.getMaxAscender(style) (the same family ascender the row height reserves) as a shared baseline origin; reader/body fonts have no fallback so getMaxAscender == primary → their rendering is unchanged.

Also folded into 6d143e7: two comment fixes (dangling "the brief" / "Option A (lookupGlyph)" references) and non-vacuity strengthening of the two FontBoundaryTest additivity tests (now pin that U+65E5 reaches the CJK fallback, and test genuine boundary-split additivity instead of an X==X self-sum). Host suite: 104/104 green.

Deferred follow-ups (validated VALID but non-blocking — recorded here since the TODO file is out of this worktree)

  • MEDIUM — Draw/advance kern paths (GfxRenderer::drawText / getTextAdvanceX / drawTextRotated90CW) do not apply the prevData == glyphData cross-font kern guard that EpdFont::getTextBoundsImpl does. Zero current defect (the primary kern table has no CJK entries, so cross-font kern is incidentally 0); purely a latent structural divergence between the draw/measure and bounds paths. Add a symmetric guard only if/when a fallback font ever ships its own kern table.
  • LOW — EpdFontFamily::setFallback asserts only the fallback's glyphMissHandler == nullptr, not the primaries', though the docstring precondition covers both. Inert: the pointer-identity probe runs only when a fallback is registered, and all three fallback-bearing UI families use static built-in primaries (no glyphMissHandler); setFallback isn't even passed the primaries. Tighten the doc or add a primary-side assert at construction if desired.
  • LOW — EpdFontFamily::getTextBounds dereferences the resolved glyph without a null guard; unreachable today because its only caller (GfxRenderer::getTextWidth) pre-guards. Add a defensive null-check if the call graph widens.

CI runs bin/clang-format-fix with clang-format-21; this applies its exact
output across the PR C files (2-space continuation indent, include grouping,
array/comment alignment). Whitespace-only; host suite 104/104 green.
@mtskf

mtskf commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

Flash size — confirmed from CI (the plan's +210 KB target was a mis-estimate)

CI build (env default) reports:

  • master ec13e69: Flash 5,427,685 bytes (82.8%), RAM 101,452 (31.0%)
  • this branch ed771b1: Flash 5,464,939 bytes (83.4%), RAM 101,228 (30.9%)
  • Net Flash delta = +37,254 bytes (+36.4 KB); RAM −224 bytes (CJK data is flash/PROGMEM, no DRAM cost).

This is +37 KB, not the plan's +210 KB target — and that is correct. The +210 KB figure was the absolute size of the new P5 font, but the plan's estimate did not subtract the P4 font that this PR deletes (cjk_ui_font_20.h), which was already linked into master's flash (master's main.cpp called enableCjkUiFallback() and GfxRenderer called getCjkUiGlyph, so the ~204 KB of P4 glyph data was live).

First-principles arithmetic:

  • P5 font linked: bitmap 169,240 + glyphs 3420×16 + intervals 2405×12 ≈ +247 KB
  • P4 font removed: CJK_UI_GLYPHS 3420×20×3 + width/codepoint tables ≈ −204 KB
  • net code churn (delete P4 synth/dispatch + CjkUiFallback − add resolveGlyph/getTextBoundsImpl/metric helpers) ≈ −6 KB
  • Total ≈ +37 KB — matches the observed +37,254 bytes exactly.

The positive delta is itself the proof the P5 font is linked: if it had been GC'd/unlinked, deleting the 204 KB P4 font would have shown a large negative delta (~−200 KB). It is neither the +420 KB ODR-duplication canary nor the ~0 link-skip case. Single copy, correctly linked.

@mtskf
mtskf merged commit e85f575 into master Jun 17, 2026
6 checks passed
@mtskf
mtskf deleted the refactor/cjk-resolveglyph-migration branch June 17, 2026 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant