fix(jellyfin-now-playing): size the progress bar to the text, not the panel - #232
Conversation
… panel
_render_now_playing gave the text area every remaining column and then drew the
progress bar across all of it:
text_w = width - text_x - 1
bar_x2 = text_x + text_w - 1
So the rendered frame was full-width whatever the title length. On a 512px panel
a short episode name left a bar stretching across the display, and because a bar
is drawn pixels rather than blank space, Vegas mode could not trim it back — the
plugin was contributing a full screen width per pass.
The bar is now sized to the widest of the title and subtitle, with a 24px floor
so a very short title still reads as a progress indicator. That also makes the
remainder genuinely blank, so a ticker can reclaim it. A title long enough to be
marqueed measures wider than the area and still fills the bar, which is correct.
Set progress_bar_match_text false for the previous full-width bar.
The safety harness cannot cover this: without a reachable Jellyfin server there
is no session, so the now-playing frame is never rendered and the harness only
ever sees the "Nothing Playing" screen. Hence the focused unit tests in
test_content_width.py, which exercise the sizing rule directly.
Harness passes at all sizes; module collisions clean.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Warning Review limit reached
Next review available in: 16 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. 📝 WalkthroughWalkthroughThe Jellyfin Now Playing plugin adds configurable progress bar sizing based on the widest title or subtitle, enforces width bounds, updates version metadata to 1.1.0, and adds focused tests for the new behavior. ChangesJellyfin progress bar sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 | 26 |
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/jellyfin-now-playing/manager.py (2)
545-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
int()cast.Ruff RUF046 correctly identifies that
round()with one argument already returns an integer; usefill_w = round(...)without changing behavior.🤖 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/jellyfin-now-playing/manager.py` at line 545, Update the fill_w assignment in the progress-bar calculation to remove the redundant int() cast and assign the result of round((bar_w - 1) * progress) directly, preserving the existing behavior.Source: Linters/SAST tools
553-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the measurement fallback exception.
The fallback is useful, but
except Exceptioncan hide unrelated programming errors and silently force full-width bars. Catch a dedicated or known measurement failure, or have_text_width()return an explicit failure sentinel while preserving the existing fallback test.🤖 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/jellyfin-now-playing/manager.py` around lines 553 - 585, Narrow the exception handling in _content_width so only the known text-measurement failure is caught, rather than masking unrelated programming errors. Use the dedicated measurement exception from _text_width, or update _text_width to return its existing failure sentinel and preserve the fallback to available for that sentinel.Source: Linters/SAST tools
🤖 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/jellyfin-now-playing/config_schema.json`:
- Line 51: Replace “drawn pixels” with “drawn as pixels” in the description at
plugins/jellyfin-now-playing/config_schema.json:51-51 and apply the same wording
correction at plugins/jellyfin-now-playing/CHANGELOG.md:9-9.
In `@plugins/jellyfin-now-playing/test_content_width.py`:
- Around line 19-42: Update the test module setup around make_plugin to load
this plugin’s manager.py via importlib under a unique module name/path instead
of importing bare manager. Replace the permanent sys.modules assignments for
src, src.plugin_system, and src.plugin_system.base_plugin with
patch.dict(sys.modules, ..., clear=False) scoped only around that import, then
obtain JellyfinNowPlayingPlugin from the uniquely loaded module.
---
Nitpick comments:
In `@plugins/jellyfin-now-playing/manager.py`:
- Line 545: Update the fill_w assignment in the progress-bar calculation to
remove the redundant int() cast and assign the result of round((bar_w - 1) *
progress) directly, preserving the existing behavior.
- Around line 553-585: Narrow the exception handling in _content_width so only
the known text-measurement failure is caught, rather than masking unrelated
programming errors. Use the dedicated measurement exception from _text_width, or
update _text_width to return its existing failure sentinel and preserve the
fallback to available for that sentinel.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 0267d86b-84d0-42ba-8e3e-a2662d8ad091
📒 Files selected for processing (6)
plugins.jsonplugins/jellyfin-now-playing/CHANGELOG.mdplugins/jellyfin-now-playing/config_schema.jsonplugins/jellyfin-now-playing/manager.pyplugins/jellyfin-now-playing/manifest.jsonplugins/jellyfin-now-playing/test_content_width.py
| "x-advanced": true, | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Size the progress bar to the widest of the title and subtitle instead of stretching it across the whole text area. On a wide panel a short title otherwise leaves a bar spanning the display, and because a bar is drawn pixels it cannot be trimmed back in Vegas scroll mode. A title long enough to scroll still fills the bar. Turn off for the original full-width bar." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the duplicated “drawn pixels” wording.
plugins/jellyfin-now-playing/config_schema.json#L51-L51: replace “drawn pixels” with “drawn as pixels”.plugins/jellyfin-now-playing/CHANGELOG.md#L9-L9: apply the same wording correction.
📍 Affects 2 files
plugins/jellyfin-now-playing/config_schema.json#L51-L51(this comment)plugins/jellyfin-now-playing/CHANGELOG.md#L9-L9
🤖 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/jellyfin-now-playing/config_schema.json` at line 51, Replace “drawn
pixels” with “drawn as pixels” in the description at
plugins/jellyfin-now-playing/config_schema.json:51-51 and apply the same wording
correction at plugins/jellyfin-now-playing/CHANGELOG.md:9-9.
| if 'src' not in sys.modules: | ||
| src = types.ModuleType('src') | ||
| plugin_system = types.ModuleType('src.plugin_system') | ||
| base_plugin = types.ModuleType('src.plugin_system.base_plugin') | ||
|
|
||
| class _BasePlugin: | ||
| def __init__(self, *args, **kwargs): | ||
| pass | ||
|
|
||
| base_plugin.BasePlugin = _BasePlugin | ||
| plugin_system.base_plugin = base_plugin | ||
| src.plugin_system = plugin_system | ||
| sys.modules['src'] = src | ||
| sys.modules['src.plugin_system'] = plugin_system | ||
| sys.modules['src.plugin_system.base_plugin'] = base_plugin | ||
|
|
||
| TEXT_AREA = 300 | ||
| TITLE_FONT = object() | ||
| SUBTITLE_FONT = object() | ||
|
|
||
|
|
||
| def make_plugin(title='', subtitle='', config=None, px_per_char=6, fail=False): | ||
| """A plugin shell with a no-op __init__; only the sizing logic is exercised.""" | ||
| from manager import JellyfinNowPlayingPlugin |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Isolate the plugin import from the global module cache.
Line 42 can import another plugin’s already-cached bare manager module, while Lines 19-33 permanently replace src modules for the rest of the test process. Load this plugin’s manager.py by a unique importlib module name/path and scope the fake src entries with patch.dict(sys.modules, ...) only during that import.
Otherwise collection order across plugins can make these tests exercise the wrong class or contaminate later core-plugin tests.
As per coding guidelines, top-level Python modules are loaded by bare name; tests must not rely on that shared module name across plugins.
🤖 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/jellyfin-now-playing/test_content_width.py` around lines 19 - 42,
Update the test module setup around make_plugin to load this plugin’s manager.py
via importlib under a unique module name/path instead of importing bare manager.
Replace the permanent sys.modules assignments for src, src.plugin_system, and
src.plugin_system.base_plugin with patch.dict(sys.modules, ..., clear=False)
scoped only around that import, then obtain JellyfinNowPlayingPlugin from the
uniquely loaded module.
Source: Coding guidelines
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
2 participants
Problem
The plugin occupied a full screen width in Vegas scroll mode regardless of how short the title was.
_render_now_playinggave the text area every remaining column, then drew the progress bar across all of it:On a 512px panel a short episode name left a bar stretching across the display. And because a bar is drawn pixels rather than blank space, a ticker cannot trim it back — the plugin was contributing a full 512px per pass with most of it carrying no information.
Fix
The bar is now sized to the widest of the title and subtitle, with a 24px floor so a very short title still reads as a progress indicator.
The knock-on effect matters as much as the bar itself: the remainder becomes genuinely blank, so Vegas mode's trimming can reclaim it. On the test rig the segment went from ~512px to 106px.
Three details:
Set
progress_bar_match_textfalse for the previous behaviour.Testing
The safety harness cannot cover this, which is worth stating plainly: without a reachable Jellyfin server there is no session, so
_render_now_playingnever runs and the harness only ever sees the "Nothing Playing" screen. Relying on it here would give false confidence.So this ships with focused unit tests in
test_content_width.py(16 cases): widest-line selection, the floor, the area clamp, the marquee case, a missing session, empty strings, the config toggle, and measurement failure.check_module_collisions.py: OK across 41 plugins.Companion lever
Core-side, ChuckBuilds/LEDMatrix#423 adds a per-plugin
vegas_width_pctso a plugin can be asked to render at a fraction of the panel. Setting that to 45 for this plugin is complementary: it makes the layout compact, while this change stops the bar re-filling whatever width it is given.🤖 Generated with Claude Code
https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Summary by CodeRabbit
New Features
Bug Fixes
Chores