Font-pack inspector + fontconvert-parity glyph builder (extend round-trip) - #92
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds host-side services for Noto downloads, PlyF decoding/encoding, glyph rendering, and pack splicing. Adds Qt dialogs for inspecting shipped packs and building edited glyph packs, plus tray access, manifests, resources, tests, and documentation. ChangesFont-pack inspect and extend system
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
tests/gui/fontpack_extend_dialog_test.py-54-55 (1)
54-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit the multi-statement test setup lines.
Ruff is already flagging Lines 54-55 with E702, so this test file will fail lint as written.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gui/fontpack_extend_dialog_test.py` around lines 54 - 55, The test setup in the dialog test uses multiple statements on single lines, which triggers Ruff E702. Split the chained setup in the fontpack extension dialog test into separate statements for the _first, _last, _size, _bundle, and _default_index calls so each action is on its own line while keeping the same test flow.Source: Linters/SAST tools
polyhost/gui/fontpack_extend_dialog.py-47-75 (1)
47-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit the semicolon-chained setup lines.
Ruff is already flagging these lines with E702, so this module will fail the configured lint gate as written.
Also applies to: 86-102, 112-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/gui/fontpack_extend_dialog.py` around lines 47 - 75, The dialog setup in FontpackExtendDialog is using semicolon-chained statements, which triggers Ruff E702 and will fail linting. Split the chained initialization lines in the constructor into one statement per line, especially around the _mode, _seq, _seq_first, _size, _gray, _dither, _norm, _inv, _edge, _outline, _rsize, _yadv, and _maxw setup blocks, and apply the same cleanup anywhere else Ruff reported the issue.Source: Linters/SAST tools
polyhost/services/fontpack_render.py-30-36 (1)
30-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard codepoints before indexing glyph arrays.
For
cp < font.first,cp - font.firstbecomes negative and can silently render the wrong glyph instead of failing.Suggested fix
+def _glyph_for(font, cp: int): + if cp < font.first or cp > font.last: + raise ValueError(f"codepoint U+{cp:04X} is outside font range") + return font.glyphs[cp - font.first] + + def glyph_to_image(font, cp: int, scale: int = 1, fg: int = 255, bg: int = 0): @@ - g = font.glyphs[cp - font.first] + g = _glyph_for(font, cp) @@ def keycap_image(font, cp: int, base_yadv: int = BASE_YADV, scale: int = 1, @@ - g = font.glyphs[cp - font.first] + g = _glyph_for(font, cp)Also applies to: 61-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/services/fontpack_render.py` around lines 30 - 36, In glyph_to_image, add a bounds guard before indexing font.glyphs so codepoints below font.first (and any other out-of-range cp values) do not use a negative index and accidentally render the wrong glyph; validate cp against the font’s supported range using the existing glyph_to_image and font.first/font.glyphs symbols, and return the intended fallback/error behavior before touching font.glyphs[cp - font.first]. Also apply the same protection in the related glyph lookup path referenced by the later block so all codepoint-to-glyph indexing is range-safe.tests/services/fontgen_test.py-43-47 (1)
43-47: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid implicit
/tmpfont discovery in tests.Auto-loading
/tmp/NotoColorEmoji.ttfmakes the parity tests depend on an environment-controlled file and triggers S108. Keep this opt-in viaNOTO_CEMOJIor use the checked repository fixture path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_test.py` around lines 43 - 47, The _find_cemoji helper in the fontgen tests is still implicitly probing /tmp/NotoColorEmoji.ttf, which makes the test depend on an uncontrolled environment file and trips the S108 check. Remove the /tmp fallback from the candidate list and keep font discovery limited to the NOTO_CEMOJI environment override and the checked-in repository fixture path referenced by _find_cemoji.Source: Linters/SAST tools
tests/services/fontgen_dither_test.py-22-26 (1)
22-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit semicolon-chained test statements.
Ruff reports E702 on these lines. Keep one statement per line so the test file passes lint.
Also applies to: 44-53, 62-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_dither_test.py` around lines 22 - 26, The test cases in the dither buffer checks are using semicolon-chained statements that trigger Ruff E702; split each chained assertion or method call into its own line in the affected test blocks. Update the relevant test methods in fontgen_dither_test.py, including the sections around the existing buffer setup and assertions, so each statement is isolated while preserving the same test behavior.Source: Linters/SAST tools
tests/services/fontgen_color_test.py-3-3 (1)
3-3: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid auto-loading a font fixture from
/tmp.The tests will parse whichever file happens to exist at
/tmp/NotoColorEmoji.ttf, making results environment-controlled and triggering S108. Prefer the explicitNOTO_CEMOJIopt-in or a repository fixture.Also applies to: 20-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_color_test.py` at line 3, The fontgen color tests are auto-loading /tmp/NotoColorEmoji.ttf as a fallback, which makes the suite environment-dependent and triggers the flagged issue. Update the test setup in the font fixture logic to remove the /tmp lookup and rely only on the explicit NOTO_CEMOJI opt-in or a checked-in repository fixture, keeping the gating logic in the fontTools/Pillow/numpy-dependent test path unchanged.Source: Linters/SAST tools
tests/services/fontgen_color_test.py-10-17 (1)
10-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
freetypein the dependency gate.These tests import
freetypein every glyph lookup path, but the skip condition only checks numpy/Pillow/fontTools. Addfreetypeto the gate so optional-dependency environments skip cleanly.🧪 Proposed fix
try: import numpy # noqa: F401 from PIL import Image # noqa: F401 import fontTools # noqa: F401 - from polyhost.services import fontgen_color as fc + import freetype # noqa: F401 _ERR = None -except Exception as e: # pragma: no cover +except ImportError as e: # pragma: no cover _ERR = e +else: + from polyhost.services import fontgen_color as fcAlso applies to: 40-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_color_test.py` around lines 10 - 17, The optional-dependency gate in fontgen_color_test.py is missing freetype, so the tests can still run into import failures in environments without it. Update the import check used to set _ERR to also import freetype alongside numpy, PIL Image, fontTools, and polyhost.services.fontgen_color, so the glyph lookup tests skip cleanly when freetype is unavailable; keep the existing test skip behavior driven by _ERR and apply the same change across the affected test block.Source: Linters/SAST tools
tests/services/fontgen_test.py-57-72 (1)
57-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProbe
fontconvert, not the Python renderer, for color parity skips.
fontgen.render_range()can now succeed via the Pillow/fontTools CBDT path even when the Cfontconvertbinary’s FreeType lacks PNG support. ThenColorEmojiParityTestruns and fails insubprocess.run(). Make_CEMOJI_OKverify the C binary path directly.🧪 Proposed fix
- if _ERR is not None or _CEMOJI is None: + if _ERR is not None or _CEMOJI is None or _FONTCONVERT is None: return False try: - fontgen.render_range(_CEMOJI, 0x1F600, 0x1F600, - RenderOptions(size=20, render_mode=1, height=40, bits=32)) + subprocess.run( + [_FONTCONVERT, "-f", _CEMOJI, "-s20", "-g", "-r40", "-b32", + "0x1F600", "0x1F600"], + capture_output=True, + text=True, + check=True, + ) return True except Exception: return FalseAlso applies to: 223-226
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_test.py` around lines 57 - 72, The `_cemoji_renderable` probe is checking `fontgen.render_range()` in Python, but that can pass even when the `fontconvert` binary still lacks PNG-capable FreeType, so the skip flag is wrong. Update `_CEMOJI_OK` to verify the C binary path directly by invoking the same `fontconvert` execution path used by `ColorEmojiParityTest` and treat failures there as non-renderable. Keep the existing helper name `_cemoji_renderable`/`_CEMOJI_OK`, but change the probe so it matches the subprocess-based parity test rather than the Python renderer.polyhost/services/fontgen_dither.py-125-129 (1)
125-129: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit multi-statement lines so Ruff passes.
Ruff flags these compact assignments/one-line conditionals as E701/E702 errors. Split them before merge to avoid lint failures.
Also applies to: 190-196, 221-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/services/fontgen_dither.py` around lines 125 - 129, Ruff is failing on compact multi-statement lines in the dither/resampling code, so split the chained assignments in fontgen_dither.py into separate statements to satisfy E701/E702. Update the blocks around the bilinear interpolation logic in the relevant functions (including the code near the xs/ys and s00/s01/s10/s11 calculations, plus the other noted multi-line one-liners in the same module) so each assignment or conditional is on its own line and remains behaviorally identical.Source: Linters/SAST tools
polyhost/services/fontgen.py-260-263 (1)
260-263: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject invalid sequence tokens instead of dropping them.
A typo in a sequence string is currently ignored, which can render and flash a partial font without warning. Raise a
ValueErrorfor invalid hex tokens.🐛 Proposed fix
try: cps.append(int(tok, 16)) - except ValueError: - pass + except ValueError as exc: + raise ValueError(f"invalid hex codepoint {tok!r} in sequence") from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/services/fontgen.py` around lines 260 - 263, The token parsing in fontgen should not silently ignore invalid hex values; in the sequence parsing path around the cps append logic, replace the ValueError swallow with explicit rejection so bad input fails fast. Update the code in the sequence-processing section to raise a ValueError when int(tok, 16) fails, and ensure the calling flow for the font generation sequence handles this as an error instead of continuing with a partial cps list.tests/services/fontgen_dither_test.py-9-14 (1)
9-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t skip real module import failures.
This catches any exception raised while importing
fontgen_dither, so a regression in the module can be reported as “numpy unavailable.” Gate only the optional dependency import, then import the module outside that broad skip path.🧪 Proposed fix
try: import numpy as np - from polyhost.services import fontgen_dither as fd _ERR = None -except Exception as e: # pragma: no cover +except ImportError as e: # pragma: no cover _ERR = e +else: + from polyhost.services import fontgen_dither as fd🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_dither_test.py` around lines 9 - 14, The import guard in fontgen_dither_test.py is too broad because the same try/except wraps both the optional numpy import and the fontgen_dither module import, which can hide real module regressions as dependency failures. Update the setup around fd and _ERR so only the numpy import is treated as optional, then import polyhost.services.fontgen_dither outside that skip path and let genuine import errors surface normally.Source: Linters/SAST tools
polyhost/services/fontgen.py-113-130 (1)
113-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSurface variable-font weight errors
These broad
except Exceptionblocks silently fall back to the default weight, so-wcan preview/flash the wrong glyph weight. Narrow the catches to the freetype-py failure cases for the supported version, or raise an error instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/services/fontgen.py` around lines 113 - 130, The variable-font weight handling in the font generation flow is swallowing failures and silently falling back to the default weight, which can make the -w preview use the wrong glyph weight. In the fontgen.py logic around get_variation_info and set_var_design_coords, replace the broad Exception catches with version-appropriate freetype-py errors or propagate a clear failure so unsupported variable-font handling does not get masked. Keep the existing weight coordinate setup in place, but ensure the caller can detect when variation info or coordinate application fails instead of silently ignoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polyhost/gui/fontpack_extend_dialog.py`:
- Around line 80-83: The save/flash path is using the currently selected bundle
instead of the bundle captured during Preview, so the output can diverge from
what was built. Update `_spliced_bytes()` in `FontPackExtendDialog` to use the
stored `_built` bundle index (set during build/preview) rather than re-reading
`self._bundle.currentIndex()`, and make sure `splice_font()` is invoked with
that same captured target so the replacement stays consistent with the previewed
bundle.
- Around line 38-39: The dialog is currently passing through invalid packs from
load_shipped_packs(), including (label, Exception) placeholders and empty
results, so the workflow can advertise unusable save/flash targets. Update
FontpackExtendDialog to filter _packs immediately after loading, and also guard
the target-selection logic so only valid bundle entries are exposed in the UI
and used by _spliced_bytes() and related extend/save paths. Use the existing
FontpackExtendDialog and load_shipped_packs() flow to keep invalid bundles out
before enabling the workflow.
In `@polyhost/host.py`:
- Around line 346-349: The font-pack inspector action is being disabled by the
generic connection-state handling and never re-enabled in disconnected or
protocol-mismatch cases. Update the connection/menu state logic in
managed_connection_status() so self.fontpack_inspector_action stays enabled for
offline inspection, while other actions remain governed by the existing
device-state rules; use the existing QAction setup around
open_fontpack_inspector as the target to preserve.
In `@polyhost/services/fontgen_dither.py`:
- Around line 387-392: The grayscale render path in _emit_loaded_glyph() ignores
DitherOpts sizing constraints, so apply fit_dimensions() there just like the
other render modes. In the branch for o.render_mode == 1 with pixel_mode ==
FT_PIXEL_MODE_GRAY, compute out_w and out_h from fit_dimensions(width, rows,
o.max_width, o.height) before building the bitmap and emitting metrics, so
grayscale glyphs respect -r/-W and downstream scaling stays consistent.
In `@polyhost/services/fontgen.py`:
- Around line 211-250: render_range currently returns PackFont objects without
enforcing the same 16-bit codepoint limit that render_sequence() applies, so add
a post-validation step in render_range (before returning) to reject any emitted
first/last values above 0xFFFF when opts.bits is not 32. Use the existing
render_sequence() range check as the model, and ensure the validation happens
after opts.offset is applied so the PackFont handed to the writer/splice layer
never contains out-of-contract codepoints.
In `@polyhost/services/fontpack_extend.py`:
- Around line 37-38: Validate the codepoint_range before calling
fontgen.render_range() in fontpack_extend.py: after unpacking first and last,
reject cases where first is greater than last and surface a clear error back to
the UI instead of continuing. Update the extend-font flow around the first/last
handling and PackFont creation so reversed ranges do not produce an empty
PackFont or get spliced into the bundle.
In `@polyhost/services/fontpack_reader.py`:
- Around line 169-187: `encode_pack` in `fontpack_reader.py` should validate
each `PackFont` before computing `glyph_offs` and `bitmap_offs`, because the
table uses `first`/`last` while the payload is built from the actual glyph list
and bitmap slice. Add a shape check in the `fonts`/`f` loop to ensure the glyph
count matches the declared range and the bitmap has the expected content length
before serializing; reject or raise on any `PackFont` mismatch so offsets cannot
point past missing glyph or bitmap data.
- Around line 101-114: decode_pack currently trusts the header too much: it
should reject unsupported abi values and validate table_off before any table
parsing. Add explicit checks near the header unpack in decode_pack to ensure
only the supported ABI is accepted and that table_off points past the header and
within the pack bounds. Keep the existing PackDecodeError path and use the same
decode_pack flow so malformed or future-format packs fail fast instead of being
parsed as v1.
In `@polyhost/services/fontpack_render.py`:
- Around line 119-123: The glyph limit is being enforced too late in the render
path, so _iter_glyphs(pack) is fully materialized before max_glyphs takes
effect. Update the glyph collection logic in fontpack_render so the cap is
applied while iterating, not after list(_iter_glyphs(pack)), and keep the
truncation behavior consistent with the existing capped flag and downstream
rendering flow.
---
Minor comments:
In `@polyhost/gui/fontpack_extend_dialog.py`:
- Around line 47-75: The dialog setup in FontpackExtendDialog is using
semicolon-chained statements, which triggers Ruff E702 and will fail linting.
Split the chained initialization lines in the constructor into one statement per
line, especially around the _mode, _seq, _seq_first, _size, _gray, _dither,
_norm, _inv, _edge, _outline, _rsize, _yadv, and _maxw setup blocks, and apply
the same cleanup anywhere else Ruff reported the issue.
In `@polyhost/services/fontgen_dither.py`:
- Around line 125-129: Ruff is failing on compact multi-statement lines in the
dither/resampling code, so split the chained assignments in fontgen_dither.py
into separate statements to satisfy E701/E702. Update the blocks around the
bilinear interpolation logic in the relevant functions (including the code near
the xs/ys and s00/s01/s10/s11 calculations, plus the other noted multi-line
one-liners in the same module) so each assignment or conditional is on its own
line and remains behaviorally identical.
In `@polyhost/services/fontgen.py`:
- Around line 260-263: The token parsing in fontgen should not silently ignore
invalid hex values; in the sequence parsing path around the cps append logic,
replace the ValueError swallow with explicit rejection so bad input fails fast.
Update the code in the sequence-processing section to raise a ValueError when
int(tok, 16) fails, and ensure the calling flow for the font generation sequence
handles this as an error instead of continuing with a partial cps list.
- Around line 113-130: The variable-font weight handling in the font generation
flow is swallowing failures and silently falling back to the default weight,
which can make the -w preview use the wrong glyph weight. In the fontgen.py
logic around get_variation_info and set_var_design_coords, replace the broad
Exception catches with version-appropriate freetype-py errors or propagate a
clear failure so unsupported variable-font handling does not get masked. Keep
the existing weight coordinate setup in place, but ensure the caller can detect
when variation info or coordinate application fails instead of silently ignoring
it.
In `@polyhost/services/fontpack_render.py`:
- Around line 30-36: In glyph_to_image, add a bounds guard before indexing
font.glyphs so codepoints below font.first (and any other out-of-range cp
values) do not use a negative index and accidentally render the wrong glyph;
validate cp against the font’s supported range using the existing glyph_to_image
and font.first/font.glyphs symbols, and return the intended fallback/error
behavior before touching font.glyphs[cp - font.first]. Also apply the same
protection in the related glyph lookup path referenced by the later block so all
codepoint-to-glyph indexing is range-safe.
In `@tests/gui/fontpack_extend_dialog_test.py`:
- Around line 54-55: The test setup in the dialog test uses multiple statements
on single lines, which triggers Ruff E702. Split the chained setup in the
fontpack extension dialog test into separate statements for the _first, _last,
_size, _bundle, and _default_index calls so each action is on its own line while
keeping the same test flow.
In `@tests/services/fontgen_color_test.py`:
- Line 3: The fontgen color tests are auto-loading /tmp/NotoColorEmoji.ttf as a
fallback, which makes the suite environment-dependent and triggers the flagged
issue. Update the test setup in the font fixture logic to remove the /tmp lookup
and rely only on the explicit NOTO_CEMOJI opt-in or a checked-in repository
fixture, keeping the gating logic in the fontTools/Pillow/numpy-dependent test
path unchanged.
- Around line 10-17: The optional-dependency gate in fontgen_color_test.py is
missing freetype, so the tests can still run into import failures in
environments without it. Update the import check used to set _ERR to also import
freetype alongside numpy, PIL Image, fontTools, and
polyhost.services.fontgen_color, so the glyph lookup tests skip cleanly when
freetype is unavailable; keep the existing test skip behavior driven by _ERR and
apply the same change across the affected test block.
In `@tests/services/fontgen_dither_test.py`:
- Around line 22-26: The test cases in the dither buffer checks are using
semicolon-chained statements that trigger Ruff E702; split each chained
assertion or method call into its own line in the affected test blocks. Update
the relevant test methods in fontgen_dither_test.py, including the sections
around the existing buffer setup and assertions, so each statement is isolated
while preserving the same test behavior.
- Around line 9-14: The import guard in fontgen_dither_test.py is too broad
because the same try/except wraps both the optional numpy import and the
fontgen_dither module import, which can hide real module regressions as
dependency failures. Update the setup around fd and _ERR so only the numpy
import is treated as optional, then import polyhost.services.fontgen_dither
outside that skip path and let genuine import errors surface normally.
In `@tests/services/fontgen_test.py`:
- Around line 43-47: The _find_cemoji helper in the fontgen tests is still
implicitly probing /tmp/NotoColorEmoji.ttf, which makes the test depend on an
uncontrolled environment file and trips the S108 check. Remove the /tmp fallback
from the candidate list and keep font discovery limited to the NOTO_CEMOJI
environment override and the checked-in repository fixture path referenced by
_find_cemoji.
- Around line 57-72: The `_cemoji_renderable` probe is checking
`fontgen.render_range()` in Python, but that can pass even when the
`fontconvert` binary still lacks PNG-capable FreeType, so the skip flag is
wrong. Update `_CEMOJI_OK` to verify the C binary path directly by invoking the
same `fontconvert` execution path used by `ColorEmojiParityTest` and treat
failures there as non-renderable. Keep the existing helper name
`_cemoji_renderable`/`_CEMOJI_OK`, but change the probe so it matches the
subprocess-based parity test rather than the Python renderer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5aba5e12-b37b-4623-9f77-3d026997f39f
📒 Files selected for processing (18)
polyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/host.pypolyhost/services/fontgen.pypolyhost/services/fontgen_color.pypolyhost/services/fontgen_dither.pypolyhost/services/fontpack_extend.pypolyhost/services/fontpack_reader.pypolyhost/services/fontpack_render.pysetup.pytests/gui/fontpack_extend_dialog_test.pytests/gui/fontpack_inspector_dialog_test.pytests/services/fontgen_color_test.pytests/services/fontgen_dither_test.pytests/services/fontgen_test.pytests/services/fontpack_extend_test.pytests/services/fontpack_reader_test.pytests/services/fontpack_render_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
polyhost/services/font_downloader.py (1)
44-46: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate flattened cache-key uniqueness while parsing the catalog.
Line 46 collapses every
desttobasename(dest), andlocal_path()/is_downloaded()later use that flattened value as the sole cache key. If two YAML entries ever share a basename, the picker will expose two fonts that alias the same on-disk file and cached-state marker. Fail fast here instead of relying on the test suite to catch drift.Proposed fix
def load_catalog(path: str | None = None) -> list[NotoFont]: """Parse noto-fonts.yaml into a list of NotoFont. The host stores a flat cache, so the local filename is the basename of the firmware-side ``dest``.""" import yaml with open(path or _catalog_path()) as f: doc = yaml.safe_load(f) or {} - out = [] + out = [] + seen_filenames = set() for e in doc.get("fonts", []): - out.append(NotoFont(name=e["name"], url=e["url"], - filename=os.path.basename(e["dest"]))) + filename = os.path.basename(e["dest"]) + if filename in seen_filenames: + raise ValueError(f"duplicate cache filename in noto-fonts.yaml: {filename}") + seen_filenames.add(filename) + out.append(NotoFont(name=e["name"], url=e["url"], filename=filename)) return out🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/services/font_downloader.py` around lines 44 - 46, The catalog parsing in font_downloader should validate that flattening dest to basename does not create duplicate cache keys. Update the parsing loop that builds NotoFont entries to track each generated filename and fail fast if two fonts would share the same basename, since local_path() and is_downloaded() treat filename as the unique cache key. Ensure the check is done where doc["fonts"] is converted into NotoFont objects so collisions are caught during catalog load.tests/gui/fontpack_extend_dialog_test.py (1)
47-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression for an unknown/external prefill bundle.
This only covers the happy path where
prefill["bundle"]exists in the shipped-pack list. Please add a case that asserts an unresolved bundle is rejected instead of silently targeting another pack; that would lock in the edit-target fix above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gui/fontpack_extend_dialog_test.py` around lines 47 - 58, Add a regression test in FontPackExtendDialog for an unknown or external prefill bundle: extend test_prefill_targets_glyph or add a sibling case that passes a prefill["bundle"] not present in the shipped list and verify the dialog does not resolve it to another pack. Use FontPackExtendDialog and its _bundle selection behavior to assert the unresolved bundle is rejected or left unset rather than silently targeting a different bundle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polyhost/gui/fontpack_extend_dialog.py`:
- Around line 44-45: The dialog is rebuilding _packs from load_shipped_packs()
and losing the injected bundle it was opened with, so edit/save can target a
different bundle than the one being inspected. Update FontpackExtendDialog to
preserve and prefer the actual inspected (label, Pack) source when present, and
ensure the edit-mode setup in the dialog flow and the save/flash path both use
that same selected bundle rather than falling back to a whole-font splice. Check
the _packs initialization, the edit/save enablement logic, and the bundle
resolution used by the save handler so they all stay bound to the original
inspected pack.
- Around line 338-351: The Noto download is still running on the GUI thread, so
the dialog freezes and Cancel cannot stop the transfer. Move the
`self._fdl.download_font(...)` work out of `_download()`/the `cb` progress
callback path and into a background worker or thread, and make `download_font()`
support cancellation during `urlopen()`/`read()` by checking a cancel flag or
abort signal. Wire `QProgressDialog`’s cancel state into that cancel path, and
have `_download()` stop updating once `wasCanceled()` is set.
In `@polyhost/services/fontpack_render.py`:
- Line 44: The docstrings in fontpack_render still use the ambiguous
multiplication symbol `×`, which Ruff flags as RUF002. Update the affected
docstring text in the relevant render helpers to use ASCII `x` instead of `×`,
keeping the wording the same otherwise; check the docstrings around the image
size descriptions in the functions referenced by the comment.
- Around line 123-125: The semicolon-separated draw calls in the font grid
rendering logic violate Ruff E702, so split them into separate statements.
Update the drawing code in the loop that uses draw.point so each call is on its
own line, keeping the behavior the same while matching the style rule.
- Around line 163-171: In fontpack_render.py, the glyph cap logic in the
rendering flow treats any truthy max_glyphs as a limit, which incorrectly allows
negative values and can break islice or render nothing for -1. Update the
conditional around the glyphs/islice handling so only positive max_glyphs values
enable capping, and keep the uncapped path for zero or negative inputs; use the
existing _iter_glyphs(pack) and capped handling in the same block.
- Line 114: The direct glyph lookup in glyph_cell still bypasses the
bounds-checked helper and can render the wrong glyph when cp is below
font.first. Update glyph_cell to reuse _glyph_for for the glyph selection
instead of indexing font.glyphs directly, keeping the lookup logic consistent
with the helper used above.
In `@tests/services/font_downloader_test.py`:
- Around line 22-24: The font URL assertion in the font downloader test is too
loose and allows insecure http:// links to pass. Update the checks in the font
downloader test around the fonts loop to require f.url to start with https://
instead of just http, keeping the existing filename extension assertion intact.
Use the same test method and the font object fields (f.name, f.url, f.filename)
to locate and tighten this invariant.
In `@tests/services/fontgen_test.py`:
- Around line 64-70: The `fontconvert` capability probe in the test helper is
invoking an external process without any timeout, so a hung binary can stall
test selection. Update the probe in the capability-check function that calls
`subprocess.run` to pass a reasonable timeout and handle the timeout the same
way as other probe failures so the color parity tests are skipped cleanly.
In `@tests/services/fontpack_render_test.py`:
- Around line 128-129: The empty placeholder assertion in fontpack_render_test
is too weak because it only checks for any non-zero pixel and would still pass
if the placeholder became full white. Update the test around the empty cell
check to explicitly verify that the rendered placeholder is not all 255 values,
using the existing empty image object in the fontpack_render_test case and
keeping the check focused on the empty placeholder rendering behavior.
---
Nitpick comments:
In `@polyhost/services/font_downloader.py`:
- Around line 44-46: The catalog parsing in font_downloader should validate that
flattening dest to basename does not create duplicate cache keys. Update the
parsing loop that builds NotoFont entries to track each generated filename and
fail fast if two fonts would share the same basename, since local_path() and
is_downloaded() treat filename as the unique cache key. Ensure the check is done
where doc["fonts"] is converted into NotoFont objects so collisions are caught
during catalog load.
In `@tests/gui/fontpack_extend_dialog_test.py`:
- Around line 47-58: Add a regression test in FontPackExtendDialog for an
unknown or external prefill bundle: extend test_prefill_targets_glyph or add a
sibling case that passes a prefill["bundle"] not present in the shipped list and
verify the dialog does not resolve it to another pack. Use FontPackExtendDialog
and its _bundle selection behavior to assert the unresolved bundle is rejected
or left unset rather than silently targeting a different bundle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e2e5861-87c0-4bfe-8892-007373c6991a
📒 Files selected for processing (17)
CLAUDE.mdpolyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/host.pypolyhost/res/fonts/noto-fonts.yamlpolyhost/services/font_downloader.pypolyhost/services/fontgen.pypolyhost/services/fontpack_reader.pypolyhost/services/fontpack_render.pytests/gui/fontpack_extend_dialog_test.pytests/gui/fontpack_inspector_dialog_test.pytests/services/font_downloader_test.pytests/services/fontgen_color_test.pytests/services/fontgen_dither_test.pytests/services/fontgen_test.pytests/services/fontpack_reader_test.pytests/services/fontpack_render_test.py
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/services/fontgen_dither_test.py
- polyhost/host.py
- tests/services/fontpack_reader_test.py
- polyhost/services/fontpack_reader.py
- polyhost/services/fontgen.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polyhost/gui/fontpack_extend_dialog.py`:
- Around line 417-419: The widget setup in this dialog uses semicolon-packed
statements that trigger Ruff E702, so split them into separate lines. In the
code path that creates the Close button and populates the layout, separate the
QPushButton("Close") assignment from the clicked.connect(self.reject) call, and
keep each btns.addWidget/addStretch call on its own line so the dialog setup
remains lint-clean.
In `@polyhost/res/fontpack/fontpack_render_settings.json`:
- Around line 1-1182: The saved settings entries for the Canadian Aboriginal and
Cherokee variants are using source_file names that do not match any basename in
the Noto catalog, so the auto-fill lookup in the font settings workflow will
fail. Update the affected records in fontpack_render_settings.json to use the
exact catalog filenames, or add matching entries in the Noto catalog so the
lookup path used by the fontpack settings loader can resolve them correctly.
In `@polyhost/services/font_downloader.py`:
- Around line 93-123: The temporary download path in font_downloader’s download
flow is shared across concurrent attempts, so one caller can overwrite or delete
another caller’s partial file. Update the logic around the
tmp/urllib.request.urlopen/open/os.replace sequence to use a unique temp file
per download attempt (for example via a unique suffix or temp file helper tied
to final/font.filename), and ensure cleanup only removes that attempt’s own temp
path before os.replace(final) runs.
- Around line 99-101: The font download flow is using a shared fixed .part temp
path, which can collide when overlapping requests target the same font. Update
the download logic in the font downloader function that builds the
urllib.request.Request and calls urllib.request.urlopen to create a unique
per-request temporary filename, write the response there, and then atomically
rename it to the final output path after a successful download.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 779743f9-b9ec-4973-bac0-db87f800b406
📒 Files selected for processing (12)
CLAUDE.mdpolyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/res/fontpack/fontpack_render_settings.jsonpolyhost/services/font_downloader.pypolyhost/services/fontpack_render.pyrequirements.txtsetup.pytests/gui/fontpack_extend_dialog_test.pytests/services/font_downloader_test.pytests/services/fontgen_test.pytests/services/fontpack_render_test.py
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/services/fontpack_render_test.py
- polyhost/services/fontpack_render.py
- tests/services/font_downloader_test.py
- polyhost/gui/fontpack_inspector_dialog.py
- tests/services/fontgen_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polyhost/gui/fontpack_inspector_dialog.py (1)
143-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent fractional zoom values from rendering at the wrong scale.
The control accepts typed decimals, but
_rebuild()truncates them withint(...), so a visible zoom like1.5still renders at scale1. Constrain this widget to whole numbers (or switch toQSpinBox) so the displayed value matches the rendered output.Suggested fix
self._zoom = QDoubleSpinBox() self._zoom.setRange(1.0, 8.0) + self._zoom.setDecimals(0) self._zoom.setSingleStep(1.0) self._zoom.setValue(2.0)Also applies to: 252-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/gui/fontpack_inspector_dialog.py` around lines 143 - 147, The zoom control in the font pack inspector allows fractional input, but `_rebuild()` effectively truncates it, so the rendered scale can disagree with the displayed value. Update the `_zoom` widget in `FontPackInspectorDialog` to accept only whole-number zoom levels, or replace `QDoubleSpinBox` with `QSpinBox`, and keep the existing `_rebuild` flow unchanged so the value shown by `_zoom` always matches the scale used during rebuild.
🧹 Nitpick comments (1)
polyhost/gui/fontpack_inspector_dialog.py (1)
121-123: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a deque for the incremental peek queue.
pop(0)shifts the whole list on every preview, so large empty-slot passes add avoidable O(n²) queue overhead right in the path meant to keep the emoji bundle responsive.collections.dequekeeps this pass linear.Suggested fix
- self._peek_queue = [] # (item, font, cp) empties awaiting a preview + self._peek_queue = deque() # (item, font, cp) empties awaiting a preview … - self._peek_queue = [] + self._peek_queue = deque() … - it, font, cp = self._peek_queue.pop(0) + it, font, cp = self._peek_queue.popleft()from collections import dequeAlso applies to: 262-262, 287-301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polyhost/gui/fontpack_inspector_dialog.py` around lines 121 - 123, The incremental preview queue in FontPackInspectorDialog is using a list, so repeated pop(0) calls in the peek path are causing avoidable O(n²) overhead. Update the queue implementation in the dialog’s peek/build flow (including the _peek_queue setup and the consumer logic around the preview pass) to use collections.deque instead of a list, and switch any list-style front removal to deque-friendly operations so the incremental empty-slot preview remains linear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polyhost/services/fontpack_extend.py`:
- Around line 37-38: The settings loader in fontpack_extend should validate the
cached `by_global_index` value before returning it, because
`json.load(f).get("by_global_index", {})` can still yield a non-dict from a
malformed file and later callers expect `.get(...)` to exist. Update the logic
in the `load`/file-read path to verify the parsed value is a dictionary and fall
back to `{}` otherwise, so the dialogs that consume this cached result do not
crash.
In `@tests/services/fontpack_extend_test.py`:
- Around line 118-123: The test in test_shipped_loads is too order-dependent
because it checks only next(iter(m.values())) for source_file. Update the
assertion to validate the shipped render settings mapping as a whole by checking
that at least one entry in load_render_settings() contains source_file, rather
than relying on insertion order. Use the load_render_settings symbol and keep
the skipTest behavior unchanged.
---
Outside diff comments:
In `@polyhost/gui/fontpack_inspector_dialog.py`:
- Around line 143-147: The zoom control in the font pack inspector allows
fractional input, but `_rebuild()` effectively truncates it, so the rendered
scale can disagree with the displayed value. Update the `_zoom` widget in
`FontPackInspectorDialog` to accept only whole-number zoom levels, or replace
`QDoubleSpinBox` with `QSpinBox`, and keep the existing `_rebuild` flow
unchanged so the value shown by `_zoom` always matches the scale used during
rebuild.
---
Nitpick comments:
In `@polyhost/gui/fontpack_inspector_dialog.py`:
- Around line 121-123: The incremental preview queue in FontPackInspectorDialog
is using a list, so repeated pop(0) calls in the peek path are causing avoidable
O(n²) overhead. Update the queue implementation in the dialog’s peek/build flow
(including the _peek_queue setup and the consumer logic around the preview pass)
to use collections.deque instead of a list, and switch any list-style front
removal to deque-friendly operations so the incremental empty-slot preview
remains linear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c842bc68-9e7d-4954-b0c9-f03de17deeea
📒 Files selected for processing (9)
CLAUDE.mdpolyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/res/fonts/noto-fonts.yamlpolyhost/services/font_downloader.pypolyhost/services/fontpack_extend.pytests/gui/fontpack_extend_dialog_test.pytests/gui/fontpack_inspector_dialog_test.pytests/services/fontpack_extend_test.py
✅ Files skipped from review due to trivial changes (2)
- polyhost/res/fonts/noto-fonts.yaml
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- polyhost/gui/fontpack_extend_dialog.py
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds host-side font-pack services for decoding, rendering, downloading, inspecting, and extending PlyF bundles, along with two Qt dialogs, tray-menu wiring, packaging updates, docs, and tests. ChangesFont-pack inspect and extend system
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polyhost/services/fontpack_extend.py`:
- Around line 62-63: The `fontpack_extend.py` options parsing in the manifest
handling is treating explicit 0 values for `gamma` and `contrast` as missing
because of the `or 1.0` fallback. Update the logic that builds these fields so
it distinguishes absent keys from present numeric zero values, preserving `0`
when provided while still defaulting to `1.0` only when the option is not set.
Use the existing manifest parsing block around `gamma_val` and `contrast` to
make the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ed2d2fc-2bab-4620-a40a-46759bd68cac
📒 Files selected for processing (7)
CLAUDE.mdpolyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/services/fontpack_extend.pytests/gui/fontpack_extend_dialog_test.pytests/gui/fontpack_inspector_dialog_test.pytests/services/fontpack_extend_test.py
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- polyhost/gui/fontpack_inspector_dialog.py
- polyhost/gui/fontpack_extend_dialog.py
Host-side tooling to visually inspect external-flash font-pack (.plyf) bundles without fontconvert or a connected device. Pure stdlib + PIL; no native deps. - polyhost/services/fontpack_reader.py: full PlyF body decoder (header, font table, glyph arrays, 1-bit bitmaps) into renderable PackFonts. Carries each font's global ALL_FONTS index (record `reserved`) and a merge_fonts() that reconstructs the firmware's front-to-back priority order across bundles. - polyhost/services/fontpack_render.py: PIL renderer mirroring the keycap OLED blit (MSB-first, bit=yy*w+xx) — native-size glyph image + dedup-by-priority contact sheet. - tests/services/fontpack_reader_test.py: synthetic round-trip + shipped-bundle smoke tests (all 6 res/fontpack/*.plyf decode, CRCs pass, sizes reconcile). Verified: all shipped bundles decode byte-perfect (4894 codepoints), glyphs rasterise correctly (contact sheets for emoji/symbol/flags). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
A standalone Qt tool, also launchable from the tray ("Inspect Font Packs..."),
that renders every glyph of each .plyf bundle exactly as the keycap OLED draws
it. Thin view over the Qt-free reader/render services, so it needs no device and
runs disconnected / in client mode.
- polyhost/gui/fontpack_inspector_dialog.py: FontPackInspectorDialog — one tab
per bundle with a metadata header (abi/content version, font/glyph counts,
size, CRC) and a zoomable, scrollable contact sheet. Pluggable sources (defaults
to the shipped res/fontpack bundles; accepts any [(label, Pack)] so a live-device
or trial pack can be fed later). Standalone: python -m
polyhost.gui.fontpack_inspector_dialog.
- polyhost/host.py: tray action + open_fontpack_inspector(), beside the MRU
inspector.
- fontpack_render: clamp out-of-range bitmap reads so a corrupt/truncated pack
renders what it can instead of crashing the window.
- tests/gui/fontpack_inspector_dialog_test.py: offscreen construction over a
synthetic pack, an error source, and the shipped bundles.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Second view mode showing each pack glyph composited into the real keycap window exactly as the firmware draws it, alongside the existing native-size glyph grid. - fontpack_render.keycap_image(): mirrors kdisp_write_gfx_char — horizontally centred, vertically placed at BASELINE + (font.yAdvance - base_yadv) + yOffset (base_yadv defaults to IconsFont's 40, so a tall flag font sits shifted down just like on hardware), clipped to 72x40. contact_sheet() gains mode="keycap" (framed 72x40 cells). - fontpack_inspector_dialog: a "View" combo switches Glyph grid <-> Keycap preview; tabs render lazily and re-render on mode/tab change, zoom rescales the cached pixmap. - tests: render-layer unit tests (native size, centring, baseline shift = yAdv delta, off-window clipping, truncated-bitmap safety, both sheet modes) + a dialog mode-switch test. Verified: flags render with the correct baseline-down placement in keycap cells. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Pure-Python port of the C fontconvert render + dither pipeline (freetype-py + uharfbuzz + NumPy), so the host can BUILD trial font-pack glyphs from a TTF/OTF with no compiled binary and no per-platform packaging — the foundation of the font-pack extend / round-trip path. - polyhost/services/fontgen_dither.py: faithful NumPy port of dither.c — bgra→gray (over black), bilinear scale, fit_dimensions, normalize(-N)/unsharp(-U)/gamma(-G)/ contrast(-c)/exposure(-e), fs/stucki/bayer/threshold/random dithers, invert(-I), interior edges(-E), outline(-O alpha-inner + morphological). MSB-first packing. - polyhost/services/fontgen.py: FreeType render/emit layer — TT interpreter v35/v40 (the byte-exactness knob), DPI 141, strike select, variable-weight(-w), metric rescale, range mode → fontpack_reader.PackFont. freetype/uharfbuzz/numpy lazily imported; declared as the optional [fontgen] extra in setup.py. - tests: pure dither unit tests + render tests with an opt-in C cross-check (FONTCONVERT_BIN / fontconvert on PATH). Verified byte-for-byte vs the C tool (FreeType 2.13.2 both sides) across fonts, ranges, all four deterministic dither modes (incl. order-sensitive FS/Stucki), -N/-G/-c/-e/-U, -O, -X/-Y/-o/-n metrics, and missing-glyph handling. Range mode done; HarfBuzz sequence/composite (flags, ZWJ emoji) is the next step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Implements fontconvert -S (and -C) in fontgen.render_sequence: shape each comma-group with uharfbuzz, emit one GFXglyph per shaped glyph id (non-composite) or one composited glyph per group (-C), at codepoints [seq_first, +count-1]. This is the flag / ZWJ-emoji generation path (gen-lang-fonts.sh uses -S -F). - Refactored the per-glyph emit into _emit_loaded_glyph, shared by range and sequence modes; masked the native yAdvance with &0xFF (the C footer's uint8_t). - Non-composite path is byte-for-byte vs the C tool incl. GSUB ligature shaping (verified on DejaVu fi/fl, separate codepoints, comma groups, gray mode, -F base). - Composite (-C) path: bitmap byte-exact; advance from FreeType (as hb_ft does) so base+mark clusters (its real use) are exact — multi-base xAdvance can differ by ≤1px because uharfbuzz has no FT-backed font (documented). Not used by flags/emoji. - tests: render_sequence well-formed tests + C-parity for separate/comma/ligature/ gray sequences and composite base+mark (exact) / multi-base (bitmap-only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Mirror extract_range_ft's load flow: load WITHOUT FT_LOAD_RENDER (which raises "unimplemented feature" trying to re-render an embedded colour bitmap), then FT_Render_Glyph only for outline glyphs. Factored into _load_glyph_rendered, shared by range + sequence modes. Verified byte-for-byte vs the C tool on NotoColorEmoji (FreeType 2.13.2 both sides): plain -g colour, the full emoji-category chain (-g -N -I -E -O1 -Dfs -r40 -Y48), -W width scaling, and shaped colour flag sequences (-g -r54 -W72 -O1 -e-0.10 -S regional-indicator pairs). The whole PolyKybd font pipeline — mono, gray, colour, range and sequence — now reproduces fontconvert exactly. - tests: ColorEmojiParityTest, gated on a NotoColorEmoji (NOTO_CEMOJI env or /tmp) AND a PNG-capable FreeType — skips (never errors) otherwise. - documented the colour-emoji PNG requirement: freetype-py's bundled libfreetype lacks PNG, so colour glyphs need a PNG-enabled FreeType (mono/gray unaffected). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Completes the extend path — build glyphs from a TTF/OTF, splice into a bundle, save/flash the result: - fontpack_reader: encode_pack() (.plyf writer, byte-identical to the firmware serializer — verified by decode→encode round-tripping all 6 shipped bundles exactly), splice_font() (replace by global_index or insert in order), and PackFont.bitmap_content_len() (trims decode's inter-font padding). - fontpack_extend: render_packfont() (fontgen → PackFont with global_index) + splice_into_bundle()/splice_into_pack_bytes() (decode → splice → re-encode, content_version bumped). Pure-stdlib splice/encode; fontgen lazily imported. - fontpack_extend_dialog: Qt form — source font, range/sequence, the fontconvert options, target bundle + auto global index; Build/Preview (keycap contact sheet), Save .plyf…, and Flash (injected callback, hidden with no device). Reached from a new "Extend…" button in the inspector; standalone via -m. - tests: writer round-trip (byte-identical shipped bundles) + splice unit tests, extend API (build→splice, replace vs add), and the dialog driven offscreen end-to-end (build→preview→splice→flash callback). Verified: build a font → splice → re-encode → decode is valid (CRC ok, font added, originals untouched, version bumped); dialog round-trip works offscreen. Live in-dialog device flash is the remaining wiring (Save .plyf + `polyctl fontpack flash` already gives a complete round-trip today). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
NotoColorEmoji's CBDT glyphs are PNG-compressed; freetype-py's bundled libfreetype lacks PNG (raises "unimplemented feature"), which previously forced a system FreeType on Linux/Mac and would have meant bundling a DLL on Windows. Instead decode the CBDT strike with fontTools and the PNG with Pillow, reproducing FreeType's premultiplied BGRA exactly — so the colour path works from pure wheels on every OS, no native juggling. - fontgen_color: ColorBitmapFont reads the strike (picked like setup_face_size), pulls each glyph's PNG, and emits premultiplied BGRA via (c*a+127)//255 in BGRA order — verified byte-identical to FreeType 2.13.2, with CBDT SmallGlyphMetrics (Advance/BearingX/BearingY) == FreeType advance/left/top. - fontgen: unified ColorRaster (FreeType slot or CBDT extractor); colour-bitmap fonts route through fontgen_color, mono/gray/outline stay on FreeType. Range and sequence modes both supported. - setup.py: add fonttools to the [fontgen] extra. Dropped the PNG-FreeType caveat. - tests: fontgen_color unit tests; the existing colour parity tests now pass via this path on a PNG-less FreeType. Verified byte-for-byte vs the C tool on the bundled (no-PNG) freetype-py: plain -g, the emoji-category chain (-N -I -E -O1 -r40 -Y48), -W scaling, and shaped colour flag + ZWJ sequences; mono/gray unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Correctness / robustness (the Major findings): - host.py: keep "Inspect Font Packs…" enabled while disconnected — it inspects shipped bundles offline, but managed_connection_status() left it greyed out. - fontpack_extend_dialog: Save/Flash now splice into the bundle captured at Build time (not the live combo), so switching bundles after Preview can't splice into the wrong slot; invalidate the build + disable Save/Flash on bundle change; filter load_shipped_packs() to valid Packs (drop (label, Exception) targets) and disable Build when none remain. - fontgen.render_range: reject first>last and enforce the 16-bit codepoint contract (bits!=32 → emitted cp ≤ 0xFFFF), mirroring render_sequence / the C tool's main() guards, so no out-of-contract PackFont reaches the writer. - fontgen sequence parsing: raise on an invalid hex token instead of silently dropping it (would otherwise flash a partial font). - fontpack_reader.decode_pack: reject unsupported ABI and out-of-bounds/misaligned table_off before parsing. encode_pack: validate each PackFont's glyph count vs its range and that the bitmap covers its glyph offsets. - fontpack_render: bounds-check codepoint→glyph indexing (a cp below first would index negatively) and apply max_glyphs during iteration (islice) so a huge range isn't fully materialised before truncation. Tests: - colour parity gate now probes the C `fontconvert` binary (the Python path no longer needs PNG-FreeType, so it would falsely enable the parity class); dropped implicit /tmp font discovery (Bandit S108) in favour of NOTO_CEMOJI / the repo fixture; narrowed import guards to ImportError + added freetype to the colour gate. Deliberately skipped: the Ruff E702/E701 line-split comments (no lint gate configured in this repo; the semicolon style is used elsewhere) and the grayscale fit_dimensions suggestion (the C tool does NOT scale the gray path — applying it would break byte-parity; -r/-W only scale colour strikes). All 66 fontpack/fontgen tests pass (with C tool + NotoColorEmoji). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
`python -m polyhost.gui.fontpack_extend_dialog` created the dialog without keeping a reference (`FontPackExtendDialog().show()`), so PyQt garbage-collected it immediately — the window flashed up and disappeared while the event loop kept running. Hold the reference like the inspector's main() does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Addresses testing feedback on the font-pack inspector: - Empty entries are now obvious. Bundles use contiguous codepoint ranges the source fonts only sparsely fill, so many entries have no glyph (e.g. symbol's U+25AD–25FF; one font is 190 empty of 193 — confirmed intended, not a bug). Those render as a dashed "ø" placeholder (distinct from a black glyph), the header shows the empty count, and a "Hide empty" toggle filters them. - Flow layout: the contact-sheet pixmap is replaced by a virtualized QListView icon grid — reflows to width (vertical scroll only) and renders only visible items, so the ~1200-glyph emoji bundle stays responsive. (fontpack_render gains glyph_cell() for one labelled cell.) - Edit / peek: double-click a glyph (or select + "Edit…") opens the extend dialog pre-targeted at that codepoint — for an empty cell this is the peek workflow (pick a source font + options, Build to preview, Save/Flash to take it). - Single-glyph insert: fontpack_reader.replace_glyph() inserts/replaces one glyph in a font, preserving its siblings; the extend dialog's edit mode uses it so editing one codepoint in a multi-glyph font no longer drops the rest (the naive whole-font splice did). The extend dialog gains a `prefill` for edit targeting. Tests: replace_glyph (sibling preservation, out-of-range), glyph_cell (present vs empty), inspector flow/hide-empty/double-click, extend prefill. Full suite 73 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The font-pack extend dialog now has a "Download Noto…" button next to the source-font picker: it fetches the Noto source fonts (the same set the firmware's dl-fonts.sh downloads) into a per-user cache and selects the result, so building/extending a bundle no longer needs a manual font hunt. The font list is a single YAML catalog (name/url/dest) shared byte-identically with qmk_firmware's fonts/noto-fonts.yaml — the host ships its own copy at polyhost/res/fonts/noto-fonts.yaml since an installed host has no firmware checkout. font_downloader.py (Qt-free, stdlib + PyYAML) parses it, caches to a .part temp then renames, and skips already-cached fonts. Tests: font_downloader_test (catalog parse, host/firmware byte-identity, download/skip via file://) + NotoDownloadDialog GUI tests (lists catalog, cached font picked with no network). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Critical: load_catalog() opened noto-fonts.yaml with the platform default encoding, so the "Download Noto…" button crashed on Windows (cp1252) with "'charmap' codec can't decode byte 0x8f" — the YAML comments carry — /⚠️ . Read it as UTF-8 (and the dl-fonts.sh parser too). CodeRabbit review fixes: - font_downloader: fail fast on duplicate basename cache keys in the catalog; add cooperative cancellation (cancel_event) to download_font, cleaning up the .part file on cancel/error. - extend dialog: the Noto download now runs on a background QThread (_DownloadWorker) so the GUI stays responsive and Cancel actually aborts the transfer; result/error captured via direct connections to avoid a post-loop race. - extend dialog: keep edit/save bound to the *inspected* bundle — accept a `sources` arg (the inspector passes its own list), raise on an unknown prefill bundle, and raise (instead of silently doing a whole-font splice) when an edit target no longer resolves. - fontpack_render: use _glyph_for in glyph_cell (bounds-checked), only treat positive max_glyphs as a cap, split semicolon draw calls, ASCII x in docstrings. - tests: assert https:// in the catalog, timeout the fontconvert probe, assert the empty placeholder stays non-white, add unknown-bundle + threaded-download cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The .plyf carries only rendered bitmaps, not the fontconvert options, so the edit dialog couldn't show "the settings this glyph was built with". Ship a global-index → fonts.yaml-options manifest (fontpack_render_settings.json, emitted by the firmware's generate_fonts.py) and prefill the edit controls from it: size, grayscale, dither, normalize/invert/edge/outline, render size, yAdvance, max width. The source TTF still isn't bundled, so the user re-picks it (or "Download Noto…"); everything else is restored. Missing/unknown index degrades gracefully (no prefill). Tests: edit prefills the saved settings for an emoji-style record; unknown index returns False. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The render-settings manifest now records each font's source_file (basename of the source TTF, matching noto-fonts.yaml). When editing a glyph, the dialog auto-fills the source font from the download cache if that file is already present — so an edit can need zero manual setup. If it isn't cached, the status names the file and points at Download Noto…/Browse. (The TTF itself still isn't bundled.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
- requirements.txt: promote freetype-py / uharfbuzz / fonttools from the optional [fontgen] extra to core deps, so Build (extend/edit a glyph) works out of the box. The extra is kept as a no-op alias for back-compat. - Build: pre-check the font-generation deps and show an actionable install hint instead of a raw "No module named 'freetype'". - Noto download dialog: add a "Download all" button (cancellable, shared multi- file worker) that fetches every catalog font into the cache; marks refresh to ✓ cached. Single-download path unchanged (selects + uses the font). - Inspector: default zoom level 2 (was 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
- font_downloader: download to a unique tempfile.mkstemp(...".part") per attempt instead of a shared "<final>.part", so two overlapping downloads of the same font can't clobber/delete each other; if a concurrent attempt finished first, drop ours and use it. - noto-fonts.yaml: add Noto Sans Canadian Aboriginal + Cherokee (both 200 on Google Fonts) so every render-settings source_file resolves in the catalog (those two edit-prefill source fonts previously fell back to manual Browse). - extend dialog: split the semicolon-packed button-row statements (Ruff E702). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
- Add Weight (-w) and X-shift (-X) controls; both are applied from the saved render settings on edit and passed into RenderOptions (0 weight in the UI = unset/-1). Weight matters for the 15 CJK/Georgian/Armenian fonts that would otherwise re-render at the font's light default instance. - Replace the modal "Download Noto…" dialog with an embeddable NotoDownloadPanel attached to the right of the extend dialog, toggled by the (now checkable) button — no more stacked modals. Picking/downloading a font fills the source field via a font_chosen signal. NotoDownloadDialog stays as a thin standalone wrapper around the panel (back-compat). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Adds a per-bundle "Peek empty (from source)" toggle that renders each empty slot from its source font — using the shipped render settings (global index → opts + source_file) and the cached Noto font — and shows them as amber-tinted previews, clearly distinct from the real (white) pack glyphs and labelled "preview … not in pack". Double-clicking a preview opens the edit flow for that codepoint, so the workflow is: peek → tweak → take. Rendering runs through FreeType behind a cancellable progress dialog; slots with no settings / uncached source / no source glyph fall back to the dashed placeholder (so peek is a no-op until you download the fonts). New Qt-free helpers in fontpack_extend: load_render_settings, render_options_from_manifest, peek_source_glyph (the dialog now shares load_render_settings too). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The eager "render all empties behind a progress dialog" pass was rough on the ~1200-slot emoji bundle (heavy color/CBDT rendering). Build all cells as placeholders immediately, then fill peek previews a couple per event-loop tick via a QTimer, so the grid is responsive regardless of bundle size. A rebuild (zoom/mode/hide/peek toggle) bumps a generation counter that cancels the in-flight pass. Added _drain_peek() to run the pass synchronously in tests, and a test that a preview actually renders when the source font is available. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
- Rename the "Download Noto…" toggle to "Font Browser" (it downloads *and* assigns the source font), and widen the dialog while the panel is open so it doesn't squeeze the form/preview. - On edit, default-select the glyph's generation font in the browser list (NotoDownloadPanel.select_filename / current_filename). - Peek now tries the other source fonts in the same bundle, not just the slot's own: symbols mix NotoSansSymbols + Symbols2, so an empty slot in one can be previewed from another. _peek_pixmap iterates _peek_candidates (own font first, then the rest, deduped by source) and reports which source each preview came from in the tooltip. Tests: prefill selects the source in the browser; peek falls back to another bundle font's source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
… float inputs, smooth zoom 1) After the editor OK, the inspector tab now re-renders the edited glyph from the working copy (_BundleTab.apply_working) so the new version is visible, and marks edited cps with a green border (MODIFIED_RGB). Save as -> Discard reverts the tab. 2) Reset button in the editor restores the render options to the values the dialog opened with (_snapshot taken after prefill; blank defaults, or an edit prefill). 3) Float inputs (gamma/contrast/exposure/sharpen/saturation) are now a uniform fixed width with a 0.1 step, and their sliders move in 0.1 notches. 4) Preview zoom is fractional: scroll wheel steps 0.5x over 0.5-7.0 (was integer 1..12 that jumped 1/3/5). Render functions take a float scale; _px rounds to pixels (NEAREST, interpolation is fine for the keycap preview). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
An edit only updated the edited tab; a cell in another bundle that is covered-by (cyan) or overridden-by (grey) the edited font kept rendering the stale glyph. _commit_edit/_discard now rebuild the merged ALL_FONTS view honouring every bundle's working copy (_rebuild_all_fonts) and _propagate pushes it to all tabs (set_all_fonts): the edited/visible tab rebuilds now, the rest invalidate and rebuild lazily on next show, so cross-bundle precedence reflects the edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
…erdraws Redesign the grid representation (the grey/cyan duplicate-cell scheme was confusing): - ONE cell per codepoint (deduped -> continuous range, easier to read). Each cell shows the winner (front-to-back precedence): white if this bundle draws it, cyan if borrowed from another bundle (_stacks()). - Duplicates are shown as a "stack": a doubled right+bottom border (_stack_pixmap) marks that another glyph is overdrawn beneath. Hovering shows the hidden glyph(s) dim in a rich (HTML) tooltip (_l_to_data_uri + _stack_tooltip). - Double-click edits the winner; the bottom-right stack corner edits the overdrawn glyph (_edit_at, position-aware via an event filter). _on_edit/_bundle_of now target the clicked font's own bundle (may be another tab), so editing an overdrawn font from a stack lands in the right bundle. The 124 real duplicates (all in the emoji bundle) now render as stacks instead of separate cells; verified no exceptions across all shipped bundles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
…review in-slot Per request: - The stack marker is now a single thick solid border (width 4) on the right + bottom edges (was a thin doubled line). - Replace the rich image tooltip with an in-slot preview: while the cursor is on a stacked cell's bottom-right stack corner, that slot's image swaps to the overdrawn glyph (dim); it restores when the cursor moves off (or leaves the grid). Both frames are precomputed per stacked cell (_WIN_PM_ROLE / _SHADOW_PM_ROLE) and swapped by _hover via mouse-tracking in the event filter; the corner region (_in_stack_corner) is shared with the double-click-to-edit. A short plain tooltip remains. Colours (cyan winner etc.) unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
…hlight on select - A codepoint can be drawn by 3+ fonts (overlapping fonts.yaml ranges from the same source, e.g. _Light_ + _EmjEffects_ + _Emojis1_ all cover U+1F4A1). The stack marker now shows its DEPTH (one offset border line per overdrawn glyph, capped at 3), and hovering the stack corner cycles the in-slot preview through ALL overdrawn glyphs (_hover stores a frame list; _cycle_advance on a timer when >1). Tooltip lists every overdrawn font. - Selecting a glyph highlights the whole range its font wins (_on_selection, subtle tint), since the pack is organised in ranges. Cleared/rebuilt with the grid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
thpoll83
force-pushed
the
claude/fontpack-emoji-tools-x2g3nx
branch
from
July 1, 2026 13:44
a2977af to
4ccada9
Compare
Mirror the regenerated fontpack_render_settings.json (main added global ALL_FONTS indices 142 _WinSwitch_, 143 _MathHints_, 144 _Dictation_) and the corrected NotoSansMath dest (Noto_Sans_Math/) from qmk_firmware so both remain byte-identical (cmp-verified). The flag-edit test hard-coded global_index 144 as a stand-in for the language-flag font, which now collides with main's real _Dictation_ entry at that index; the flag font has no render_settings record (it is generated by gen-lang-fonts.sh, not fonts.yaml), so bump the test's index past the manifest range to exercise the true flag-record fallback (sequence-mode edit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Add a Simulate OLED checkbox to the font-pack build/edit preview so a glyph can
be previewed the way the physical per-key OLEDs actually show it, calibrated from
photos of real PolyKybd keycaps.
- simulate_oled() (fontpack_render, Qt-free, NumPy/PIL): maps a monochrome 'L'
keycap to RGB with pale-cyan emissive pixels (OLED_TINT) on true black, a soft
bluer bloom (Gaussian, screen-blended) around lit pixels, and a faint dark
pixel grid ("screen door") once each logical pixel is drawn >= 3 px, so at zoom
every pixel reads as a distinct dot — the look of the photographed keycaps.
- preview_sheet(oled=): renders the sheet in RGB with only the keycap run through
the effect; the source-font reference glyph and the chrome (labels/frames) stay
natural so you can compare the OLED render against what the font draws.
- FontPackExtendDialog: "Simulate OLED" checkbox beside "Auto update", wired to
re-render; the preview pixmap path now preserves RGB (_pil_to_pixmap).
Tests: simulate_oled colour/black/glow/grid behaviour, the RGB-vs-L sheet mode,
and the dialog checkbox + re-render.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Refine simulate_oled from feedback that the panels aren't a crisp uniform grid —
real keycaps show pixel bleed and small brightness differences between pixels:
- per-pixel brightness jitter (one seeded random dim per logical OLED pixel) so
the lit area shimmers instead of reading as one flat block;
- staggered ("zigzag") pixel grid — seam columns brick-lay half a cell on
alternate rows rather than a rigid square screen door;
- a light diffusion blur that softens the pixel edges so cells bleed together
like the real panel, killing the too-crisp square look.
The jitter is seeded so a keycap renders identically each time (no flicker on
zoom/re-render). Tests cover the flat-fill texture, per-cell brightness variation,
edge diffusion and determinism.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The pixel-bleed diffusion models the transparent keycap cover over the panel acting as a diffuser/light-guide, not the OLED emission itself — correct the docstring, inline comment and CLAUDE.md note (behaviour unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Address feedback that the panels read as white to the eye (the cyan is a camera artifact) and that the two looks should be selectable: - OLED_TINT is now a cool near-white (a hint of blue), not pale cyan; the bloom halo stays slightly bluer for a subtle cool glow. - simulate_oled gains a `stagger` flag; the crisp "oled" preset turns off jitter + diffusion and uses a square (non-staggered) grid — the raw pixel look — while "keycap" keeps the jitter + staggered grid + diffusion (through the clear cover). - preview_sheet takes `style="normal"|"oled"|"keycap"` (replacing the oled bool); only the keycap is post-processed, reference + chrome stay natural. - The extend/edit dialog's "Simulate OLED" checkbox becomes a Normal/OLED/Keycap radio group (default Normal), re-rendering on selection. Tests updated for the style param, the crisp-vs-diffused difference, and the radio group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Both OLED styles read a bit dim, and the Keycap style dimmer still because its diffusion blur spreads thin strokes and lowers their peak. Add a `brightness` gain to simulate_oled applied AFTER the diffusion blur (so it lifts exactly the strokes the blur dimmed): OLED uses 1.18, Keycap 1.5 to compensate the spread. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
brightness 1.5 clamped the keycap's lit pixels to flat white, washing out the per-pixel shimmer and the difference from the OLED style. Drop the keycap gain to 1.25 (matches OLED brightness rather than exceeding it, so the gain no longer saturates) and raise its jitter to 0.22 so the shimmer stays clearly visible. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The uniform diffusion blur read artificial — a real keycap cover diffuses unevenly. Mix a lighter (0.6x) and heavier (1.7x) blur through a smooth, seeded low-frequency mask so some patches of the keycap are sharper and others softer instead of one even smear. Seeded, so it stays deterministic (no flicker). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
tests/services/fontgen_test.py (1)
156-159: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd timeouts to the parity helpers too.
The capability probe has a timeout now, but the actual parity helpers still invoke
fontconvertwithout one. If the binary wedges on a specific font or sequence, the whole test run hangs instead of failing cleanly.Also applies to: 233-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/fontgen_test.py` around lines 156 - 159, The parity helper method _c still calls subprocess.run on fontconvert without a timeout, so it can hang the test suite if the binary wedges. Update _c (and the related parity helper at the other referenced block) to pass a timeout to subprocess.run, matching the timeout behavior already added to the capability probe, and keep the existing check=True/capture_output/text flow intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polyhost/gui/fontpack_extend_dialog.py`:
- Around line 585-604: Reject multi-glyph results when the dialog is in edit
mode by checking the output of ext.render_packfont() before accepting it in the
build flow used by FontPackExtendDialog; when self._edit_target is set, only
allow a single glyph candidate and surface an error/critical message instead of
proceeding if glyph_count is greater than one. Keep the change localized around
the render/accept path that sets self._built and updates the preview, and reuse
the existing edit-mode state and status/error handling.
- Around line 274-277: The dialog can still proceed into a build path even when
there is no target bundle, which leaves _built with an invalid pack index and
later crashes in _ok(). Update FontpackExtendDialog’s _build/_ok flow to
explicitly guard against an empty self._packs before accepting an auto-update
build, and avoid enabling OK or indexing self._packs[bi] unless a valid bundle
exists. Use the existing _default_index(), _build(), and _ok() logic to
short-circuit when no target pack is available.
In `@polyhost/gui/fontpack_inspector_dialog.py`:
- Around line 527-528: The keycap preview paths in the font pack inspector are
incorrectly converting RGB glyph cells back through the grayscale tint helpers,
so preserve the original RGB output when glyph_cell(..., mode="keycap") returns
RGB. Update the relevant rendering branches in the fontpack inspector dialog
(the ones used by the keycap preview grid) to route RGB images through
_pil_to_pixmap() instead of _pil_l_to_tinted_pixmap()/_pil_l_to_pixmap()
grayscale handling, while keeping the existing tinted path for non-RGB cells.
- Around line 744-747: The bundle lookup in _bundle_of() is too ambiguous
because it selects the first pack matching a font’s global_index, so newly
opened packs in _sources can be mistaken for older ones. Update the ownership
resolution used by Edit/Extend so it matches the actual edited pack, not just
global_index, and ensure the code paths in fontpack_inspector_dialog that add
packs to _sources and later resolve them use a stable pack identity or direct
bundle reference instead.
In `@polyhost/services/font_downloader.py`:
- Around line 49-50: The TTC fast-path in is_downloaded() is trusting any file
that starts with tag == b"ttcf", which can incorrectly cache truncated
collections. Update the TTC branch in is_downloaded() to parse and validate the
TTC offset table and each member font directory before returning True, so
incomplete sfnt files are rejected and refetched.
In `@polyhost/services/fontgen.py`:
- Around line 324-328: The sequence bounds check in render_sequence() only
validates the upper limit, so a negative opts.seq_first can still flow into
PackFont and later fail serialization. Add a lower-bound validation right after
first = opts.seq_first (before computing last or returning PackFont) to reject
negative start codepoints with a clear ValueError, alongside the existing 16-bit
upper-bound check.
In `@polyhost/services/fontpack_extend.py`:
- Around line 75-84: The _cached_face helper currently memoizes freetype.Face
instances by source_path forever, so source_has_glyph can keep using a stale
face after the underlying font file is replaced in place. Update _cached_face
and the callers that rely on it to invalidate or refresh _FACE_CACHE when the
file changes, using a file identity check such as mtime/size/hash or a
replace-aware cache key, so a re-downloaded font at the same path is reloaded
instead of reusing the old Face.
In `@polyhost/services/fontpack_reader.py`:
- Around line 138-150: In decode_pack(), add a per-glyph bounds check for each
glyph record before building the PackFont so malformed bitmap slices are
rejected early. Use the existing glyph parsing loop in fontpack_reader.py to
compute each non-empty glyph’s payload size from width and height, verify
bitmapOffset plus that size stays within the font’s bitmap block [bstart:bstop],
and raise PackDecodeError with font/glyph context if it overruns. Keep the new
validation alongside the existing glyph block and bitmap block checks in
decode_pack().
In `@tests/services/font_downloader_test.py`:
- Around line 74-77: The success-path cleanup check in the font downloader test
is using a fixed temp filename that no longer matches `download_font()`’s
randomized `.part` naming. Update the assertion in `test_download_font` to
inspect `self.cache` for any remaining files ending in `.part` after the second
`fdl.download_font(...)` call, so the test verifies no temporary partial
downloads are left behind. Use `download_font`, `fdl.download_font`, and
`self.cache` to locate the test logic.
In `@tests/services/fontgen_test.py`:
- Around line 18-26: The import guard in the test module is too broad because it
catches every Exception and hides real failures in polyhost.services.fontgen and
polyhost.services.fontgen_dither behind _ERR. Narrow the except in the
module-level import block to only the missing-dependency cases (ImportError and
shared-library OSError), so syntax errors and other import-time bugs surface
normally instead of causing all tests to skip.
---
Duplicate comments:
In `@tests/services/fontgen_test.py`:
- Around line 156-159: The parity helper method _c still calls subprocess.run on
fontconvert without a timeout, so it can hang the test suite if the binary
wedges. Update _c (and the related parity helper at the other referenced block)
to pass a timeout to subprocess.run, matching the timeout behavior already added
to the capability probe, and keep the existing check=True/capture_output/text
flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bad026f2-f492-4466-b614-e13fc93b9580
📒 Files selected for processing (25)
CLAUDE.mdpolyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/host.pypolyhost/res/fontpack/fontpack_render_settings.jsonpolyhost/res/fontpack/lang_flags.jsonpolyhost/res/fonts/noto-fonts.yamlpolyhost/services/font_downloader.pypolyhost/services/fontgen.pypolyhost/services/fontgen_color.pypolyhost/services/fontgen_dither.pypolyhost/services/fontpack_extend.pypolyhost/services/fontpack_reader.pypolyhost/services/fontpack_render.pyrequirements.txtsetup.pytests/gui/fontpack_extend_dialog_test.pytests/gui/fontpack_inspector_dialog_test.pytests/services/font_downloader_test.pytests/services/fontgen_color_test.pytests/services/fontgen_dither_test.pytests/services/fontgen_test.pytests/services/fontpack_extend_test.pytests/services/fontpack_reader_test.pytests/services/fontpack_render_test.py
✅ Files skipped from review due to trivial changes (1)
- polyhost/res/fontpack/lang_flags.json
🚧 Files skipped from review as they are similar to previous changes (6)
- requirements.txt
- polyhost/res/fonts/noto-fonts.yaml
- setup.py
- polyhost/host.py
- tests/services/fontgen_color_test.py
- polyhost/services/fontgen_color.py
Robustness fixes from PR review: - fontpack_reader.decode_pack: reject a glyph whose packed bitmap (offset + ceil(w*h/8)) overruns its font's bitmap block, so a malformed .plyf fails at decode instead of later in the renderer. - font_downloader: validate TTC collections in _validate_sfnt (parse the offset table + each member font directory) instead of trusting any 'ttcf' header, so a truncated .ttc is refetched; shared _validate_sfnt_dir helper. - fontpack_extend._cached_face: key the FreeType face cache on (mtime, size) so a font re-downloaded in place (the corrupt-cache refetch path) reloads the face instead of serving the stale one. - fontgen.render_sequence: reject a negative seq_first (unsigned on the wire). - extend dialog: guard _build against no target bundle (empty _packs would leave an invalid index that _ok() indexed and crashed on); reject a multi-glyph build in edit mode (the inspector replaces one slot, so extra glyphs were dropped silently). Also de-fancy an en-dash the linter flagged. - inspector._bundle_of: resolve the owning bundle by object identity first, so two opened .plyf that reuse a global_index can't mis-target the wrong bundle. Tests: - fontgen_test: narrow the module import guard to (ImportError, OSError) so a real bug in the modules under test surfaces instead of skipping the suite; add a timeout to the C-parity subprocess helpers. - font_downloader_test: scan the cache dir for leftover *.part (the temp name is randomized now) instead of a fixed filename. - new coverage: glyph-bitmap overrun rejected; extend-dialog no-bundle guard and edit-mode multi-glyph rejection. Not applied: the "preserve RGB in keycap mode" finding — glyph_cell always returns an 'L' image (the inspector doesn't OLED-simulate; only the extend preview does), so the grayscale pixmap path is correct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Flag keycaps were cut off at the bottom in the preview: keycap_image applied the (yAdvance - IconsFont 40) baseline-align shift, pushing a tall flag (yAdvance 54) +14 px down so its bottom rows clipped off the 40 px window. On hardware flags are drawn through a single-font array (baseline adjustment 0), so add fontpack_render.base_yadv_for(font, cp): use the flag's own yAdvance as the baseline reference for the PUA 0xE000 flag band, else the default. Emoji (yAdvance 48, drawn via g_all_fonts) keep the shift, so the fix is gated to the flag band. preview_sheet + glyph_cell now apply it per glyph — the flag's bottom row is visible again. Also add the OLED and Keycap-through-cover view modes to the inspector (matching the extend dialog's Normal/OLED/Keycap): glyph_cell renders those as an RGB cell via simulate_oled, and _BundleTab._pm keeps RGB (only the plain 'L' modes get the borrowed/shadow/peek tints). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
…-tools-x2g3nx # Conflicts: # CLAUDE.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/services/fontpack_render_test.py`:
- Around line 284-299: The assertion in test_flag_bottom_not_clipped is tied to
a hardcoded row limit instead of the placement geometry, so it may stop
validating the regression if baseline constants change. Update the test to
derive the expected last visible row from the same math used by rd.keycap_image
and rd.base_yadv_for for the font/glyph pair, and reference the
shifted_rows/fixed_rows checks rather than comparing fixed_rows[-1] directly to
39.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 960c36b7-1db9-49e5-a47a-87b98a9a1400
📒 Files selected for processing (15)
CLAUDE.mdpolyhost/gui/fontpack_extend_dialog.pypolyhost/gui/fontpack_inspector_dialog.pypolyhost/host.pypolyhost/services/font_downloader.pypolyhost/services/fontgen.pypolyhost/services/fontpack_extend.pypolyhost/services/fontpack_reader.pypolyhost/services/fontpack_render.pytests/gui/fontpack_extend_dialog_test.pytests/gui/fontpack_inspector_dialog_test.pytests/services/font_downloader_test.pytests/services/fontgen_test.pytests/services/fontpack_reader_test.pytests/services/fontpack_render_test.py
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (11)
- polyhost/services/fontpack_reader.py
- tests/services/font_downloader_test.py
- polyhost/host.py
- tests/services/fontgen_test.py
- polyhost/services/fontpack_extend.py
- polyhost/services/font_downloader.py
- tests/services/fontpack_reader_test.py
- tests/gui/fontpack_inspector_dialog_test.py
- polyhost/services/fontpack_render.py
- polyhost/services/fontgen.py
- polyhost/gui/fontpack_extend_dialog.py
Address a review note: test_flag_bottom_not_clipped hardcoded row 39, coupling it to BASELINE — a later constant tweak could make it stop distinguishing the regression. Compute the glyph's last-row window position from keycap_image's own placement math (BASELINE + yAdvance-baseref + yOffset + height-1) and assert the buggy placement lands past OLED_H (clipped) while the fixed one lands inside, then check the rendered last-lit row matches that derived bottom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
…-tools-x2g3nx # Conflicts: # CLAUDE.md
The firmware build now empties pack glyphs a higher-priority font already draws byte-identically (shadowed duplicates that never render). Reship the two bundles that shrink: symbol v5->v6 (33,980->33,788 B) and emoji v1->v2 (227,460->214,344 B, 13,116 B reclaimed). The bumped content_version makes the host re-flash them on the next connect. All other bundles are byte-identical and untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
The shipped fantasy.plyf predated three firmware commits that deliberately shrank the glyph-script fonts (render Aurebesh & Cirth smaller / size 14 / Cirth size 16). Its Aurebesh, Cirth, APL and Braille glyphs were rendered at the old larger size (124 glyphs differing), so it no longer matched the committed firmware headers (24,324 vs 23,720 B). Rebuild fantasy.plyf from the current committed headers (the pinned-toolchain source of truth) and bump content_version v2->v3 so keyboards re-flash the corrected, smaller glyphs. No dedupe glyph was in this bundle; this is purely a host/firmware resync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Move the "Inspect Font Packs…" tray entry out of the always-visible menu into the existing "Debugging" submenu, so it only appears when the app runs with --debug >= 1 (debug_mode > 0). It's a developer/power-user tool, not something the default menu should surface. Unlike the other Debugging entries it stays available in both in-process and client mode (offline — no device needed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
Merged
5 tasks
A reversed range (first > last) fell through to fontgen.render_range(), which iterates zero codepoints and yields an empty PackFont that could then be spliced into a bundle. Raise ValueError early so the dialog surfaces a clear error instead of writing bad output. (CodeRabbit review, PR #92.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018BfvAc2SC8esDJ54F2VaMr
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Host-side tooling to visually inspect the external-flash font packs and build/extend them from a TTF/OTF — entirely in Python, no compiled
fontconvertand no per-platform native packaging.Inspect (no device, no native deps)
polyhost/services/fontpack_reader.py— pure-stdlib.plyfdecoder → renderable fonts (carries each font's globalALL_FONTSindex; merge-sorts bundles into firmware priority order), plus anencode_packwriter verified byte-identical by round-tripping all 6 shipped bundles.polyhost/services/fontpack_render.py— PIL renderer mirroring the keycap OLED blit (MSB-first); glyph-grid and keycap-72×40 preview.polyhost/gui/fontpack_inspector_dialog.py— Qt window with per-bundle tabs, metadata, zoom, and a view-mode toggle. Tray entry "Inspect Font Packs…"; alsopython -m polyhost.gui.fontpack_inspector_dialog.Build / Extend (optional
[fontgen]extra: freetype-py + uharfbuzz + numpy + fonttools)polyhost/services/fontgen.py+fontgen_dither.py— a faithful Python port of the Cfontconvertrender + dither pipeline (range and HarfBuzz sequence modes). Verified byte-for-byte vs the C tool across fonts, ranges, all four deterministic dither modes,-N/-I/-E/-O/-G/-c/-e/-U/-X/-Y/-W/-r/-b, GSUB ligature shaping, and colour flag/ZWJ sequences.polyhost/services/fontgen_color.py— decodes colour-emoji (CBDT/PNG) glyphs with fontTools + Pillow, reproducing FreeType's premultiplied BGRA exactly. This removes any need for a PNG-enabled FreeType, so the colour path works from pure wheels on every OS (no system FreeType juggling, no Windows DLL).polyhost/services/fontpack_extend.py+polyhost/gui/fontpack_extend_dialog.py— build glyphs → splice into a bundle → preview → Save .plyf (or Flash via an injected callback). Reached from the inspector's "Extend…" button.All new device-coupling is optional/lazy: the inspector and
fontpack_reader/fontpack_renderneed no device and no[fontgen]deps;fontgen*import freetype/uharfbuzz/numpy/fonttools lazily.Notes / scope
fontconvertfor byte-reproducibility — the Python builder is for host-side inspection and flash-trial..plyfthenpolyctl fontpack flash <file>), and "promote tofonts.yaml". Composite-Cis bitmap-exact (multi-basexAdvancemay differ ≤1px; the base+mark case it targets is exact).Version bump label
bump:minor— new feature, backwards-compatible (additive modules + one tray menu entry; no protocol or behaviour change to existing paths).Testing
Automated: ~90 new unit tests (decoder/writer round-trip, dither + render byte-parity vs the C tool incl. colour emoji, splice/extend, Qt dialogs offscreen). C-parity and colour tests skip cleanly when the C binary / NotoColorEmoji aren't present. See the manual test plan in the PR thread.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
forcere-download.