news: stop cutting headlines short, add headline paging - #226
Conversation
The ticker was being pulled off screen mid-headline no matter how much content was left to scroll. Root cause: the plugin never declared dynamic-duration support. Its config keeps the dynamic_duration block under `global`, but BasePlugin.supports_dynamic_duration() reads a *top-level* `dynamic_duration` key, so it saw nothing and returned False. The display controller therefore treated the ticker as fixed-duration -- it never consulted is_cycle_complete() and cut the strip at a flat wall-clock time (ScrollHelper's 60s placeholder on the first turn). Implement the full hook family so the controller waits for the scroll to actually finish: supports_dynamic_duration(), get_cycle_duration(), get_dynamic_duration_cap() and reset_cycle_state(). get_cycle_duration() carries overrun slack because a loaded Pi renders below the nominal frame rate and ScrollHelper drops the missed pixels rather than catching up; the cap includes that slack too, since the controller takes min(cycle duration, cap) and a bare cap would clip it. Add headline paging on top. Headlines are never abbreviated, so a full feed set produces a strip minutes long -- more than any single turn can show. Each turn now carries as many headlines as can finish scrolling in the time available, and the next turn resumes at the first headline that didn't fit. Nothing is drawn that can't be read to the end, and the tail of a long list finally reaches the panel, replacing the old rotate-one-headline-every-three-cycles behaviour that left it unreachable. Also stop rewinding the page window on every data refresh, cache rendered headline images for the life of a refresh, and only report a scroll-derived display duration once a strip actually exists. New global.headline_paging config block, enabled by default; the legacy rotation path is kept for when it's disabled. Verified with the core safety harness (PASS at all 8 panel sizes) and a new test module covering page sizing, ordering, and the duration hooks. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01CpxDq4HS7vgmMXi5ZC168G
|
Warning Review limit reached
Next review available in: 2 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 (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 111 |
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.
PIL anti-aliases text by default, blending glyph edges into dim partial-lit pixels. On a 1:1 LED matrix those read as blur rather than as smoothing. It bites hardest at sizes that miss the font's design grid. Press Start 2P is drawn on an 8px grid, so at size 8 or 16 it lands on the pixel grid and stays crisp either way -- but at this plugin's default size of 12, a single headline picks up ~150 blended pixels. Measured across sizes 8/10/12/16/20: 0/186/151/0/258 blended pixels with the default, 0 at every size with fontmode = "1". Route every draw surface through a _pixel_draw() helper that sets fontmode = "1", covering headlines, feed labels, separators and both fallback screens. The throwaway surfaces used only to measure text go through it too, so metrics can never disagree with what actually renders. This matches the convention already used by ledmatrix-flights and the sports scoreboards. Vertical centring was checked and deliberately left alone: it centres on the font bbox rather than on ink extents, which keeps the baseline stable across headlines whether or not a given one has descenders. Test module renamed test_headline_paging.py -> test_news_ticker.py now that it covers more than paging, and extended with three crispness tests. They assert zero blended pixels -- measured as pixels that are neither background nor an exact fill colour, since the plugin draws legitimately dim greys (feed label 150, fallback 220) that a brightness threshold would wrongly flag. Each test was confirmed to fail with the fix reverted. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01CpxDq4HS7vgmMXi5ZC168G
2 participants
Pull Request
Summary
The news ticker was being pulled off screen mid-headline no matter how much content was left to scroll. This fixes the root cause (the plugin never actually opted into dynamic duration, so the display controller cut it at a flat wall-clock time) and adds headline paging, which sizes each display turn to what can genuinely finish scrolling and resumes the next turn where the last one stopped.
Type of change
Plugin(s) affected
newsRelated issues
N/A
Test plan
EMULATOR=true python3 run.py)scripts/dev_server.py)Testing done, against a core checkout:
scripts/check_plugin.py --plugin news: PASS at all 8 panel sizes (64x32, 128x32, 64x64, 96x48, 128x64, 256x32, 128x96, 256x128).plugins/news/test_headline_paging.py(13 tests) covering page sizing against the duration budget, page ordering (every headline reached, in order, no repeats), the oversized-single-headline case, paging disabled,max_headlines_per_page, the parked-page rollover, refresh not rewinding the page window, and each dynamic-duration hook.reset_cycle_state()→ frames →is_cycle_complete()sequence bounded byget_cycle_duration(): 24 headlines walked across 5 turns, every page finishing inside its wall.python scripts/check_module_collisions.py— clean.schema/manifest_schema.json.Not verified on real hardware — the timing behaviour under a loaded Pi (the case the overrun allowance exists for) is worth a look on device.
Required for plugin changes
versioninplugins/<id>/manifest.json(1.0.10 → 1.1.0)class_nameinmanifest.jsonmatches the actual class inmanager.pyexactlyentry_pointmatches the real fileREADME.md(new config keys + a Headline Paging section + troubleshooting entries)config_schema.jsonis the source of truth for the web UI form — the newglobal.headline_pagingblock has defaults, descriptions and constraints, with the fine-tuning keys markedx-advancedplugins.json)Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mdNotes for reviewer
The root cause is worth reading carefully, because it's a trap other plugins could fall into.
BasePlugin.supports_dynamic_duration()reads a top-leveldynamic_durationconfig key. This plugin has always kept that block underglobal.dynamic_duration, so the base implementation found nothing and returnedFalse. Indisplay_controller.pythat meansdynamic_enabled = False, which sends the mode down theelsebranch:max_duration = base_duration,_should_exit_dynamic()short-circuits toFalse, andis_cycle_complete()is never consulted. The ticker got exactly one fixed number of seconds and was then cut wherever it happened to be — on the very first turn that number was ScrollHelper'scalculated_duration = 60placeholder, handed out before any strip existed.So all the machinery for "don't cut headlines off" was already written; it just was never reachable. This PR implements the rest of the hook family (
supports_dynamic_duration,get_cycle_duration,get_dynamic_duration_cap,reset_cycle_state) so the controller waits for the scroll to actually finish.Two decisions I'd like a second opinion on:
Overrun slack (
duration_overrun_allowance, default 0.25). The controller treatsget_cycle_duration()as a hard wall. Under load a Pi renders below the nominal frame rate, andScrollHelper.update_scroll_position()caps catch-up atmax_steps— it drops the missed pixels rather than making them up, so the strip finishes progressively behind schedule and the wall lands mid-headline. The slack absorbs that. It's also folded intoget_dynamic_duration_cap(), because the controller takesmin(cycle duration, cap)and a cap set to the bare page duration would clip exactly the slack that's meant to help. The allowance only costs real time when the scroll genuinely ran late — the controller still hands over the instantis_cycle_complete()flips.CORE_DEFAULT_DURATION_CAP = 180.0mirrors a core constant. Page sizing has to know the ceiling that will actually be enforced, which ismin(plugin cap, display.dynamic_duration.max_duration_seconds), and the core falls back to its own 180s default when that key is unset. The plugin reads the real value fromconfig_managerwhen it can and only falls back to the mirrored constant otherwise, but it is a deliberate coupling to core behaviour and worth flagging.Behaviour notes:
rotation_enabled/rotation_thresholdpath are preserved unchanged — those two keys are now documented as only applying when paging is disabled.rotation_threshold(default 3) completed cycles. With ~18 headlines that needed ~51 cycles to bring the last one to the front, and since cycles were being cut early the tail was effectively unreachable. Paging advances a whole page per cycle instead.update()deliberately keeps_page_startacross data refreshes rather than rewinding. Rewinding everyupdate_intervalwas itself part of why the tail never showed.Generated by Claude Code