From 72330387c8ab092cb24f6691403a49a61811621e Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 06:28:33 +0000 Subject: [PATCH] fix resuming after removing the playing track in a shuffled queue The "play whatever now fills the removed track's slot" branch was written for the old re-derive model: it mapped the removed canonical slot through the new permutation, which now (with the fixed shuffle order) picks an unrelated track. Removing the playing track should just resume at the same slot, which holds whatever was next. - Drop the special slot branch: `position == current` resumes at `current` for shuffled and unshuffled alike. Without a block that is the next shuffled track; with one, re-anchor the block from the removed track onto the track now preceding that slot so the block takes the slot and plays next (previously it was stranded behind a random track). - Tests assert the immediate playing track, not just what is upcoming, which is why they missed this. Both new tests fail on the previous commit. - Cover advancing out of a block track and moving a block track while shuffled (the last items from the QA review). --- .../subtracks/data/repo/QueueRepository.kt | 2 + .../subtracks/playback/PlaybackController.kt | 30 ++--- .../playback/PlaybackControllerTest.kt | 121 +++++++++--------- 3 files changed, 77 insertions(+), 76 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 ec8825eb..8fe28ab8 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt @@ -437,6 +437,8 @@ class QueueRepository( return Reanchored(play, true) } + suspend fun setUpNextAnchor(anchor: Long) = cursorMutex.withLock { dao.setCursor(cursorRow().copy(upNextAnchor = anchor)) } + suspend fun clearUpNext() { writeUpNext(emptyList()) } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 075e37f7..897172f5 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -344,26 +344,24 @@ class PlaybackController( lastEdit = ReloadUndo(entries, cursor, snapshot.shuffleSeed, upNext, snapshot.upNextAnchor) return@withLock } + // The block floats behind the currently playing context track, so removing that track + // removes its anchor. Re-anchor the block to the track that now precedes the removed + // slot in play order, so it takes the slot and plays next, rather than the anchor + // landing wherever `adjustAnchorOnRemove` left it. + if (snapshot.shuffled && position == current && !wasUpNext && updated.upNextSize > 0L) { + val play = snapshot.contextPlay(current) + val predecessor = play - 1L + val anchor = if (predecessor < 0L) -1L else updated.flatContext(predecessor) ?: -1L + queueRepository.setUpNextAnchor(anchor) + updated = readSnapshot() + this.snapshot = updated + } // Derive the current track's new position from the edit. Resolving its song id after // the edit can match an identical song elsewhere (a duplicate in the context or the - // block), so use canonical positions throughout: a shuffled removal re-derives the - // permutation at the smaller size, so the current track's play position is a remap of - // its flat index, not a simple shift. + // block), so use positions throughout. Removing the playing track resumes at the same + // slot, which now holds whatever was next (the block, if one is queued). var target = when { - position == current && snapshot.shuffled && !wasUpNext -> { - // The permutation is re-derived at the smaller size, so there is no stable - // "next"; play whatever now fills the removed track's slot. - val removedFlat = snapshot.flatContext(snapshot.contextPlay(position)) ?: 0L - val newContextSize = (snapshot.contextSize - 1).coerceAtLeast(0) - if (newContextSize == 0L) { - 0L - } else { - val slot = removedFlat.coerceIn(0, newContextSize - 1) - updated.combined(updated.playContext(slot) ?: 0L) - } - } - position == current -> { current } diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 3d266a38..6edd5bc8 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -1640,63 +1640,6 @@ class PlaybackControllerTest { assertEquals(5, remaining.distinct().size) } - @Test - fun removingTheCurrentTrackWhileShuffledPlaysTheSongThatFillsItsSlot() { - seedAlbum(6, sourceId = 1) - - controller.playAlbum(1, "al1", 2) - await { - controller.state.value.item - ?.id == "s3" - } - - controller.toggleShuffle() - await { controller.state.value.shuffle } - val current = controller.state.value.position!! - val before = runBlocking { queues.snapshot() } - val removedFlat = before.flatContext(current)!! - - runBlocking { controller.removeAt(current) } - - val after = runBlocking { queues.snapshot() } - val newContextSize = (before.contextSize - 1).coerceAtLeast(0) - val slot = removedFlat.coerceIn(0, newContextSize - 1) - assertEquals(5L, after.size) - assertEquals( - // 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 removingThePlayingAnchorWhileShuffledPlaysTheQueuedTrackNext() { - seedAlbum(6, sourceId = 1) - seedSong("x1", "al2") - - controller.playAlbum(1, "al1", 2) - await { - controller.state.value.item - ?.id == "s3" - } - 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) } - - await { runBlocking { controller.upcomingItem()?.id } == "x1" } - } - @Test fun removingThePlayingTrackWhileShuffledLeavesTheBlockReachable() { seedAlbum(6, sourceId = 1) @@ -1714,14 +1657,34 @@ class PlaybackControllerTest { val current = controller.state.value.position!! runBlocking { controller.removeAt(current) } + // The block was queued behind the removed track, so it takes the slot and plays now. + await { + controller.state.value.item + ?.id == "x1" + } val after = runBlocking { queues.snapshot() } assertTrue(after.shuffled) assertEquals( - controller.state.value.item - ?.id, + "x1", runBlocking { queues.itemAt(after, controller.state.value.position!!)?.song?.id }, ) - await { runBlocking { controller.upcomingItem()?.id } == "x1" } + } + + @Test + fun removingThePlayingTrackWhileShuffledPlaysTheNextShuffledTrack() { + seedShuffledQueue(seed = 12345L) + val orderBefore = resolvedIds() + val current = controller.state.value.position!! + val expected = orderBefore.toMutableList().apply { removeAt(current.toInt()) }[current.toInt()] + + runBlocking { controller.removeAt(current) } + + await { + controller.state.value.item + ?.id == expected + } + val after = runBlocking { queues.snapshot() } + assertEquals(expected, runBlocking { queues.itemAt(after, controller.state.value.position!!)?.song?.id }) } @Test @@ -1740,6 +1703,44 @@ class PlaybackControllerTest { assertEquals(currentId, runBlocking { queues.itemAt(after, controller.state.value.position!!)?.song?.id }) } + @Test + fun advancingOutOfABlockTrackWhileShuffledPlaysTheNextBlockTrack() { + seedShuffledQueue(seed = 5L) + seedSong("x1", "al2") + seedSong("x2", "al2") + controller.addToQueue(1, QueueKind.Song, "x1") + controller.addToQueue(1, QueueKind.Song, "x2") + await { runBlocking { controller.upcomingItem()?.id } == "x1" } + + controller.playAt(1) + await { + controller.state.value.item + ?.id == "x1" + } + controller.next() + + await { + controller.state.value.item + ?.id == "x2" + } + } + + @Test + fun movingABlockTrackWhileShuffledReordersTheBlock() { + seedShuffledQueue(seed = 5L) + seedSong("x1", "al2") + seedSong("x2", "al2") + controller.addToQueue(1, QueueKind.Song, "x1") + controller.addToQueue(1, QueueKind.Song, "x2") + await { runBlocking { controller.upcomingItem()?.id } == "x1" } + + runBlocking { controller.move(2, 1) } + + val after = runBlocking { queues.snapshot() } + assertEquals("x2", runBlocking { queues.itemAt(after, after.anchorPlay + 1)?.song?.id }) + assertEquals("x1", runBlocking { queues.itemAt(after, after.anchorPlay + 2)?.song?.id }) + } + @Test fun undoingARemovalRestoresTheShuffledOrder() { seedShuffledQueue(seed = 42L)