From 2c333195efe38ff04a60e22a775eae420c58d2d3 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:01:52 +0000 Subject: [PATCH] address shuffle review findings --- .../subtracks/data/repo/QueueRepository.kt | 33 +++++++++++-------- .../subtracks/playback/PlaybackController.kt | 31 ++++++++++------- .../data/repo/QueueRepositoryTest.kt | 14 ++++++++ .../playback/PlaybackControllerTest.kt | 24 ++++++++++++++ 4 files changed, 76 insertions(+), 26 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 85a48195..2b098af5 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt @@ -21,6 +21,7 @@ data class ResolvedQueueEntry( data class QueueSnapshot( val entries: List, val shuffleOrder: LongArray? = null, + val version: Long = 0, ) { val size: Long = entries.sumOf { it.length } @@ -96,20 +97,24 @@ class QueueRepository( dao.setCursor(cursorRow().copy(queuePosition = 0)) } - suspend fun snapshot(): QueueSnapshot = - db.useReaderConnection { transactor -> - transactor.deferredTransaction { - val entries = dao.entries().map { ResolvedQueueEntry(it, it.resolvedLength()) } - val row = dao.cursor() - val order = - if (row?.shuffleEnabled == true) { - loadShuffleOrder(row.shuffleSeed, entries.sumOf { it.length }) - } else { - null - } - QueueSnapshot(entries, order) + suspend fun snapshot(): QueueSnapshot { + val loaded = + db.useReaderConnection { transactor -> + transactor.deferredTransaction { + val entries = dao.entries().map { ResolvedQueueEntry(it, it.resolvedLength()) } + val row = dao.cursor() + val shuffled = row?.shuffleEnabled == true + val order = if (shuffled) loadShuffleOrder(row.shuffleSeed, entries.sumOf { it.length }) else null + Triple(entries, order, shuffled) + } } + val (entries, order, shuffled) = loaded + if (shuffled && order == null) { + setShuffle(false, null) + return QueueSnapshot(entries, null, queueVersion) } + return QueueSnapshot(entries, order, queueVersion) + } suspend fun modes(): QueueModes { val row = dao.cursor() ?: return QueueModes(shuffle = false, repeat = 0) @@ -266,7 +271,7 @@ class QueueRepository( private suspend fun flatIds(snapshot: QueueSnapshot): List { val cached = cachedFlatIds - if (cached != null && cachedFlatVersion == queueVersion) return cached + if (cached != null && cachedFlatVersion == snapshot.version) return cached val ids = ArrayList(snapshot.size.toInt()) for (resolved in snapshot.entries) { val entry = resolved.entry @@ -282,7 +287,7 @@ class QueueRepository( ids.addAll(entryIds.subList(from, to)) } cachedFlatIds = ids - cachedFlatVersion = queueVersion + cachedFlatVersion = snapshot.version return ids } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index bbd97f71..d28c67d0 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -205,7 +205,7 @@ class PlaybackController( val playing = player.playWhenReady val removed = if (position == current) null else queueRepository.itemAt(snapshot, position)?.toQueueItem() queueRepository.removeAt(snapshot, position) - this.snapshot = queueRepository.snapshot() + this.snapshot = readSnapshot() windowJob?.cancel() lastEdit = if (removed == null) { @@ -252,7 +252,7 @@ class PlaybackController( val cursor = queueRepository.cursor() val entries = snapshot.entries.map { it.entry } if (!queueRepository.move(snapshot, from, to)) return@withLock - this.snapshot = queueRepository.snapshot() + this.snapshot = readSnapshot() val target = movedCursor(current, from, to) windowJob?.cancel() val fromInWindow = from in windowStart..windowEnd @@ -303,7 +303,7 @@ class PlaybackController( val positionMs = player.currentPositionMs queueRepository.replace(undo.entries, undo.shuffleOrder) shuffleEnabled = undo.shuffleOrder != null - val restored = queueRepository.snapshot() + val restored = readSnapshot() this.snapshot = restored lastEdit = null if (restored.size == 0L) { @@ -435,7 +435,7 @@ class PlaybackController( fun toggleShuffle() { scope.launch { startLock.withLock { - val snapshot = queueRepository.snapshot() + val snapshot = readSnapshot() this@PlaybackController.snapshot = snapshot if (snapshot.size == 0L) return@withLock val player = player ?: return@withLock @@ -443,17 +443,18 @@ class PlaybackController( val positionMs = player.currentPositionMs val playing = player.playWhenReady val flat = snapshot.flatPosition(current) ?: return@withLock + windowJob?.cancel() if (shuffleEnabled) { queueRepository.setShuffle(false, null) shuffleEnabled = false queueRepository.setCursor(flat) - this@PlaybackController.snapshot = queueRepository.snapshot() + this@PlaybackController.snapshot = readSnapshot() loadWindow(flat, autoplay = playing, startPositionMs = positionMs) } else { queueRepository.setShuffle(true, shuffledOrder(snapshot.size, flat)) shuffleEnabled = true queueRepository.setCursor(0) - this@PlaybackController.snapshot = queueRepository.snapshot() + this@PlaybackController.snapshot = readSnapshot() loadWindow(0, autoplay = playing, startPositionMs = positionMs) } endedHandled = false @@ -522,7 +523,7 @@ class PlaybackController( start } queueRepository.setCursor(position) - this.snapshot = queueRepository.snapshot() + this.snapshot = readSnapshot() loadWindow(position, autoplay = true) } @@ -536,7 +537,7 @@ class PlaybackController( shuffleEnabled = modes.shuffle repeatMode = modes.repeat.toRepeatMode() player?.setRepeatOne(repeatMode == RepeatMode.One) - val snapshot = queueRepository.snapshot() + val snapshot = readSnapshot() this.snapshot = snapshot val queueSourceId = snapshot.entries @@ -599,17 +600,17 @@ class PlaybackController( private fun jumpTo(target: Long) { scope.launch { - val snapshot = queueRepository.snapshot() + 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) - if (dest == current) return@launch + val wasEnded = player?.isEnded == true + if (dest == current && !wasEnded) return@launch windowJob?.cancel() queueRepository.setCursor(dest) shiftWindow(dest) val player = player ?: return@launch - val wasEnded = player.isEnded player.ensurePrepared() player.seekToIndex((dest - windowStart).toInt()) if (wasEnded) player.play() @@ -642,7 +643,7 @@ class PlaybackController( private suspend fun shiftWindowLocked(center: Long) { val player = player ?: return - val snapshot = queueRepository.snapshot() + val snapshot = readSnapshot() this.snapshot = snapshot if (snapshot.size == 0L) return val target = center.coerceIn(0, snapshot.size - 1) @@ -727,6 +728,12 @@ class PlaybackController( private fun Int.toRepeatMode(): RepeatMode = RepeatMode.entries.getOrElse(this) { RepeatMode.Off } + private suspend fun readSnapshot(): QueueSnapshot { + val snapshot = queueRepository.snapshot() + if (!snapshot.shuffled) shuffleEnabled = false + return snapshot + } + private fun contextAt(position: Long?): QueueContext? { val entry = position?.let { snapshot?.locate(it)?.first } ?: return null return QueueContext(entry.kind, entry.refId) 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 f3f271f7..e2c18e08 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt @@ -320,6 +320,20 @@ class QueueRepositoryTest { private suspend fun resolveAll(snapshot: QueueSnapshot): List = (0 until snapshot.size).mapNotNull { repository.itemAt(snapshot, it)?.song?.id } + @Test + fun aStaleShuffleOrderIsDroppedWhenTheQueueLengthChanges() = + runTest { + seedLibrary() + repository.replace(listOf(repository.albumEntry(1, "al1"))) + repository.setShuffle(true, longArrayOf(2, 0, 1)) + assertTrue(repository.snapshot().shuffled) + + db.libraryDao().upsertSongs(listOf(song("s6", "al1", track = 4, album = "First Album"))) + + assertFalse(repository.snapshot().shuffled) + assertFalse(repository.modes().shuffle) + } + private suspend fun seedLibrary() { db.sourcesDao().upsertSource( Source(id = 1, name = "test", address = "http://localhost", isActive = true, createdAt = 0), diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 25371cb9..3e88a4e2 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -795,6 +795,30 @@ class PlaybackControllerTest { ) } + @Test + fun repeatAllLoopsASingleTrackQueue() { + seedAlbum(1, sourceId = 1) + + controller.playAlbum(1, "al1", 0) + await { + controller.state.value.item + ?.id == "s1" && controller.state.value.isPlaying + } + + controller.cycleRepeat() + assertEquals(RepeatMode.All, controller.state.value.repeat) + + handle.finish() + handle.emitEvents() + await { controller.state.value.isPlaying } + + assertEquals( + "s1", + controller.state.value.item + ?.id, + ) + } + private fun seedAlbum( count: Int, sourceId: Long,