Compact window: Library ↔ Playlist toggle - #417
Merged
Merged
Conversation
The Compact window can now switch its content region between the library browser and the current playlist via a toggle, so users can view and edit the playlist without opening the full Playlist window. Modern/Metal (ModernLibraryBrowserView): - New persisted `compactContentMode` (shared UserDefaults key across UI families). - Library | Playlist toggle rendered as a bottom footer; content region bottoms out at a single `contentRegionBottomY` lever (status bar + footer) so the list, alphabet index, hit-testing, and scroll clamping all reserve footer space. - Embedded `ModernPlaylistView` fills the content region in Playlist mode; the library skips its content chrome there and the playlist draws transparently so the shared Cava/art backdrop shows through it like the library list. ModernPlaylistView: `isEmbedded` flag hides the standalone title bar/close button and disables window drag/undock (six title-bar sites), and makes the view transparent when embedded; first-responder handoff enables keyboard nav. Classic (PlexBrowser compact): managed in the container/controller rather than editing PlexBrowserView's internals — classic compact has no backdrop, so the embedded playlist stays opaque. New `ClassicCompactFooterView` draws the toggle in authentic classic skin style; `ClassicCompactContainerView` lays out footer + browser/playlist (browser XOR playlist by mode). `PlaylistView` gains `isEmbedded` (non-draggable, no close) and hides its title bar via frame-extension behind the player bar (the browser's gap-free mechanism), sized to its own scale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SavedTrack doesn't persist mediaType, and restorePlaylistState rebuilt local tracks with the Track initializer's default (.audio) — so a video left in the playlist from a previous session came back as audio and playTrack/loadTrack routed it to AVAudioFile, which can't decode the video container (fails with kAudioFileInvalidFile 'dta?'). Re-derive mediaType from the file extension on restore (extension-only check, no file I/O) so videos open in the video player. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCompact Window now switches between the library and an embedded playlist with persisted state and skin support. Restored local video files now retain their video media type. ChangesCompact Window playlist switching
Local video state restoration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ClassicCompactFooterView
participant PlexBrowserWindowController
participant ModernLibraryBrowserView
participant ModernPlaylistView
User->>ClassicCompactFooterView: select Library or Playlist
ClassicCompactFooterView->>PlexBrowserWindowController: report selected mode
PlexBrowserWindowController->>ModernLibraryBrowserView: update compact content
PlexBrowserWindowController->>ModernPlaylistView: toggle visibility and focus
ModernLibraryBrowserView->>ModernPlaylistView: update embedded frame
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Sources/NullPlayer/App/AppStateManager.swift (1)
999-1003: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for restored media types.
Test representative local video and audio extensions. Assert that restoration creates tracks with
mediaType == .videoandmediaType == .audiorespectively. This protects the fix from future regressions.🤖 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 `@Sources/NullPlayer/App/AppStateManager.swift` around lines 999 - 1003, Add a regression test covering restoration through AppStateManager for representative local video and audio file extensions. Assert restored video tracks have mediaType .video and restored audio tracks have mediaType .audio, exercising the AudioFileValidator.isVideoFile-based media type derivation.
🤖 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
`@Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift`:
- Around line 382-384: Remove the duplicate nested CompactContentMode
declaration in ModernLibraryBrowserView, retaining a single enum definition with
its existing library and queue cases.
- Around line 1204-1218: Synchronize compact playlist views after layout: in
Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift:1204-1218,
call queue.reloadData() after the final queue.frame assignment only when queue
is newly created; in
Sources/NullPlayer/Windows/PlexBrowser/PlexBrowserWindowController.swift:203-210,
call playlist.reloadData() after container.layoutChildren() and before showing
the selected compact content.
- Around line 1360-1364: Update the earlier sbRect.contains(dirtyRect) fast path
to also require showLibraryContent before calling drawServerBar. This prevents
server-bar-only redraws in compact queue mode while preserving the existing fast
path for other library content modes.
In `@Sources/NullPlayer/Windows/Playlist/PlaylistView.swift`:
- Around line 46-49: Update the embeddedTopInset computed property to return
Layout.titleBarHeight multiplied by scaleFactor only when isEmbedded is true;
otherwise return 0 as the safe standalone-mode default.
In `@Sources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swift`:
- Around line 137-146: Update ClassicCompactFooterView.mouseDown and the
corresponding footer implementation in
Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift:4017-4030
to expose separate Library and Playlist accessibility buttons with descriptive
labels, selected state, and press actions matching each segment. Add keyboard
activation for both controls while preserving the existing pointer selection
behavior; both listed sites require equivalent accessibility and keyboard
support.
---
Nitpick comments:
In `@Sources/NullPlayer/App/AppStateManager.swift`:
- Around line 999-1003: Add a regression test covering restoration through
AppStateManager for representative local video and audio file extensions. Assert
restored video tracks have mediaType .video and restored audio tracks have
mediaType .audio, exercising the AudioFileValidator.isVideoFile-based media type
derivation.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c1e7f0e-a0e2-4009-8d61-40d9d48c4454
📒 Files selected for processing (8)
CHANGELOG.mdSources/NullPlayer/App/AppStateManager.swiftSources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swiftSources/NullPlayer/Windows/ModernPlaylist/ModernPlaylistView.swiftSources/NullPlayer/Windows/Playlist/PlaylistView.swiftSources/NullPlayer/Windows/PlexBrowser/ClassicCompactContainerView.swiftSources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swiftSources/NullPlayer/Windows/PlexBrowser/PlexBrowserWindowController.swift
Comment on lines
+382
to
+384
| /// Content shown below the compact player bar: the library browser or the play queue. | ||
| /// Only consulted when `compactMode == true`; the full library window is unaffected. | ||
| enum CompactContentMode: Int { case library = 0, queue = 1 } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate CompactContentMode declaration.
Line 384 redeclares the nested enum from Line 382. Swift rejects this invalid redeclaration, so the target cannot build. Keep one declaration.
🤖 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
`@Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift`
around lines 382 - 384, Remove the duplicate nested CompactContentMode
declaration in ModernLibraryBrowserView, retaining a single enum definition with
its existing library and queue cases.
Comment on lines
+137
to
+146
| override func mouseDown(with event: NSEvent) { | ||
| let point = convert(event.locationInWindow, from: nil) | ||
| let scale = nativeScale | ||
| guard scale > 0 else { return } | ||
| // View point (bottom-left origin) → native units (top-left origin). | ||
| let nativeX = point.x / scale | ||
| let segments = segmentRects() | ||
| // Split at the divider so the entire footer width is clickable (incl. the thin margins). | ||
| onSelect?(nativeX >= segments.playlist.minX) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Expose the compact selector to keyboard and assistive technologies. Both Library/Playlist selectors are custom-drawn and pointer-only. VoiceOver users and keyboard-only users cannot switch compact content.
Sources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swift#L137-L146: expose Library and Playlist as accessible buttons with labels, selected state, and press actions. Add keyboard activation.Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift#L4017-L4030: expose the two footer segments with equivalent accessibility and keyboard actions.
📍 Affects 2 files
Sources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swift#L137-L146(this comment)Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift#L4017-L4030
🤖 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 `@Sources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swift` around
lines 137 - 146, Update ClassicCompactFooterView.mouseDown and the corresponding
footer implementation in
Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift:4017-4030
to expose separate Library and Playlist accessibility buttons with descriptive
labels, selected state, and press actions matching each segment. Add keyboard
activation for both controls while preserving the existing pointer selection
behavior; both listed sites require equivalent accessibility and keyboard
support.
Owner
Author
|
Addressed the compact playlist review findings in 8f81f62:\n\n- Forward playlist mutations to both standalone and embedded compact playlist views.\n- Initialize embedded Classic and Modern/Metal playlists after final layout so current-track selection and scrolling are correct on first reveal.\n- Restore first-responder focus when Modern/Metal reopens directly in Playlist mode.\n- Suppress the Modern/Metal server-bar fast path while the transparent embedded playlist is visible.\n- Return a safe zero classic playlist inset outside embedded mode and remove the redundant playlist-color fallback.\n\nValidation:\n- |
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
`@Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserWindowController.swift`:
- Around line 154-156: Update
ModernLibraryBrowserWindowController.reloadCompactPlaylist() to return
immediately unless the controller is in compact mode, using the applicable mode
check (isCompactMode or the project’s established isModernUIEnabled check)
before calling browserView.reloadCompactPlaylist().
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db02fff6-482f-4c42-af7f-6682a6c9b75f
📒 Files selected for processing (8)
Sources/NullPlayer/App/AppDelegate.swiftSources/NullPlayer/App/WindowManager.swiftSources/NullPlayer/Windows/CompactMode/CompactModeWindowController.swiftSources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swiftSources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserWindowController.swiftSources/NullPlayer/Windows/Playlist/PlaylistView.swiftSources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swiftSources/NullPlayer/Windows/PlexBrowser/PlexBrowserWindowController.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/NullPlayer/Windows/PlexBrowser/ClassicCompactFooterView.swift
- Sources/NullPlayer/Windows/PlexBrowser/PlexBrowserWindowController.swift
- Sources/NullPlayer/Windows/Playlist/PlaylistView.swift
- Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserView.swift
Comment on lines
+154
to
+156
| func reloadCompactPlaylist() { | ||
| browserView.reloadCompactPlaylist() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the compact-only reload action.
ModernLibraryBrowserWindowController is also created for the normal library window. Add guard isCompactMode else { return } before forwarding reloadCompactPlaylist(). The downstream optional check is a safe fallback, but it does not enforce the mode boundary at this action layer.
As per coding guidelines, mode-specific action functions must use isModernUIEnabled or the applicable mode check.
Proposed fix
func reloadCompactPlaylist() {
+ guard isCompactMode else { return }
browserView.reloadCompactPlaylist()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func reloadCompactPlaylist() { | |
| browserView.reloadCompactPlaylist() | |
| } | |
| func reloadCompactPlaylist() { | |
| guard isCompactMode else { return } | |
| browserView.reloadCompactPlaylist() | |
| } |
🤖 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
`@Sources/NullPlayer/Windows/ModernLibraryBrowser/ModernLibraryBrowserWindowController.swift`
around lines 154 - 156, Update
ModernLibraryBrowserWindowController.reloadCompactPlaylist() to return
immediately unless the controller is in compact mode, using the applicable mode
check (isCompactMode or the project’s established isModernUIEnabled check)
before calling browserView.reloadCompactPlaylist().
Source: Coding guidelines
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
1 participant
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
Adds a Library | Playlist toggle to the Compact window so users can view and edit the current playlist without opening the full Playlist window. Available in all three skin families (Modern, Metal, Classic). Also fixes a pre-existing bug found while testing.
Compact window toggle
compactContentModekey across UI families).contentRegionBottomYlever (status bar + footer), mirroringtopChromeBottomYfor the top, so the list/alphabet/hit-testing/scroll all reserve footer space.PlexBrowserView's internals. NewClassicCompactFooterViewrenders the toggle in authentic classic skin style;ClassicCompactContainerViewlays out footer + browser/playlist (one visible at a time). Classic compact has no backdrop, so the embedded playlist stays opaque.isEmbeddedflag (hides standalone title bar/close, disables window drag, routes body clicks to selection). The classic playlist hides its title bar via frame-extension behind the player bar (the browser's gap-free mechanism), sized to its own scale.Bug fix (pre-existing)
SavedTrackdoesn't persistmediaType, so a video left in the playlist from a previous session was rebuilt as an audio track and failed to decode (kAudioFileInvalidFile). Restore now re-derivesmediaTypefrom the file extension (no file I/O), so videos open in the video player.Testing
swift buildclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes