fix(7-segment-clock): draw the digits at all, and document every setting - #355
Merged
Conversation
The plugin has been rendering a completely blank panel. The digit assets are 1-bit bitmaps with no alpha channel, but `_render_digit` only lit pixels whose alpha was 0 — a condition those assets never satisfy — so no segment was ever drawn, at any panel size. Neither failure path raises, so it showed up as a black screen rather than an error. The plugin's own `test_render_polarity.py` was failing, and the committed golden images drift on all eight harness sizes. Lit segments now come from the bright pixels, and the digit and separator paths share one `_colorize` helper instead of two copies of the same pixel loop. The harness matches the existing goldens on all eight sizes again, and the polarity test passes. Its docstring described the wrong polarity too, which is how the regression got written in the first place; that is corrected. Also re-read the clock in `display()` rather than trusting the value cached by `update()`. The scheduler only calls `update()` once per manifest `update_interval` (60s here), so the panel could show a time up to a minute stale, and the flashing separator — whose state is derived from the seconds field — was frozen for that whole minute instead of blinking. `clock-simple` already refreshes this way; reading the system clock is not I/O, so it belongs in `display()`. The README is rewritten around real rendered screenshots: a hero shot, and a comparison grid for each of the display settings (time format, leading zero, separator flash, digit spacing, colour, and the four common panel sizes). Every config key now has its own section with the values it accepts and what it looks like. Two behaviours that were previously undocumented are called out: the timezone resolution order, and the fact that the auto-scaler sizes digits without accounting for `digit_spacing`, so a large gap clips the outer digits off a 64-wide panel. Adds `scripts/render_docs_assets.py`, which drives the core renderer from a declarative shot list and assembles the labelled grids, so these images can be regenerated and checked rather than hand-made. Images live in `docs/assets/` so plugin installs stay lean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 critical |
🟢 Metrics 85 complexity
Metric Results Complexity 85
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The "Available Plugins" tables named and described each plugin but never showed one. A plugin now gets a thumbnail of its hero screenshot as soon as docs/assets/<id>/hero.png exists, linked through to the plugin directory. The column is driven by scripts/update_readme_previews.py rather than kept by hand: it keys off the ./plugins/<id>/ link already in each row, so there is no second list to keep in step, and --check catches a row whose image was never committed (or an image that landed without the table being updated). Only 7-segment-clock has a hero image so far; the remaining rows fill in as each plugin's README pass lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Drop the unused `shutil` import (F401). - Resolve the `"Image.Image"` annotations. Pillow is imported lazily so `--help` works without it, which left the name undefined at module scope (F821 x3). A `TYPE_CHECKING` import of `PIL.Image.Image as PILImage` gives linters the symbol with no runtime import. - Harden the renderer invocation the two command-injection findings point at. The plugin id reaches both the subprocess argv and the `docs/assets/<id>` path, so it is now validated against the manifest id shape -- which also closes a path traversal (`--plugin ../etc` previously resolved outside the assets tree; it is now rejected). The renderer path is checked before use, `shell=False` and `check=False` are explicit, and the suppressions carry their justification on the preceding line so bandit stops parsing the prose as test ids. - Swap main()'s import-probe for `importlib.util.find_spec`, so the Pillow check no longer needs an unused import. Verified with `--check`: the committed images still re-render byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChuckBuilds
added a commit
that referenced
this pull request
Sep 2, 2026
… renders Re-lands #356, which merged but never reached main: I based it on docs/7-segment-clock-readme so it could use the render tooling before that tooling existed on main, and #355 squash-merged that branch to main *before* at since. This is the same content, cut from main instead. Documentation only; no behaviour change. The README covered roughly fifteen of the plugin's 120-odd settings, had no images, and its "Key settings" table had drifted from the schema -- it listed show_favorite_teams_only as defaulting to false (it is true), display_duration as 30 (it is 15), and showed show_odds: false in an example labelled as the defaults. Game selection gets its own section, because it is the part that surprises people. There are three distinct code paths -- no favourites, favourites exclusively, and favourites-first-then-others -- and which one runs depends on whether favorite_teams is empty and whether show_favorite_teams_only is on. Most importantly, upcoming_games_to_show and recent_games_to_show mean a per-team budget in the exclusive path and a total in the other two, so three favourites and a value of 3 is nine cards or three depending on one unrelated checkbox. Four dead ends are recorded, each verified rather than assumed: show_odds is a no-op for AFL because ESPN publishes no odds block for the league (a full finals-week payload contains zero) though it still issues one odds request per selected game; show_ranking has no poll to read; and dynamic_duration.min_duration_seconds and background_service.max_workers are in the schema but never applied. Re-verified against main rather than assumed still-current: every documented default still matches config_schema.json after #353 and #354, and the committed images re-render byte-identical against main's sports.py, which those PRs changed. The harness passes 24/24. Version bumped from main's current 1.19.0 rather than the 1.17.3 the stranded branch carried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChuckBuilds
added a commit
that referenced
this pull request
Sep 2, 2026
* docs(afl-scoreboard): document every setting, with real finals-series renders Re-lands #356, which merged but never reached main: I based it on docs/7-segment-clock-readme so it could use the render tooling before that tooling existed on main, and #355 squash-merged that branch to main *before* at since. This is the same content, cut from main instead. Documentation only; no behaviour change. The README covered roughly fifteen of the plugin's 120-odd settings, had no images, and its "Key settings" table had drifted from the schema -- it listed show_favorite_teams_only as defaulting to false (it is true), display_duration as 30 (it is 15), and showed show_odds: false in an example labelled as the defaults. Game selection gets its own section, because it is the part that surprises people. There are three distinct code paths -- no favourites, favourites exclusively, and favourites-first-then-others -- and which one runs depends on whether favorite_teams is empty and whether show_favorite_teams_only is on. Most importantly, upcoming_games_to_show and recent_games_to_show mean a per-team budget in the exclusive path and a total in the other two, so three favourites and a value of 3 is nine cards or three depending on one unrelated checkbox. Four dead ends are recorded, each verified rather than assumed: show_odds is a no-op for AFL because ESPN publishes no odds block for the league (a full finals-week payload contains zero) though it still issues one odds request per selected game; show_ranking has no poll to read; and dynamic_duration.min_duration_seconds and background_service.max_workers are in the schema but never applied. Re-verified against main rather than assumed still-current: every documented default still matches config_schema.json after #353 and #354, and the committed images re-render byte-identical against main's sports.py, which those PRs changed. The harness passes 24/24. Version bumped from main's current 1.19.0 rather than the 1.17.3 the stranded branch carried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(afl-scoreboard): re-bump to 1.19.2 after the logo fix landed #357 took the plugin to 1.19.1 while this was open, so the docs bump moves to 1.19.2 and sits on top of it. Images re-verified byte-identical after the rebase; plugins.json regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
ChuckBuilds
pushed a commit
that referenced
this pull request
Sep 2, 2026
main gained #355, #357 and #358 while this branch was in review. The only conflicts were plugins.json and the eight scoreboard manifests, all of them version bookkeeping -- no code conflicted. #357 ("retry a logo whose download previously failed") was the one worth checking, since it touches the scoreboards. It changed sports.py, not game_renderer.py, and none of the twenty methods this branch delegates to src/common/sports_card.py. Its 1.x.1 releases are kept in each manifest's version history beneath this branch's entry, and each version was recomputed as the next minor above main's rather than reusing the number this branch picked before #357 landed. plugins.json is regenerated with update_registry.py, never merged by hand.
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
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
First plugin in a pass to bring every ChuckBuilds-owned plugin README up to a
consistent standard: real rendered screenshots, and a documented explanation of
every configuration option.
While generating the screenshots for this one I found the plugin doesn't draw
anything at all.
The rendering bug
The digit assets in
assets/images/are 1-bit bitmaps with no alpha channel,but
_render_digitonly lit pixels whose alpha was0— a condition thoseassets never satisfy. Every segment was skipped, so the plugin rendered a black
panel at every size. Neither failure path raises, so it presented as "the clock
doesn't show up" rather than an error.
Evidence it was a live regression rather than intended behaviour:
plugins/7-segment-clock/test_render_polarity.pywas failing onmainmain:After the fix all eight pass against those same unmodified goldens.
Lit segments now come from the bright pixels, and the digit and separator paths
share one
_colorizehelper instead of two copies of the same pixel loop. Thepolarity test's docstring described the wrong polarity, which is presumably how
the regression got written; that's corrected too.
The stale-clock bug
display()used thecurrent_timecached byupdate(), but the scheduler onlycalls
update()once per manifestupdate_interval— 60s here. So the panelcould show a time up to a minute stale, and
has_flashing_separatorneveractually flashed: the separator state is derived from the seconds field of that
frozen timestamp, so it held one state for the whole minute and then jumped.
display()now re-reads the clock each tick.clock-simplealready does this;reading the system clock is not I/O, so it belongs in
display().README
Rewritten around real plugin output — a hero shot plus a comparison grid per
setting. Every config key gets its own section with accepted values and what it
looks like on a panel. Two previously undocumented behaviours are now written
down:
digit_spacing, so a large gap pushes the outer digits off a 64-wide panel —shown in the grid rather than just asserted
Tooling
scripts/render_docs_assets.pydrives the core renderer from a declarative shotlist (
docs/assets/<id>/shots.json) and assembles the labelled grids, so READMEimages are reproducible instead of hand-made. A
freeze_timeshim pins what theplugin thinks "now" is, so re-rendering is deterministic and
--checkcan verify the committed images still match the plugin.Images live in
docs/assets/rather thanplugins/<id>/assets/so plugininstalls stay lean.
Verification
python scripts/check_plugin.py --plugin 7-segment-clock— 8/8 PASS, goldens matchpython test_render_polarity.py— 3/3 passpython scripts/check_module_collisions.py— OK across 43 pluginsplugins.jsonregenerated viaupdate_registry.py🤖 Generated with Claude Code