Customizable nav bar, artwork toggles, and track-menu items (Tide-style UX) - #22
Open
KEZO555 wants to merge 4 commits into
Open
Customizable nav bar, artwork toggles, and track-menu items (Tide-style UX)#22KEZO555 wants to merge 4 commits into
KEZO555 wants to merge 4 commits into
Conversation
Let users choose which bottom-bar tabs appear and in what order, edited inline from Settings and persisted across launches. Settings stays pinned so you can't hide your way out of the app. The tabs are shared UI, so this applies to both the Spotify and TIDAL backends. - NavBarPreferences: persists tab order + visibility in plain SharedPreferences (phono_navbar), same shape as ThemePreferences, and publishes the order as a process-wide StateFlow so the shell and the Settings editor share one source of truth. - PhonoShell: feeds PhonoTabBar the persisted, filtered, ordered list via NavBarPreferences.visibleTabs(...), and re-homes the selected tab if it becomes hidden. - SettingsScreen: inline "Navigation bar" editor (toggle + up/down reorder), matching the existing inline-options pattern. Existing installs default to today's exact bar. Downloads gating is preserved (never shown on builds that don't support it). UI-only: no Room migration, no Rust/engine/auth changes. Co-Authored-By: Claude <noreply@anthropic.com>
Two UI-only, backend-agnostic settings under a new "Artwork" section: - "List thumbnails" hides the cover thumbnails in track/album/playlist rows. Gated inside PhonoMediaListItem, so it applies everywhere at once while still honoring callers that explicitly pass showImage = false. - "Now playing artwork" shows the album cover on the now-playing screen (from the existing PlaybackUiState.artUrl), which previously had no art. Backed by ArtworkPreferences — plain SharedPreferences like ThemePreferences, republished as process-wide StateFlows so lists and now-playing react live to changes in Settings. Defaults to on, so current behavior is unchanged (except now-playing, which gains art). No Rust/engine/auth changes. Co-Authored-By: Claude <noreply@anthropic.com>
Extends the existing track long-press menu with three items from Tide. All three resolve what they need from the track uri at action time via PlaybackController.trackMetadataForUri (which returns albumId + artistIds), so no per-screen call sites change and the items work uniformly across every track list, for both Spotify and TIDAL: - Add To Queue -> resolves metadata, then addTrackToQueue. - Go To Album / Go To Artist -> resolve the id, then navigate via the same one-shot ContextMenuUiState pattern already used for the playlist picker (new navigateToAlbumId / navigateToArtistId, consumed in ContextMenuHost and routed to the album/artist overlays in PhonoShell). No engine/auth changes. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe application now persists configurable navigation and artwork preferences, applies those settings across navigation and media displays, adds track context-menu actions for queueing and album/artist navigation, provides long-press haptics, and optimizes library date parsing. ChangesUI navigation and media preferences
Track context-menu actions
Supporting UI behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AppViewModel
participant ContextMenuHost
participant PhonoShell
User->>AppViewModel: choose album or artist action
AppViewModel->>AppViewModel: resolve track metadata
AppViewModel-->>ContextMenuHost: publish navigation target
ContextMenuHost->>PhonoShell: invoke navigation callback
PhonoShell->>PhonoShell: open album or artist overlay
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
- List thumbnails: Albums, Liked Songs and Playlists rows now pass the real cover art_url (they hard-coded showImage=false before, so the List thumbnails toggle could never show anything). Cover art is now downsampled to the ~50dp thumbnail slot so scrolling stays smooth. - Now-playing: shrink the optional cover from 70% to 40% width so the play/skip transport controls always stay on screen on the LP3. - Performance: replace the per-item Instant.parse in buildLibraryDateIndex with a cheap yyyy-MM substring parse, removing the main-thread jank when switching to the Albums / Liked Songs tabs. - Long-press menu: add a LongPress haptic to tapWithLongPress so the context menu long-press has tactile feedback like Tide.
Owner
|
@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
🤖 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 `@app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt`:
- Around line 2104-2133: Update onContextMenuAction and the context-menu state
flow to capture a context/session generation before dismissing the menu, pass it
into each asynchronous metadata lookup, and verify it still matches before
applying navigation or queue side effects. Increment or replace the generation
when dismissContextMenu or resetSessionUiState clears the context, and cancel
any tracked context-menu jobs from the session reset path so stale actions
cannot complete.
In `@app/src/main/java/com/lightphone/spotify/ui/components/LibraryDateIndex.kt`:
- Around line 66-70: Update the fast-path parsing in the date-index function
containing the iso prefix check so it only handles fully validated dates whose
UTC grouping matches the existing Instant.parse normalization. Route
offset-bearing timestamps, malformed dates, and inputs that could shift across a
month boundary through the existing UTC fallback instead; add regression
coverage for the offset example and invalid-date cases.
In `@app/src/main/java/com/lightphone/spotify/ui/screens/SettingsScreen.kt`:
- Around line 220-254: Update NavBarOptions to accept the downloads capability
and skip the Downloads entry when downloads are unsupported, matching
PhonoShell’s visibleTabs behavior. Thread the existing caps.downloads value from
SettingsScreen into NavBarOptions, and filter or omit the unsupported route
before rendering and reordering so users cannot toggle an unavailable tab.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a09cab3c-1ce1-42c0-aa3e-fcb870c41760
📒 Files selected for processing (13)
app/src/main/java/com/lightphone/spotify/App.ktapp/src/main/java/com/lightphone/spotify/ui/AppViewModel.ktapp/src/main/java/com/lightphone/spotify/ui/components/ContextMenuHost.ktapp/src/main/java/com/lightphone/spotify/ui/components/LibraryDateIndex.ktapp/src/main/java/com/lightphone/spotify/ui/components/PhonoComponents.ktapp/src/main/java/com/lightphone/spotify/ui/light/ArtworkPreferences.ktapp/src/main/java/com/lightphone/spotify/ui/navigation/NavBarPreferences.ktapp/src/main/java/com/lightphone/spotify/ui/navigation/PhonoShell.ktapp/src/main/java/com/lightphone/spotify/ui/screens/AlbumsScreen.ktapp/src/main/java/com/lightphone/spotify/ui/screens/LikedSongsScreen.ktapp/src/main/java/com/lightphone/spotify/ui/screens/PlayingScreen.ktapp/src/main/java/com/lightphone/spotify/ui/screens/PlaylistsScreen.ktapp/src/main/java/com/lightphone/spotify/ui/screens/SettingsScreen.kt
Comment on lines
+2104
to
+2133
| ContextMenuAction.AddToQueue -> { | ||
| if (target !is ContextMenuTarget.Track) return | ||
| dismissContextMenu() | ||
| viewModelScope.launch { | ||
| val meta = controller.trackMetadataForUri(target.uri) ?: return@launch | ||
| addTrackToQueue(meta) | ||
| } | ||
| } | ||
| ContextMenuAction.AddToPlaylists -> { | ||
| if (target !is ContextMenuTarget.Track) return | ||
| dismissContextMenu() | ||
| _contextMenu.update { it.copy(navigateToPlaylistPickerUri = target.uri) } | ||
| } | ||
| ContextMenuAction.GoToAlbum -> { | ||
| if (target !is ContextMenuTarget.Track) return | ||
| dismissContextMenu() | ||
| viewModelScope.launch { | ||
| val albumId = controller.trackMetadataForUri(target.uri)?.albumId ?: return@launch | ||
| _contextMenu.update { it.copy(navigateToAlbumId = albumId) } | ||
| } | ||
| } | ||
| ContextMenuAction.GoToArtist -> { | ||
| if (target !is ContextMenuTarget.Track) return | ||
| dismissContextMenu() | ||
| viewModelScope.launch { | ||
| val artistId = controller.trackMetadataForUri(target.uri) | ||
| ?.artistIds?.firstOrNull() ?: return@launch | ||
| _contextMenu.update { it.copy(navigateToArtistId = artistId) } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section plus surrounding context.
if [ -f app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt ]; then
wc -l app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt
sed -n '2060,2155p' app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt | nl -ba -v2060
else
echo "File not found"
fd -a AppViewModel.kt .
fiRepository: jonathancaudill/phono
Length of output: 264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols: dismissContextMenu, contextMenu state, session/reset/job cancellation, contextMenu actions.
rg -n "dismissContextMenu|_contextMenu|contextMenu|navigateToPlaylistPickerUri|navigateToAlbumId|navigateToArtistId|addTrackToQueue|onSessionReset|cancelJobs|cancel.*job|CoroutineScope|addTrackToQueue|tracksMetadata|trackMetadataForUri|currentGeneration|sessionGeneration" app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt app/src/main/java/com/lightphone/spotify 2>/dev/null | head -n 200Repository: jonathancaudill/phono
Length of output: 24278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the UI actions handler and reset logic around the reported menu actions.
sed -n '85,520p' app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt | sed -n '1,220p'
printf '\n--- contextMenu action handler ---\n'
sed -n '2090,2160p' app/src/main/java/com/lightphone/spotify/ui/AppViewModel.ktRepository: jonathancaudill/phono
Length of output: 11671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session reset and cancellation helpers ---'
sed -n '450,660p' app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt
printf '\n%s\n' '--- current session generation guards in AppViewModel ---'
python3 - <<'PY'
from pathlib import Path
p = Path('app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt')
lines = p.read_text().splitlines()
for i,l in enumerate(lines, 1):
if 'sessionGeneration' in l or 'viewModelScope.launch' in l or 'tryCancel' in l or 'jobs' in l or 'jobCollection' in l or 'cancelChildren' in l or 'cancel' in l:
if 200 <= i <= 2160 or 440 <= i <= 650:
print(f'{i}: {l}')
PYRepository: jonathancaudill/phono
Length of output: 14794
Guard asynchronous context menu actions against stale targets.
onContextMenuAction(...) captures _contextMenu.value.target and launch async metadata lookups that still run after dismissContextMenu() or after resetSessionUiState() clears the UI. Capture a context/session generation at action start, include it in each launch, reject the side effect when it mismatches, and cancel context menu jobs from the session reset path.
🤖 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 `@app/src/main/java/com/lightphone/spotify/ui/AppViewModel.kt` around lines
2104 - 2133, Update onContextMenuAction and the context-menu state flow to
capture a context/session generation before dismissing the menu, pass it into
each asynchronous metadata lookup, and verify it still matches before applying
navigation or queue side effects. Increment or replace the generation when
dismissContextMenu or resetSessionUiState clears the context, and cancel any
tracked context-menu jobs from the session reset path so stale actions cannot
complete.
Comment on lines
+66
to
+70
| if (iso.length >= 10 && iso[4] == '-' && iso[7] == '-') { | ||
| val year = iso.substring(0, 4).toIntOrNull() | ||
| val month = iso.substring(5, 7).toIntOrNull() | ||
| if (year != null && month != null && month in 1..12) { | ||
| return YearMonth.of(year, month) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve full date validation and UTC semantics in the fast path.
Lines [66-70] accept any yyyy-MM-shaped prefix, including invalid dates, and bypass UTC normalization. For example, 2023-05-01T00:30:00+02:00 previously grouped as April after conversion to UTC, but now returns May. Restrict the optimization to fully validated UTC timestamps, or retain the Instant.parse fallback for offset formats and malformed dates; add regression tests for both cases.
🤖 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 `@app/src/main/java/com/lightphone/spotify/ui/components/LibraryDateIndex.kt`
around lines 66 - 70, Update the fast-path parsing in the date-index function
containing the iso prefix check so it only handles fully validated dates whose
UTC grouping matches the existing Instant.parse normalization. Route
offset-bearing timestamps, malformed dates, and inputs that could shift across a
month boundary through the existing UTC fallback instead; add regression
coverage for the offset example and invalid-date cases.
Comment on lines
+220
to
+254
| @Composable | ||
| private fun NavBarOptions() { | ||
| val context = LocalContext.current | ||
| val prefs = remember(context) { NavBarPreferences(context) } | ||
| val order by NavBarPreferences.order.collectAsState() | ||
| order.forEachIndexed { i, pref -> | ||
| val tab = PhonoTab.entries.firstOrNull { it.route == pref.route } | ||
| ?: return@forEachIndexed | ||
| val locked = pref.route in NavBarPreferences.LOCKED | ||
| NavBarEditorRow( | ||
| label = tab.label, | ||
| enabled = pref.enabled, | ||
| locked = locked, | ||
| canMoveUp = i > 0, | ||
| canMoveDown = i < order.lastIndex, | ||
| onToggle = { | ||
| if (!locked) { | ||
| prefs.setOrder( | ||
| order.map { | ||
| if (it.route == pref.route) it.copy(enabled = !it.enabled) else it | ||
| }, | ||
| ) | ||
| } | ||
| }, | ||
| onMove = { delta -> | ||
| val to = i + delta | ||
| if (to in order.indices) { | ||
| val reordered = order.toMutableList() | ||
| reordered.add(to, reordered.removeAt(i)) | ||
| prefs.setOrder(reordered) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nav-bar editor exposes "Downloads" toggle even when the build/backend doesn't support downloads.
order always includes the Downloads route because NavBarPreferences.DEFAULT is built with includeDownloads = true. NavBarOptions() renders every entry unconditionally, but PhonoShell computes tabs via NavBarPreferences.visibleTabs(phonoTabs(includeDownloads = vm.downloadsSupported)), which silently drops Downloads for unsupported builds. Users on such builds/backends can see and toggle a "Downloads" nav entry that never actually appears — a confusing dead control. SettingsScreen already has caps.downloads in scope a few lines below (used for "Download quality"), so it can be threaded through here.
🐛 Proposed fix to filter unsupported tabs from the editor
- SectionLabel("Navigation bar")
- NavBarOptions()
+ SectionLabel("Navigation bar")
+ NavBarOptions(downloadsSupported = caps.downloads) `@Composable`
-private fun NavBarOptions() {
+private fun NavBarOptions(downloadsSupported: Boolean) {
val context = LocalContext.current
val prefs = remember(context) { NavBarPreferences(context) }
val order by NavBarPreferences.order.collectAsState()
order.forEachIndexed { i, pref ->
+ if (pref.route == PhonoTab.Downloads.route && !downloadsSupported) return@forEachIndexed
val tab = PhonoTab.entries.firstOrNull { it.route == pref.route }
?: return@forEachIndexed📝 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
| @Composable | |
| private fun NavBarOptions() { | |
| val context = LocalContext.current | |
| val prefs = remember(context) { NavBarPreferences(context) } | |
| val order by NavBarPreferences.order.collectAsState() | |
| order.forEachIndexed { i, pref -> | |
| val tab = PhonoTab.entries.firstOrNull { it.route == pref.route } | |
| ?: return@forEachIndexed | |
| val locked = pref.route in NavBarPreferences.LOCKED | |
| NavBarEditorRow( | |
| label = tab.label, | |
| enabled = pref.enabled, | |
| locked = locked, | |
| canMoveUp = i > 0, | |
| canMoveDown = i < order.lastIndex, | |
| onToggle = { | |
| if (!locked) { | |
| prefs.setOrder( | |
| order.map { | |
| if (it.route == pref.route) it.copy(enabled = !it.enabled) else it | |
| }, | |
| ) | |
| } | |
| }, | |
| onMove = { delta -> | |
| val to = i + delta | |
| if (to in order.indices) { | |
| val reordered = order.toMutableList() | |
| reordered.add(to, reordered.removeAt(i)) | |
| prefs.setOrder(reordered) | |
| } | |
| }, | |
| ) | |
| } | |
| } | |
| `@Composable` | |
| private fun NavBarOptions(downloadsSupported: Boolean) { | |
| val context = LocalContext.current | |
| val prefs = remember(context) { NavBarPreferences(context) } | |
| val order by NavBarPreferences.order.collectAsState() | |
| order.forEachIndexed { i, pref -> | |
| if (pref.route == PhonoTab.Downloads.route && !downloadsSupported) return@forEachIndexed | |
| val tab = PhonoTab.entries.firstOrNull { it.route == pref.route } | |
| ?: return@forEachIndexed | |
| val locked = pref.route in NavBarPreferences.LOCKED | |
| NavBarEditorRow( | |
| label = tab.label, | |
| enabled = pref.enabled, | |
| locked = locked, | |
| canMoveUp = i > 0, | |
| canMoveDown = i < order.lastIndex, | |
| onToggle = { | |
| if (!locked) { | |
| prefs.setOrder( | |
| order.map { | |
| if (it.route == pref.route) it.copy(enabled = !it.enabled) else it | |
| }, | |
| ) | |
| } | |
| }, | |
| onMove = { delta -> | |
| val to = i + delta | |
| if (to in order.indices) { | |
| val reordered = order.toMutableList() | |
| reordered.add(to, reordered.removeAt(i)) | |
| prefs.setOrder(reordered) | |
| } | |
| }, | |
| ) | |
| } | |
| } |
🤖 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 `@app/src/main/java/com/lightphone/spotify/ui/screens/SettingsScreen.kt` around
lines 220 - 254, Update NavBarOptions to accept the downloads capability and
skip the Downloads entry when downloads are unsupported, matching PhonoShell’s
visibleTabs behavior. Thread the existing caps.downloads value from
SettingsScreen into NavBarOptions, and filter or omit the unsupported route
before rendering and reordering so users cannot toggle an unavailable tab.
Owner
|
I need to take a look at the customizable navbar. I think technically speaking the current app is not compliant with the SDK's nav bar requirements? This will require some creative rethinking. Customizability isn't a bad idea though. |
Owner
|
One thing to note is that the current path being used for customizable navbar AND artwork is LocalContext, and I'm fairly sure the SDK blocks that as well (unless they've added it since I last checked) |
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
2 participants
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.
Three small, self-contained UI additions, plus follow-up fixes from on-device testing on the Light Phone III. All backend-agnostic (they apply to both Spotify and TIDAL).
1. Customizable navigation bar
Show/hide and reorder the bottom tabs, edited inline in Settings, persisted via a new
NavBarPreferences(sameSharedPreferencesshape asThemePreferences, republished as a process-wideStateFlow).PhonoTabBaris fed the persisted/filtered/ordered list; the selected tab re-homes if it becomes hidden. Settings stays pinned so you can't hide your way out.PhonoTab/PhonoTabBar/PhonoShellViewModelare untouched.2. Artwork show/hide toggles
A new "Artwork" settings section with two toggles, backed by
ArtworkPreferences:PhonoMediaListItem. Rows pass their realart_url, and covers are decoded down to the ~50dp thumbnail slot so scrolling stays smooth.PlaybackUiState.artUrl), which previously showed none. Sized compactly so the transport controls stay on-screen on the LP3.3. Track long-press menu items
Adds Add To Queue, Go To Album, and Go To Artist to the existing long-press track context menu. Each resolves what it needs from the track uri at action time via
PlaybackController.trackMetadataForUri(which returnsalbumId+artistIds), so no per-screen call sites change and the items work uniformly across every track list. Navigation reuses the same one-shotContextMenuUiStatepattern already used for the playlist picker (newnavigateToAlbumId/navigateToArtistId, consumed inContextMenuHost, routed to the album/artist overlays inPhonoShell). The long-press now also fires aLongPresshaptic for tactile feedback.Performance
buildLibraryDateIndexno longer runs an allocating, exception-guardedInstant.parseper item on the main thread during composition; it parses the leadingyyyy-MMof the ISO timestamp by substring instead. Removes the jank when switching to the Albums / Liked Songs tabs on-device.Notes
Summary by CodeRabbit
New Features
Bug Fixes