feat(ledmatrix-music): opt-in adaptive layout + core element-style resolver (v1.1.1) - #187
Conversation
Add layout_mode: "classic" | "adaptive" (default classic — rendering is byte-identical unless the user opts in; verified by a golden-comparison test across the full harness size spread). Adaptive mode scales the title/artist/album fonts to the panel height, matching how the album art already scales (album_art_size = matrix_height has always been fully height-driven — only the text sizes were fixed). MusicPlugin is itself a BasePlugin, so it gets self.layout for free — unlike football-scoreboard's GameRenderer (a standalone helper class), no separate LayoutContext/FontManager wiring was needed. The vertical space above the progress bar splits into three equal rows via Region; each row's font is the largest crisp ladder rung whose LINE HEIGHT fits that row — height-only, not width, since long titles/artists/albums scroll rather than shrink (the existing marquee scroll/pause state machine is completely untouched; only the font objects and Y positions feeding into it change). Ladder reuses the same rungs verified crisp in text-display/ football-scoreboard (measure_font_crispness == 0.0): PressStart2P at exact multiples of its 8px design grid, plus 4x6-font at its actual crisp size (7px, not 6). User-configured fonts/sizes for title_text/ artist_text/album_text win over auto-sizing; y_percent overrides still apply on top of the computed row position. Revert path: set layout_mode back to "classic" in config (no reinstall), or git-revert this single commit. Tests: test_adaptive_layout_mode.py — classic-unchanged byte-identity across 5 sizes, title font height grows with panel height (measured from actual rendered pixels, not re-derived math), user-font-wins, ladder crispness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_user_font_set() treated the mere PRESENCE of customization.<element>.font or .font_size as 'the user explicitly forced this font' and skipped adaptive sizing. But the web UI's save flow (schema_manager.merge_with_defaults, api_v3.py save_plugin_config) writes the FULL schema-declared default object into config.json on every save -- for every plugin, whether or not the user touched that section. Since config_schema.json declares a font/font_size default for every element (title_text, artist_text, album_text), ANY config that has ever been saved once via the web UI already has these keys present -- meaning _user_font_set() returned True unconditionally, and adaptive font sizing never actually engaged in practice, only in a from-scratch config that had never touched the settings page. Fixed by comparing the configured (font, font_size) against the classic default for that element, not just checking presence -- only a genuine deviation from the default counts as a user override now. Verified: a config carrying only schema defaults now correctly enables adaptive sizing; a real override (e.g. 12px instead of 8px) is still respected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delegate font loading and the user-font-override check to src.element_style (new core module) instead of the plugin-local copies. The resolver reads its override reference from this plugin's own config_schema.json, so it works in every context — including the test harness and dev server, where BasePlugin.style_resolver would degrade because the mock plugin manager has no schema manager (which is also why the resolver is cached on its own attribute, not BasePlugin's). Behavior-identical: verified byte-identical renders old-vs-new across 6 configs (bare/realistic/partial x classic/adaptive + a real override) and 5 panel sizes (30 comparisons). The empty-customization early-out (display_manager fonts) and y_percent positioning are untouched; the local loader remains as the fallback path on older cores. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
|
Warning Review limit reached
Next review available in: 32 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 Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe music plugin adds opt-in adaptive typography, schema and documentation updates, release metadata, compatibility fallback, shared font resolution, and regression tests covering classic and adaptive rendering. ChangesAdaptive music layout
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant MusicPlugin
participant ElementStyleResolver
participant LEDMatrixDisplay
Config->>MusicPlugin: Provide layout_mode and font configuration
MusicPlugin->>ElementStyleResolver: Resolve schema-aware element styles
MusicPlugin->>MusicPlugin: Calculate adaptive font sizes and Y positions
MusicPlugin->>LEDMatrixDisplay: Render title, artist, and album rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 | 43 |
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codacy (F401): media_row was imported from src.adaptive_layout but never used -- only FontStep and LADDER_ARCADE are referenced in this file.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/ledmatrix-music/test_adaptive_layout_mode.py (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModule-wide
skipifaccidentally disables the fallback/classic-path tests on the exact cores they're meant to validate.Gating the whole file on
ADAPTIVE_AVAILABLEmeansTestClassicUntouched,TestElementStyleResolver, andtest_falls_back_to_classic_without_core_support(which specifically monkeypatchesADAPTIVE_AVAILABLE=Falseto test the no-support scenario) all get skipped on any core that genuinely lackssrc.adaptive_layout— precisely the environment where verifying "classic is untouched" and "falls back cleanly" matters most.♻️ Proposed fix: scope the skip to only the tests that need the real ladder
-pytestmark = pytest.mark.skipif( - not ADAPTIVE_AVAILABLE, - reason="core without src.adaptive_layout — adaptive mode falls back to classic", -) +_requires_adaptive_core = pytest.mark.skipif( + not ADAPTIVE_AVAILABLE, + reason="core without src.adaptive_layout — adaptive mode falls back to classic", +)Then apply
@_requires_adaptive_coreonly toTestLadderCrispnessand the adaptive-rendering tests inTestAdaptiveMode(excludingtest_falls_back_to_classic_without_core_support), leavingTestClassicUntouchedandTestElementStyleResolverunconditional.🤖 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 `@plugins/ledmatrix-music/test_adaptive_layout_mode.py` around lines 32 - 35, Remove the module-wide pytestmark skipif based on ADAPTIVE_AVAILABLE. Define or reuse the _requires_adaptive_core marker and apply it only to TestLadderCrispness and the adaptive-rendering tests in TestAdaptiveMode, excluding test_falls_back_to_classic_without_core_support; leave TestClassicUntouched and TestElementStyleResolver runnable on cores without adaptive support.
🤖 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 `@plugins/ledmatrix-music/manager.py`:
- Around line 268-287: Wrap the STYLE_AVAILABLE resolver path in the existing
font-loading fallback so exceptions from _get_element_style_resolver() or
resolver.style() do not abort construction. On failure, log the issue as
appropriate and continue into the legacy customization-based loading branch,
preserving its default font behavior.
- Around line 137-150: The adaptive layout state is initialized only during
__init__, so runtime configuration changes are ignored. Update the plugin’s
config-change handling to refresh self.layout_mode from the current
configuration and recompute self._adaptive, preserving the existing fallback
warning when adaptive mode is requested but ADAPTIVE_AVAILABLE is false.
In `@plugins/ledmatrix-music/manifest.json`:
- Line 4: Align the ledmatrix-music release metadata: set manifest.json's
top-level version to 1.1.0 to match the newest versions[] entry, keep the
existing top changelog entry unchanged, and update plugins.json's latest_version
to 1.1.0. Affected sites: plugins/ledmatrix-music/manifest.json lines 4 and
69-74 (the changelog site requires no direct change), and plugins.json lines
481-484.
---
Nitpick comments:
In `@plugins/ledmatrix-music/test_adaptive_layout_mode.py`:
- Around line 32-35: Remove the module-wide pytestmark skipif based on
ADAPTIVE_AVAILABLE. Define or reuse the _requires_adaptive_core marker and apply
it only to TestLadderCrispness and the adaptive-rendering tests in
TestAdaptiveMode, excluding test_falls_back_to_classic_without_core_support;
leave TestClassicUntouched and TestElementStyleResolver runnable on cores
without adaptive support.
🪄 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
Run ID: fd36e6ee-0965-4d71-b765-864957f0a46c
📒 Files selected for processing (6)
plugins.jsonplugins/ledmatrix-music/README.mdplugins/ledmatrix-music/config_schema.jsonplugins/ledmatrix-music/manager.pyplugins/ledmatrix-music/manifest.jsonplugins/ledmatrix-music/test_adaptive_layout_mode.py
…ess, version alignment, test skip scoping - manager.py: wrap STYLE_AVAILABLE resolver calls in try/except so a runtime failure (not just an ImportError) falls through to the legacy font loader instead of aborting plugin construction. - manager.py: extract layout_mode/_adaptive computation into _apply_layout_mode(), called from both __init__ and a new on_config_change override, so a runtime layout_mode change via the web UI takes effect without a restart. - manifest.json/plugins.json: align version metadata to 1.1.0 (the newest versions[] changelog entry) instead of the stale 1.1.1. - test_adaptive_layout_mode.py: replace the module-wide ADAPTIVE_AVAILABLE skipif with a reusable _requires_adaptive_core marker scoped to only the tests that need real adaptive-core support, so classic-path tests, the element-style resolver tests, and the fallback-without-core-support test itself all remain runnable on cores without src.adaptive_layout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
1 participant
Summary
Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
Summary by CodeRabbit
New Features
classicoradaptivelayout modes.Documentation
Bug Fixes