From c28271590eb68ff9ea501e43db6365891713214d Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sat, 26 Sep 2026 02:55:53 +0000 Subject: [PATCH] address feedback: optimistic star and menu polish - star/unstar updates the local row immediately and rolls back with a toast if the server rejects it, instead of waiting on the response - the menu collapses with an animation on dismiss - star/unstar keeps the menu open and toggles its item in place - the album/artist rows show the target's name instead of "View album"/"View artist" - the menu header shows the cover art thumbnail --- .../subtracks/data/repo/LibraryRepository.kt | 20 ++++- .../ui/components/ItemContextMenu.kt | 88 ++++++++++++++----- .../subtracks/ui/library/AlbumDetailScreen.kt | 4 +- .../com/subtracks/ui/library/AlbumsContent.kt | 2 +- .../ui/library/ArtistDetailScreen.kt | 4 +- .../subtracks/ui/library/ArtistsContent.kt | 2 +- .../ui/library/PlaylistDetailScreen.kt | 4 +- .../subtracks/ui/library/PlaylistsContent.kt | 2 +- .../com/subtracks/ui/library/SongsContent.kt | 2 +- .../subtracks/ui/playback/NowPlayingScreen.kt | 2 +- .../data/repo/LibraryRepositoryTest.kt | 34 ++++++- 11 files changed, 130 insertions(+), 34 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt index fb699076..525cf47e 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt @@ -18,6 +18,7 @@ import com.subtracks.data.prefs.SongSort import com.subtracks.data.prefs.StarredFilter import com.subtracks.data.source.StarType import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.first class LibraryRepository( private val db: SubtracksDatabase, @@ -249,11 +250,26 @@ class LibraryRepository( val sourceId = sourceRepository.activeSourceIdOnce() val source = sourceRepository.activeMusicSource() if (sourceId == null || source == null) return Result.failure(IllegalStateException("No active server")) + val previous = starredValue(sourceId, type, id) + updateStarred(sourceId, type, id, if (starred) System.currentTimeMillis() else null) return runCatching { source.setStar(type, id, starred) } - .onSuccess { updateStarred(sourceId, type, id, if (starred) System.currentTimeMillis() else null) } - .onFailure { showMessage("Could not update star") } + .onFailure { + updateStarred(sourceId, type, id, previous) + showMessage("Could not update star") + } } + private suspend fun starredValue( + sourceId: Long, + type: StarType, + id: String, + ): Long? = + when (type) { + StarType.Song -> song(sourceId, id).first()?.starred + StarType.Album -> album(sourceId, id).first()?.starred + StarType.Artist -> artist(sourceId, id).first()?.starred + } + private suspend fun updateStarred( sourceId: Long, type: StarType, diff --git a/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt b/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt index d0b73329..6642d90b 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt @@ -1,6 +1,9 @@ package com.subtracks.ui.components import androidx.compose.foundation.clickable +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.shape.CircleShape +import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material.icons.Icons import androidx.compose.material.icons.rounded.Album import androidx.compose.material.icons.rounded.Person @@ -15,12 +18,22 @@ import androidx.compose.material3.ListItemDefaults import androidx.compose.material3.MaterialTheme import androidx.compose.material3.ModalBottomSheet import androidx.compose.material3.Text +import androidx.compose.material3.rememberModalBottomSheetState import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.clip import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.vector.ImageVector import androidx.compose.ui.text.style.TextOverflow +import androidx.compose.ui.unit.dp +import com.subtracks.data.model.CoverArtRef import com.subtracks.data.source.StarType +import kotlinx.coroutines.launch import com.subtracks.data.model.Album as AlbumModel import com.subtracks.data.model.Artist as ArtistModel import com.subtracks.data.model.Playlist as PlaylistModel @@ -29,9 +42,11 @@ import com.subtracks.data.model.Song as SongModel sealed interface MenuTarget { val title: String val subtitle: String? + val coverArt: CoverArtRef? data class Song( val song: SongModel, + override val coverArt: CoverArtRef? = null, ) : MenuTarget { override val title: String get() = song.title @@ -40,6 +55,7 @@ sealed interface MenuTarget { data class Album( val album: AlbumModel, + override val coverArt: CoverArtRef? = null, ) : MenuTarget { override val title: String get() = album.name @@ -48,6 +64,7 @@ sealed interface MenuTarget { data class Artist( val artist: ArtistModel, + override val coverArt: CoverArtRef? = null, ) : MenuTarget { override val title: String get() = artist.name @@ -56,6 +73,7 @@ sealed interface MenuTarget { data class Playlist( val playlist: PlaylistModel, + override val coverArt: CoverArtRef? = null, ) : MenuTarget { override val title: String get() = playlist.name @@ -81,54 +99,64 @@ fun ItemContextMenu( actions: ItemActions, onDismiss: () -> Unit, ) { - ModalBottomSheet(onDismissRequest = onDismiss) { - val dismissThen: (() -> Unit) -> Unit = { action -> + val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true) + val scope = rememberCoroutineScope() + + fun dismiss(after: () -> Unit = {}) { + scope.launch { + sheetState.hide() + after() onDismiss() - action() } + } + + ModalBottomSheet( + onDismissRequest = { dismiss() }, + sheetState = sheetState, + ) { MenuHeader(target) when (target) { is MenuTarget.Song -> { actions.playSong?.let { play -> - MenuItem(Icons.Rounded.PlayArrow, "Play") { dismissThen { play(target.song) } } - } - MenuItem(starIcon(target.song.starred), starLabel(target.song.starred)) { - dismissThen { actions.setStar(StarType.Song, target.song.id, target.song.starred == null) } + MenuItem(Icons.Rounded.PlayArrow, "Play") { dismiss { play(target.song) } } } + StarItem(target.song.starred) { starring -> actions.setStar(StarType.Song, target.song.id, starring) } target.song.albumId?.let { albumId -> actions.viewAlbum?.let { view -> - MenuItem(Icons.Rounded.Album, "View album") { dismissThen { view(albumId) } } + MenuItem(Icons.Rounded.Album, target.song.album?.takeIf { it.isNotBlank() } ?: "Album") { + dismiss { view(albumId) } + } } } target.song.artistId?.let { artistId -> actions.viewArtist?.let { view -> - MenuItem(Icons.Rounded.Person, "View artist") { dismissThen { view(artistId) } } + MenuItem(Icons.Rounded.Person, target.song.artist?.takeIf { it.isNotBlank() } ?: "Artist") { + dismiss { view(artistId) } + } } } } is MenuTarget.Album -> { - MenuItem(Icons.Rounded.PlayArrow, "Play") { dismissThen { actions.playAlbum(target.album) } } - MenuItem(Icons.Rounded.Shuffle, "Shuffle") { dismissThen { actions.shuffleAlbum(target.album) } } - MenuItem(starIcon(target.album.starred), starLabel(target.album.starred)) { - dismissThen { actions.setStar(StarType.Album, target.album.id, target.album.starred == null) } - } + MenuItem(Icons.Rounded.PlayArrow, "Play") { dismiss { actions.playAlbum(target.album) } } + MenuItem(Icons.Rounded.Shuffle, "Shuffle") { dismiss { actions.shuffleAlbum(target.album) } } + StarItem(target.album.starred) { starring -> actions.setStar(StarType.Album, target.album.id, starring) } target.album.artistId?.let { artistId -> actions.viewArtist?.let { view -> - MenuItem(Icons.Rounded.Person, "View artist") { dismissThen { view(artistId) } } + MenuItem(Icons.Rounded.Person, target.album.albumArtist?.takeIf { it.isNotBlank() } ?: "Artist") { + dismiss { view(artistId) } + } } } } is MenuTarget.Artist -> { - MenuItem(starIcon(target.artist.starred), starLabel(target.artist.starred)) { - dismissThen { actions.setStar(StarType.Artist, target.artist.id, target.artist.starred == null) } - } + StarItem(target.artist.starred) { starring -> actions.setStar(StarType.Artist, target.artist.id, starring) } } is MenuTarget.Playlist -> { - MenuItem(Icons.Rounded.PlayArrow, "Play") { dismissThen { actions.playPlaylist(target.playlist) } } - MenuItem(Icons.Rounded.Shuffle, "Shuffle") { dismissThen { actions.shufflePlaylist(target.playlist) } } + MenuItem(Icons.Rounded.PlayArrow, "Play") { dismiss { actions.playPlaylist(target.playlist) } } + MenuItem(Icons.Rounded.Shuffle, "Shuffle") { dismiss { actions.shufflePlaylist(target.playlist) } } } } } @@ -155,10 +183,30 @@ private fun MenuHeader(target: MenuTarget) { ) } }, + leadingContent = { + CoverArt( + ref = target.coverArt, + name = target.title, + modifier = Modifier.size(48.dp).clip(if (target is MenuTarget.Artist) CircleShape else RoundedCornerShape(8.dp)), + ) + }, colors = ListItemDefaults.colors(containerColor = Color.Transparent), ) } +@Composable +private fun StarItem( + current: Long?, + onToggle: (Boolean) -> Unit, +) { + var starred by remember(current) { mutableStateOf(current) } + MenuItem(starIcon(starred), starLabel(starred)) { + val starring = starred == null + starred = if (starring) System.currentTimeMillis() else null + onToggle(starring) + } +} + @Composable private fun MenuItem( icon: ImageVector, diff --git a/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt index 25d03dce..09ecc7bb 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt @@ -77,7 +77,7 @@ fun AlbumDetailRoute( onSongLongClick = { menuTarget = it }, onShuffle = viewModel::shuffle, onPlay = viewModel::playAll, - onMore = { album?.let { menuTarget = MenuTarget.Album(it) } }, + onMore = { album?.let { menuTarget = MenuTarget.Album(it, viewModel.coverArt(it.coverArt, true)) } }, playingSongId = playback.item?.id.takeIf { context?.kind == QueueKind.Album && context.refId == albumId }, ) menuTarget?.let { target -> @@ -161,7 +161,7 @@ fun AlbumDetailScreen( modifier = rowModifier.combinedClickable( onClick = { onSongClick(index) }, - onLongClick = { onSongLongClick(MenuTarget.Song(song)) }, + onLongClick = { onSongLongClick(MenuTarget.Song(song, coverArt(album?.coverArt, true))) }, ), ) } diff --git a/app/src/main/kotlin/com/subtracks/ui/library/AlbumsContent.kt b/app/src/main/kotlin/com/subtracks/ui/library/AlbumsContent.kt index 288f9281..2a4b2066 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/AlbumsContent.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/AlbumsContent.kt @@ -94,7 +94,7 @@ fun AlbumsContent( .clip(RoundedCornerShape(2.dp)) .combinedClickable( onClick = { onAlbumClick(album) }, - onLongClick = { onLongClick(MenuTarget.Album(album)) }, + onLongClick = { onLongClick(MenuTarget.Album(album, art)) }, ).testTag(ALBUM_COVER_TAG), ) } diff --git a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt index 8cdeb610..efd6ca57 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt @@ -116,7 +116,7 @@ fun ArtistDetailRoute( onBack = onBack, onAlbumClick = onAlbumClick, onAlbumLongClick = { menuTarget = it }, - onMore = { artist?.let { menuTarget = MenuTarget.Artist(it) } }, + onMore = { artist?.let { menuTarget = MenuTarget.Artist(it, artThumbnail ?: art) } }, ) menuTarget?.let { target -> ItemContextMenu(target = target, actions = actions, onDismiss = { menuTarget = null }) @@ -238,7 +238,7 @@ fun ArtistDetailScreen( end = if (index % 2 == 1) 16.dp else 0.dp, ).combinedClickable( onClick = { onAlbumClick(album) }, - onLongClick = { onAlbumLongClick(MenuTarget.Album(album)) }, + onLongClick = { onAlbumLongClick(MenuTarget.Album(album, coverArt(album.coverArt, true))) }, ), ) { CoverArt( diff --git a/app/src/main/kotlin/com/subtracks/ui/library/ArtistsContent.kt b/app/src/main/kotlin/com/subtracks/ui/library/ArtistsContent.kt index 542c66c9..48c37126 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistsContent.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistsContent.kt @@ -98,7 +98,7 @@ fun ArtistsContent( modifier = Modifier.combinedClickable( onClick = { onArtistClick(artist) }, - onLongClick = { onLongClick(MenuTarget.Artist(artist)) }, + onLongClick = { onLongClick(MenuTarget.Artist(artist, coverArt(artist.coverArt, true))) }, ), colors = ListItemDefaults.colors(containerColor = Color.Transparent), ) diff --git a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt index acf113a4..088770a4 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt @@ -61,7 +61,7 @@ fun PlaylistDetailRoute( onSongLongClick = { menuTarget = it }, onShuffle = viewModel::shuffle, onPlay = viewModel::playAll, - onMore = { playlist?.let { menuTarget = MenuTarget.Playlist(it) } }, + onMore = { playlist?.let { menuTarget = MenuTarget.Playlist(it, viewModel.coverArt(it.coverArt, true)) } }, playingSongId = playback.item?.id.takeIf { context?.kind == QueueKind.Playlist && context.refId == playlistId }, ) menuTarget?.let { target -> @@ -116,7 +116,7 @@ fun PlaylistDetailScreen( modifier = rowModifier.combinedClickable( onClick = { onSongClick(index) }, - onLongClick = { onSongLongClick(MenuTarget.Song(item.song)) }, + onLongClick = { onSongLongClick(MenuTarget.Song(item.song, coverArt(item.coverArt, true))) }, ), ) } diff --git a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistsContent.kt b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistsContent.kt index bc5dd0d4..e206a057 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistsContent.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistsContent.kt @@ -96,7 +96,7 @@ fun PlaylistsContent( modifier = Modifier.combinedClickable( onClick = { onPlaylistClick(playlist) }, - onLongClick = { onLongClick(MenuTarget.Playlist(playlist)) }, + onLongClick = { onLongClick(MenuTarget.Playlist(playlist, coverArt(playlist.coverArt, true))) }, ), colors = ListItemDefaults.colors(containerColor = Color.Transparent), ) diff --git a/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt b/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt index 442613db..d4744647 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt @@ -91,7 +91,7 @@ fun SongsContent( modifier = Modifier.combinedClickable( onClick = { onSongClick(item.song.id) }, - onLongClick = { onLongClick(MenuTarget.Song(item.song)) }, + onLongClick = { onLongClick(MenuTarget.Song(item.song, coverArt(item.coverArt, true))) }, ), ) } diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt index dbfa12b7..9fc4919d 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt @@ -138,7 +138,7 @@ fun NowPlayingRoute( onPrevious = controller::previous, onShuffle = controller::toggleShuffle, onRepeat = controller::cycleRepeat, - onMore = { song?.let { menuTarget = MenuTarget.Song(it) } }, + onMore = { song?.let { menuTarget = MenuTarget.Song(it, thumbnail) } }, onSeek = controller::seekTo, modifier = modifier, ) diff --git a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt index d0971b62..5cbca415 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt @@ -9,8 +9,11 @@ import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.model.Song import com.subtracks.data.prefs.fakeUserPreferences import com.subtracks.data.source.StarType +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer @@ -22,6 +25,7 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import java.util.concurrent.TimeUnit @RunWith(AndroidJUnit4::class) class LibraryRepositoryTest { @@ -80,7 +84,19 @@ class LibraryRepositoryTest { } @Test - fun aRejectedStarLeavesTheRowUntouchedAndReports() = + fun starringUpdatesTheRowBeforeTheServerResponds() = + runBlocking { + withSource { server -> + server.enqueue(ok().setBodyDelay(10, TimeUnit.SECONDS)) + + val call = launch { repository.setStar(StarType.Song, "s1", true) } + withTimeout(3_000) { while (starred() == null) delay(10) } + call.cancel() + } + } + + @Test + fun aRejectedStarRollsBackAndReports() = runBlocking { withSource { server -> server.enqueue(failed(40)) @@ -92,6 +108,22 @@ class LibraryRepositoryTest { } } + @Test + fun aRejectedUnstarRestoresTheStarredRow() = + runBlocking { + withSource { server -> + server.enqueue(ok()) + repository.setStar(StarType.Song, "s1", true) + val starredAt = starred() + server.enqueue(failed(40)) + + assertTrue(repository.setStar(StarType.Song, "s1", false).isFailure) + + assertEquals(starredAt, starred()) + assertEquals(listOf("Could not update star"), messages) + } + } + private suspend fun withSource(block: suspend (MockWebServer) -> Unit) { val server = MockWebServer() server.start()