From b2f654e10f252345e58208cccb251621b45735dc Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:19:32 +0000 Subject: [PATCH] address second review: shuffle cache invalidation, locking, ended latch --- .../subtracks/data/repo/QueueRepository.kt | 26 ++++++---- .../com/subtracks/data/sync/SyncManager.kt | 7 ++- .../main/kotlin/com/subtracks/di/AppModule.kt | 2 +- .../subtracks/playback/PlaybackController.kt | 51 ++++++++++++------- .../com/subtracks/ui/playback/QueueScreen.kt | 2 +- .../data/repo/QueueRepositoryTest.kt | 3 +- .../playback/PlaybackControllerTest.kt | 21 ++++++++ 7 files changed, 78 insertions(+), 34 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt index 2b098af5..c112654a 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt @@ -109,11 +109,8 @@ class QueueRepository( } } val (entries, order, shuffled) = loaded - if (shuffled && order == null) { - setShuffle(false, null) - return QueueSnapshot(entries, null, queueVersion) - } - return QueueSnapshot(entries, order, queueVersion) + val valid = if (shuffled && order == null) null else order + return QueueSnapshot(entries, valid, queueVersion) } suspend fun modes(): QueueModes { @@ -126,15 +123,24 @@ class QueueRepository( order: LongArray?, ) { val seed = if (enabled && order != null) System.nanoTime() else 0L - dao.clearShuffleOrder() - if (enabled && order != null) { - dao.insertShuffleOrder(order.mapIndexed { index, flat -> ShuffleOrder(index.toLong(), flat) }) + db.useWriterConnection { transactor -> + transactor.immediateTransaction { + dao.clearShuffleOrder() + if (enabled && order != null) { + dao.insertShuffleOrder(order.mapIndexed { index, flat -> ShuffleOrder(index.toLong(), flat) }) + } + dao.setCursor(cursorRow().copy(shuffleEnabled = enabled, shuffleSeed = seed)) + } } - dao.setCursor(cursorRow().copy(shuffleEnabled = enabled, shuffleSeed = seed)) cachedShuffleSeed = 0 cachedShuffleOrder = null } + fun invalidateLibraryCache() { + cachedFlatIds = null + queueVersion++ + } + suspend fun setRepeat(mode: Int) = dao.setCursor(cursorRow().copy(repeatMode = mode)) private suspend fun loadShuffleOrder( @@ -271,7 +277,7 @@ class QueueRepository( private suspend fun flatIds(snapshot: QueueSnapshot): List { val cached = cachedFlatIds - if (cached != null && cachedFlatVersion == snapshot.version) return cached + if (cached != null && cachedFlatVersion == snapshot.version && cached.size.toLong() == snapshot.size) return cached val ids = ArrayList(snapshot.size.toInt()) for (resolved in snapshot.entries) { val entry = resolved.entry diff --git a/app/src/main/kotlin/com/subtracks/data/sync/SyncManager.kt b/app/src/main/kotlin/com/subtracks/data/sync/SyncManager.kt index 981b7fdc..9f1bf0c4 100644 --- a/app/src/main/kotlin/com/subtracks/data/sync/SyncManager.kt +++ b/app/src/main/kotlin/com/subtracks/data/sync/SyncManager.kt @@ -1,6 +1,7 @@ package com.subtracks.data.sync import android.util.Log +import com.subtracks.data.repo.QueueRepository import com.subtracks.data.repo.SourceRepository import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -23,6 +24,7 @@ sealed interface SyncStatus { class SyncManager( private val sourceRepository: SourceRepository, + private val queueRepository: QueueRepository, ) { private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) private val _status = MutableStateFlow(SyncStatus.Idle) @@ -36,7 +38,10 @@ class SyncManager( _status.value = SyncStatus.Running _status.value = sourceRepository.sync().fold( - onSuccess = { SyncStatus.Success }, + onSuccess = { + queueRepository.invalidateLibraryCache() + SyncStatus.Success + }, onFailure = { error -> Log.w(TAG, "Sync failed", error) SyncStatus.Failed(error.message ?: "Sync failed") diff --git a/app/src/main/kotlin/com/subtracks/di/AppModule.kt b/app/src/main/kotlin/com/subtracks/di/AppModule.kt index 15249269..6e077cb6 100644 --- a/app/src/main/kotlin/com/subtracks/di/AppModule.kt +++ b/app/src/main/kotlin/com/subtracks/di/AppModule.kt @@ -34,7 +34,7 @@ fun appModule( single { SourceRepository(get(), get(), get()) } single { LibraryRepository(get(), get()) } single { ArtworkSeedRepository(get()) } - single { SyncManager(get()) } + single { SyncManager(get(), get()) } single { QueueRepository(get()) } single { MediaSessionConnection(context.applicationContext, get()) } single { PlaybackController(get(), get(), get()) } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index d28c67d0..a5cb8752 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -537,7 +537,15 @@ class PlaybackController( shuffleEnabled = modes.shuffle repeatMode = modes.repeat.toRepeatMode() player?.setRepeatOne(repeatMode == RepeatMode.One) - val snapshot = readSnapshot() + var snapshot = queueRepository.snapshot() + if (shuffleEnabled && !snapshot.shuffled && snapshot.size > 0L) { + val start = queueRepository.cursor().coerceIn(0, snapshot.size - 1) + queueRepository.setShuffle(true, shuffledOrder(snapshot.size, start)) + queueRepository.setCursor(0) + snapshot = queueRepository.snapshot() + } else if (!snapshot.shuffled) { + shuffleEnabled = false + } this.snapshot = snapshot val queueSourceId = snapshot.entries @@ -600,22 +608,24 @@ class PlaybackController( private fun jumpTo(target: Long) { scope.launch { - val snapshot = readSnapshot() - this@PlaybackController.snapshot = snapshot - if (snapshot.size == 0L) return@launch - val current = currentPosition() ?: return@launch - val dest = target.coerceIn(0, snapshot.size - 1) - val wasEnded = player?.isEnded == true - if (dest == current && !wasEnded) return@launch - windowJob?.cancel() - queueRepository.setCursor(dest) - shiftWindow(dest) - val player = player ?: return@launch - player.ensurePrepared() - player.seekToIndex((dest - windowStart).toInt()) - if (wasEnded) player.play() - endedHandled = false - refresh(dest) + startLock.withLock { + val snapshot = readSnapshot() + this@PlaybackController.snapshot = snapshot + if (snapshot.size == 0L) return@withLock + val current = currentPosition() ?: return@withLock + val dest = target.coerceIn(0, snapshot.size - 1) + val wasEnded = player?.isEnded == true + if (dest == current && !wasEnded) return@withLock + windowJob?.cancel() + queueRepository.setCursor(dest) + shiftWindowLocked(dest) + val player = player ?: return@withLock + player.ensurePrepared() + player.seekToIndex((dest - windowStart).toInt()) + if (wasEnded) player.play() + endedHandled = false + refresh(dest) + } } } @@ -702,7 +712,7 @@ class PlaybackController( } override fun onEvents() { - if (player?.isEnded == true) handleEnded() + if (player?.isEnded == true) handleEnded() else endedHandled = false refresh() } @@ -730,7 +740,10 @@ class PlaybackController( private suspend fun readSnapshot(): QueueSnapshot { val snapshot = queueRepository.snapshot() - if (!snapshot.shuffled) shuffleEnabled = false + if (!snapshot.shuffled) { + if (shuffleEnabled && snapshot.size > 0L) queueRepository.setShuffle(false, null) + shuffleEnabled = false + } return snapshot } diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt index fdfbd6c6..520ea929 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -250,7 +250,7 @@ fun QueueRoute( sourceRepository: SourceRepository = koinInject(), ) { val playback by controller.state.collectAsStateWithLifecycle() - LaunchedEffect(Unit) { viewModel.open() } + LaunchedEffect(playback.shuffle) { viewModel.open() } QueueScreen( rows = viewModel.rows, ready = viewModel.ready, diff --git a/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt index e2c18e08..e8c93ac2 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt @@ -321,7 +321,7 @@ class QueueRepositoryTest { (0 until snapshot.size).mapNotNull { repository.itemAt(snapshot, it)?.song?.id } @Test - fun aStaleShuffleOrderIsDroppedWhenTheQueueLengthChanges() = + fun aStaleShuffleOrderIsIgnoredWhenTheQueueLengthChanges() = runTest { seedLibrary() repository.replace(listOf(repository.albumEntry(1, "al1"))) @@ -331,7 +331,6 @@ class QueueRepositoryTest { db.libraryDao().upsertSongs(listOf(song("s6", "al1", track = 4, album = "First Album"))) assertFalse(repository.snapshot().shuffled) - assertFalse(repository.modes().shuffle) } private suspend fun seedLibrary() { diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 3e88a4e2..d2bda52c 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -819,6 +819,27 @@ class PlaybackControllerTest { ) } + @Test + fun repeatAllLoopsAfterEndingWithRepeatOff() { + seedAlbum(2, sourceId = 1) + + controller.playAlbum(1, "al1", 0) + await { controller.state.value.isPlaying } + + handle.finish() + handle.emitEvents() + await { !controller.state.value.isPlaying } + + controller.togglePlayPause() + await { controller.state.value.isPlaying } + controller.cycleRepeat() + assertEquals(RepeatMode.All, controller.state.value.repeat) + + handle.finish() + handle.emitEvents() + await { controller.state.value.isPlaying } + } + private fun seedAlbum( count: Int, sourceId: Long,