From fa5595db3030395f87fcf19a6166f6b688e18de4 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Tue, 29 Sep 2026 09:23:02 +0000 Subject: [PATCH] react to active source changes The active source becomes one immutable id+source pair, so cover art and stream/download URLs can no longer read a torn state mid-switch. The root navigation watches the active source id and, on a real switch, closes the overlays and pops an open album/artist/playlist detail back to the library. --- .../subtracks/data/repo/SourceRepository.kt | 34 ++++---- .../main/kotlin/com/subtracks/ui/AppRoot.kt | 36 ++++++++ .../com/subtracks/ui/SourceSwitchResetTest.kt | 86 +++++++++++++++++++ 3 files changed, 139 insertions(+), 17 deletions(-) create mode 100644 app/src/test/kotlin/com/subtracks/ui/SourceSwitchResetTest.kt diff --git a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt index 5bc8ea29..f47a03e7 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt @@ -40,11 +40,13 @@ class SourceRepository( ) { private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) - @Volatile - private var active: SubsonicMusicSource? = null + private data class ActiveSource( + val id: Long, + val source: SubsonicMusicSource, + ) @Volatile - private var activeSourceId: Long? = null + private var active: ActiveSource? = null private val _quality = MutableStateFlow(StreamQuality()) val quality: StateFlow = _quality @@ -87,10 +89,9 @@ class SourceRepository( ) { config, mode, wifi, mobile, concurrency -> Triple(config, if (mode == NetworkMode.Wifi) wifi else mobile, concurrency) }.collect { (config, quality, concurrency) -> - active = config?.toMusicSource(quality, concurrency) + val sourceChanged = config?.id != active?.id + active = config?.let { ActiveSource(it.id, it.toMusicSource(quality, concurrency)) } _quality.value = quality - val sourceChanged = config?.id != activeSourceId - activeSourceId = config?.id if (sourceChanged && config != null && config.useTokenAuth && !_offline.value) { scope.launch { runCatching { config.toClient().check("ping") } } } @@ -116,12 +117,12 @@ class SourceRepository( coverArt: String?, thumbnail: Boolean = false, ): CoverArtRef? { - val source = active ?: return null + val current = active ?: return null if (coverArt == null) return null - val cacheKey = coverArtKey(source.id, coverArt, thumbnail) - artworkStore.uri(source.id, cacheKey)?.let { return CoverArtRef(url = it, cacheKey = cacheKey) } + val cacheKey = coverArtKey(current.id, coverArt, thumbnail) + artworkStore.uri(current.id, cacheKey)?.let { return CoverArtRef(url = it, cacheKey = cacheKey) } if (_offline.value) return null - val url = source.coverArtUri(coverArt, thumbnail)?.toString() ?: return null + val url = current.source.coverArtUri(coverArt, thumbnail)?.toString() ?: return null return CoverArtRef(url = url, cacheKey = cacheKey) } @@ -131,9 +132,8 @@ class SourceRepository( thumbnail: Boolean, ): String? { if (_offline.value) return null - val source = active ?: return null - if (source.id != sourceId) return null - return source.coverArtUri(coverArt, thumbnail)?.toString() + val current = active?.takeIf { it.id == sourceId } ?: return null + return current.source.coverArtUri(coverArt, thumbnail)?.toString() } fun streamUri( @@ -142,7 +142,7 @@ class SourceRepository( quality: StreamQuality, ): String? { if (_offline.value) return null - val uri = active?.streamUri(songId)?.toString() ?: return null + val uri = active?.source?.streamUri(songId)?.toString() ?: return null val length = declaredStreamLength(durationMs, quality) ?: return uri return uri + streamLengthSuffix(length) } @@ -152,14 +152,14 @@ class SourceRepository( songId: String, ): String? { if (_offline.value) return null - val source = active?.takeIf { activeSourceId == sourceId } ?: return null + val current = active?.takeIf { it.id == sourceId } ?: return null // A transcode preference downloads the stream, so the saved file is already the wanted // quality rather than the original that would then have to be transcoded on every play. val quality = downloadQuality return if (quality.transcodes) { - source.streamUri(songId, quality.maxBitrate, quality.format).toString() + current.source.streamUri(songId, quality.maxBitrate, quality.format).toString() } else { - source.downloadUri(songId).toString() + current.source.downloadUri(songId).toString() } } diff --git a/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt b/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt index b67c5016..03b22e4a 100644 --- a/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt +++ b/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt @@ -131,6 +131,23 @@ private object Routes { fun editServer(id: Long) = "edit-server/$id" } +private val DETAIL_ROUTES = + setOf(Routes.ALBUM_DETAIL, Routes.ARTIST_DETAIL, Routes.PLAYLIST_DETAIL) + +internal fun resetOnSourceSwitch( + previousSourceId: Long?, + currentSourceId: Long?, + route: String?, + closeNowPlaying: () -> Unit, + closeQueue: () -> Unit, + popDetail: () -> Unit, +) { + if (previousSourceId == null || currentSourceId == null || previousSourceId == currentSourceId) return + closeNowPlaying() + closeQueue() + if (route in DETAIL_ROUTES) popDetail() +} + internal data class BackStackKey( val route: String?, val argument: String?, @@ -191,6 +208,25 @@ private fun MainNavigation() { val contextMenuHost = remember { ContextMenuHost() } var infoSong by remember { mutableStateOf(null) } + val activeSourceId by libraryRepository.activeSourceId.collectAsStateWithLifecycle(initialValue = null) + var previousSourceId by rememberSaveable { mutableStateOf(null) } + LaunchedEffect(activeSourceId) { + val id = activeSourceId + resetOnSourceSwitch( + previousSourceId = previousSourceId, + currentSourceId = id, + route = currentRoute, + closeNowPlaying = { + settleJob?.cancel() + nowPlayingOpen = false + nowPlayingProgress = 0f + }, + closeQueue = { showingQueue = false }, + popDetail = { navController.popBackStack(Routes.LIBRARY, inclusive = false) }, + ) + if (id != null) previousSourceId = id + } + fun settleNowPlaying(open: Boolean) { if (open) { nowPlayingOpen = true diff --git a/app/src/test/kotlin/com/subtracks/ui/SourceSwitchResetTest.kt b/app/src/test/kotlin/com/subtracks/ui/SourceSwitchResetTest.kt new file mode 100644 index 00000000..8450ec05 --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/ui/SourceSwitchResetTest.kt @@ -0,0 +1,86 @@ +package com.subtracks.ui + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class SourceSwitchResetTest { + private var nowPlayingClosed = false + private var queueClosed = false + private var detailPopped = false + + private fun reset( + previous: Long?, + current: Long?, + route: String?, + ) { + nowPlayingClosed = false + queueClosed = false + detailPopped = false + resetOnSourceSwitch( + previousSourceId = previous, + currentSourceId = current, + route = route, + closeNowPlaying = { nowPlayingClosed = true }, + closeQueue = { queueClosed = true }, + popDetail = { detailPopped = true }, + ) + } + + @Test + fun theFirstSourceResetsNothing() { + reset(previous = null, current = 1L, route = "album/{albumId}?coverArt={coverArt}") + + assertFalse(nowPlayingClosed) + assertFalse(queueClosed) + assertFalse(detailPopped) + } + + @Test + fun reselectingTheSameSourceResetsNothing() { + reset(previous = 1L, current = 1L, route = "settings") + + assertFalse(nowPlayingClosed) + assertFalse(queueClosed) + assertFalse(detailPopped) + } + + @Test + fun losingTheSourceResetsNothing() { + reset(previous = 1L, current = null, route = "settings") + + assertFalse(nowPlayingClosed) + assertFalse(queueClosed) + assertFalse(detailPopped) + } + + @Test + fun aSwitchClosesTheOverlays() { + reset(previous = 1L, current = 2L, route = "settings") + + assertTrue(nowPlayingClosed) + assertTrue(queueClosed) + assertFalse(detailPopped) + } + + @Test + fun aSwitchOnADetailPopsIt() { + reset(previous = 1L, current = 2L, route = "album/{albumId}?coverArt={coverArt}") + + assertTrue(nowPlayingClosed) + assertTrue(queueClosed) + assertTrue(detailPopped) + } + + @Test + fun everyDetailRoutePopsOnASwitch() { + listOf( + "album/{albumId}?coverArt={coverArt}", + "artist/{artistId}?coverArt={coverArt}", + "playlist/{playlistId}", + ).forEach { route -> + reset(previous = 1L, current = 2L, route = route) + assertTrue("$route should be dismissed", detailPopped) + } + } +}