Selecting a song on the search page navigates to a song's album with the song focused#1661
Selecting a song on the search page navigates to a song's album with the song focused#1661StefanBS wants to merge 1 commit into
Conversation
Selecting a song from search now resolves to its album's detail page, scrolled to and focusing that song, instead of dead-ending on an "unsupported item type" page. Implemented in the shared BaseItem.destination() rather than in search specifically, so any caller that navigates to a song gets the same behavior. An album-less song has no album page to show, so navigating to one plays it directly instead: NavigationManager.navigateTo() intercepts a bare AUDIO destination and hands it to a new MusicService.playSong(), rather than pushing a page. Handling this in the single navigation funnel means every caller gets it for free with no loading-screen flash. Adds BaseItemDestinationTest covering the new destination() logic.
damontecres
left a comment
damontecres
left a comment
There was a problem hiding this comment.
Thanks for the contribution! However, this PR needs some work.
How common are songs without albums? I think it would be better to open them to a placeholder page instead of playing directly. I think a user would not expect it to play immediately and it would disrupt the music queue they are building.
| @Inject | ||
| constructor() { | ||
| constructor( | ||
| private val musicService: MusicService, |
There was a problem hiding this comment.
The NavigationManager should not have any dependencies.
Calling musicService.playSong should be performed by a ViewModel instead
| fun playSong(itemId: UUID) { | ||
| defaultScope.launch { |
There was a problem hiding this comment.
Make this suspend instead and have ViewModels handle the scope
| val backHandlerActive by remember { | ||
| derivedStateOf { | ||
| listState.firstVisibleItemIndex > itemsBefore | ||
| // Deep-linked opens already start scrolled to the song, so let back pop the |
There was a problem hiding this comment.
I don't think this behavior would be what the user expects since it changes based on how the user opened the page. The back button behavior should be consistent and predictable.
| focusRequesters.getOrNull(position.row)?.tryRequestFocus() | ||
| } | ||
| val onClickItem = { index: Int, item: BaseItem -> | ||
| val onClickItem = { _: Int, item: BaseItem -> |
| val itemsBefore = 2 | ||
| val initialSongIndex = state.initialSongIndex | ||
| val hasInitialSong = initialSongIndex >= 0 | ||
| var position by rememberPosition( | ||
| row = if (hasInitialSong) SONG_ROW else 0, | ||
| column = initialSongIndex.coerceAtLeast(0), | ||
| ) | ||
|
|
||
| LaunchedEffect(Unit) { viewModel.updateBackDrop() } |
There was a problem hiding this comment.
Not quite sure what all of this was moved around
damontecres
added
enhancement
2 participants
Description
Selecting a song from search now resolves to its album's detail page, scrolled to and focusing that song, instead of sending you to an "unsupported item type" page. Implemented in the shared BaseItem.destination() rather than in search specifically, so any caller that navigates to a song gets the same behavior.
A song with no albumId plays directly while remaining in the search results page: NavigationManager.navigateTo() intercepts a bare AUDIO destination and hands it to a new MusicService.playSong(). Handling this in the single navigation funnel means every caller gets it for free with no loading-screen flash.
Adds BaseItemDestinationTest covering the new destination() logic.
Related issues
#1656
Testing
Device: Sony Bravia 4K VH21
Tests on a search results page, song with albumId:
Tested with both songs at the start of an album and songs deep into a 100+ songs album.
Tests on a search results page, song with no albumId:
AI or LLM usage
I used Cursor and Claude Code for planning, implementation and code review.