From e6386ebddd94fa8b14bbd94a5be0a14484e8a792 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:06:38 +0000 Subject: [PATCH] keep the up-next block reachable when the playing context track is removed while shuffled Review found that removing the playing context track while shuffled with a non-empty up-next block stranded the block behind the cursor: the repository moves `upNextAnchor` to the preceding track, but the cursor's re-derived slot position can land after the block, so the queued tracks were skipped. Re-anchor the block to the track the cursor lands on, same as `playAt`/`toggleShuffle`. - regression test loops the sequence over fresh random seeds and asserts the block stays reachable; fails without the fix ("stranded behind the cursor"). - `removingTheCurrentTrackWhileShuffled...` now asserts the canonical slot the current track occupies instead of recomputing the production position formula (which only checked the implementation against itself). - `QueueRepository.removeAt` returns Unit; every caller discarded the snapshot and re-read it, so it no longer builds one. --- .../subtracks/data/repo/QueueRepository.kt | 7 ++- .../subtracks/playback/PlaybackController.kt | 15 +++++- .../playback/PlaybackControllerTest.kt | 47 +++++++++++++++++-- 3 files changed, 58 insertions(+), 11 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 7ae286f5..dfcddb16 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt @@ -384,17 +384,17 @@ class QueueRepository( suspend fun removeAt( snapshot: QueueSnapshot, position: Long, - ): QueueSnapshot { + ) { if (snapshot.isUpNext(position)) { removeUpNextAt(snapshot, snapshot.upNextIndex(position)) - return snapshot() + return } val play = snapshot.contextPlay(position) if (snapshot.shuffled) { // The permutation is derived from (seed, size), so removing a track re-derives it at // the smaller size; keep the same seed and store the new size so the queue stays // shuffled. There is no stable "next" to preserve here. - val flat = snapshot.flatContext(play) ?: return snapshot + val flat = snapshot.flatContext(play) ?: return write(compact(removeEntry(snapshot.entries, flat)).map { it.entry }) adjustAnchorOnRemove(flat) setShuffleSize(contextSize()) @@ -403,7 +403,6 @@ class QueueRepository( write(removed.map { it.entry }) adjustAnchorOnRemove(play) } - return snapshot() } private suspend fun adjustAnchorOnRemove(removedFlat: Long) = diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 83e5276c..7bd98146 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -323,7 +323,7 @@ class PlaybackController( val wasUpNext = snapshot.isUpNext(current) val removed = if (position == current) null else queueRepository.itemAt(snapshot, position)?.toQueueItem() queueRepository.removeAt(snapshot, position) - val updated = readSnapshot() + var updated = readSnapshot() this.snapshot = updated windowJob?.cancel() if (updated.size == 0L) { @@ -348,12 +348,23 @@ class PlaybackController( } else { (if (position < current) current - 1 else current) } - val target = + var target = ( currentId?.let { if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it) } ?: fallback ).coerceIn(0, updated.size - 1) + // Removing the playing context track while shuffled leaves the block anchored to some + // other track; re-anchor it to the track the cursor landed on so the queued tracks + // still play next rather than being stranded behind the cursor. + if (snapshot.shuffled && !wasUpNext && position == current && updated.upNextSize > 0L) { + val reanchored = queueRepository.reanchorFor(updated, target) + if (reanchored.changed) { + updated = readSnapshot() + this.snapshot = updated + target = reanchored.position + } + } lastPosition = target lastEdit = if (removed == null) { diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index d6ccf537..fb58d03e 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -17,7 +17,6 @@ import com.subtracks.data.prefs.StreamQuality import com.subtracks.data.prefs.UserPreferences import com.subtracks.data.prefs.fakeUserPreferences import com.subtracks.data.repo.QueueRepository -import com.subtracks.data.repo.Shuffle import com.subtracks.data.repo.SourceRepository import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.asCoroutineDispatcher @@ -1595,17 +1594,55 @@ class PlaybackControllerTest { val after = runBlocking { queues.snapshot() } val newContextSize = (before.contextSize - 1).coerceAtLeast(0) - val slot = removedFlat.coerceIn(0, (newContextSize - 1).coerceAtLeast(0)) - val expected = after.combined(Shuffle.toSequence(before.shuffleSeed!!, newContextSize, slot)) + val slot = removedFlat.coerceIn(0, newContextSize - 1) assertEquals(5L, after.size) - assertEquals(expected, controller.state.value.position) assertEquals( - runBlocking { queues.itemAt(after, expected)?.song?.id }, + // The song now occupying the removed track's canonical slot is the one playing. + slot, + after.flatContext(after.contextPlay(controller.state.value.position!!)), + ) + assertEquals( + runBlocking { queues.itemAt(after, controller.state.value.position!!)?.song?.id }, controller.state.value.item ?.id, ) } + @Test + fun removingThePlayingTrackWhileShuffledLeavesTheBlockReachable() { + seedAlbum(6, sourceId = 1) + seedSong("x1", "al2") + + repeat(30) { + if (controller.state.value.shuffle) { + controller.toggleShuffle() + await { !controller.state.value.shuffle } + } + runBlocking { controller.clearUpNext() } + controller.playAlbum(1, "al1", 2) + await { + controller.state.value.item + ?.id == "s3" && !controller.state.value.shuffle + } + controller.addToQueue(1, QueueKind.Song, "x1") + await { runBlocking { controller.upcomingItem()?.id } == "x1" } + + controller.toggleShuffle() + await { controller.state.value.shuffle } + await { runBlocking { controller.upcomingItem()?.id } == "x1" } + val current = controller.state.value.position!! + + runBlocking { controller.removeAt(current) } + + val snapshot = runBlocking { queues.snapshot() } + val position = controller.state.value.position!! + assertTrue( + "queued track stranded behind the cursor at $position (block at ${snapshot.anchorPlay})", + position <= snapshot.anchorPlay || snapshot.isUpNext(position), + ) + } + } + @Test fun movingContextTracksWhileShuffledIsIgnored() { seedAlbum(6, sourceId = 1)