From 767c3db1aeb85be0696e9b6697593de90482f43d Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 12 Nov 2026 10:26:51 +0000 Subject: [PATCH] playback: serialize transitions and preserve detached playback Run transition bookkeeping under the queue mutation lock, retain detached playback when its final survivor is removed, and restore offline repeat wrapping without a forward survivor. Centralize detached survivor rebuilding and add regressions for each defect. --- .../subtracks/playback/PlaybackController.kt | 115 +++++++++--------- .../com/subtracks/playback/FakePlayer.kt | 4 + .../playback/PlaybackControllerTest.kt | 105 ++++++++++++++++ 3 files changed, 169 insertions(+), 55 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index ae8e14c3..39106ebc 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -408,6 +408,24 @@ class PlaybackController( if (snapshot.shuffled && !snapshot.isUpNext(position)) return@withMutationLock if (snapshot.size == 1L) { if (!removeAt(snapshot, position)) return@withMutationLock + if (detachedWindow && currentPosition() == null) { + val updated = readSnapshot() + this@PlaybackController.snapshot = updated + windowJob?.cancel() + rebuildDetachedSurvivors(updated, 0) + queueRepository.setCursor(0) + lastEdit = + ReloadUndo( + entries, + cursor, + snapshot.shuffleSeed, + upNext, + snapshot.upNextAnchor, + updated.version, + ) + refresh(null) + return@withMutationLock + } stopLocked() lastEdit = ReloadUndo( @@ -736,11 +754,14 @@ class PlaybackController( val snapshot = readSnapshot() this@PlaybackController.snapshot = snapshot if (snapshot.size == 0L) return@withMutationLock - val current = currentQueuePosition() ?: return@withMutationLock if (detachedWindow) { - detachedPosition?.let { jumpToLocked(it, null) } + val target = + detachedPosition + ?: if (offline && repeatMode != RepeatMode.Off) downloadedAtOrAfter(snapshot, 0) else null + target?.let { jumpToLocked(it, null) } return@withMutationLock } + val current = currentQueuePosition() ?: return@withMutationLock if (snapshot.isUpNext(current)) { consumeUpNextLocked(current) return@withMutationLock @@ -777,14 +798,6 @@ class PlaybackController( else -> null } - private suspend fun consumeUpNext( - position: Long, - requestedVersion: Long, - ) = queueRepository.withMutationLock { - if (this@PlaybackController.snapshot?.resetVersion != requestedVersion) return@withMutationLock - consumeUpNextLocked(position) - } - private suspend fun QueueMutationScope.consumeUpNextLocked(position: Long) { val snapshot = this@PlaybackController.snapshot ?: return if (!snapshot.isUpNext(position)) return @@ -1007,19 +1020,24 @@ class PlaybackController( isOffline = offline } val snapshot = current ?: return null - val at = position ?: return null val detachedNext = detachedWindow && currentPosition() == null if (!isOffline) { + val at = position ?: return null if (detachedNext) return queueItemAt(snapshot, at) return nextPosition(snapshot, at) ?.takeIf { it != at } ?.let { queueItemAt(snapshot, it) } } downloads.awaitLoaded() - val from = if (detachedNext) at else at + 1 + val at = position val target = - downloadedAtOrAfter(snapshot, from) - ?: if (repeatMode != RepeatMode.Off) downloadedAtOrAfter(snapshot, 0L) else null + if (at == null) { + if (detachedNext && repeatMode != RepeatMode.Off) downloadedAtOrAfter(snapshot, 0L) else null + } else { + val from = if (detachedNext) at else at + 1 + downloadedAtOrAfter(snapshot, from) + ?: if (repeatMode != RepeatMode.Off) downloadedAtOrAfter(snapshot, 0L) else null + } return target?.takeIf { it != at || detachedNext }?.let { queueItemAt(snapshot, it) } } @@ -1327,21 +1345,23 @@ class PlaybackController( } private suspend fun onPositionChanged() { - val player = player ?: return - pruneDetached(player) - val snapshot = snapshot ?: return - if (snapshot.size == 0L) return - val position = (currentQueuePosition() ?: return).coerceIn(0, snapshot.size - 1) - val previous = lastPosition - lastPosition = position - if (previous != null && previous != position && snapshot.isUpNext(previous)) { - consumeUpNext(previous, snapshot.resetVersion) - return + queueRepository.withMutationLock { + val player = player ?: return@withMutationLock + pruneDetached(player) + val snapshot = snapshot ?: return@withMutationLock + if (snapshot.size == 0L) return@withMutationLock + val position = (currentQueuePosition() ?: return@withMutationLock).coerceIn(0, snapshot.size - 1) + val previous = lastPosition + lastPosition = position + if (previous != null && previous != position && snapshot.isUpNext(previous)) { + consumeUpNextLocked(previous) + return@withMutationLock + } + lastSavedPositionMs = 0 + queueRepository.setCursor(position) + refresh(position) + scheduleWindowShift(position) } - lastSavedPositionMs = 0 - queueRepository.setCursor(position) - refresh(position) - scheduleWindowShift(position) } private fun scheduleWindowShift(center: Long) { @@ -1466,30 +1486,10 @@ class PlaybackController( } return } - trimToCurrent(player) - if (updated.size == 0L) { - markDetached(emptyList(), null) - lastPosition = null - queueRepository.setCursor(0) - refresh(null) - return - } - val nearest = (current?.position ?: lastPosition ?: 0L).coerceIn(0, updated.size - 1) - val window = - queueRepository - .range(updated, nearest, (nearest + QUEUE_WINDOW_RADIUS).coerceAtMost(updated.size - 1)) - .filter { !offline || downloads.localUri(it.item.song.id) != null } - updating = true - try { - window.forEachIndexed { index, entry -> player.insertAt(index + 1, entry.playerItem(updated)) } - } finally { - updating = false - } - val next = window.firstOrNull()?.position - markDetached(window, next) - lastPosition = next - queueRepository.setCursor(next ?: 0L) - refresh(next) + val nearest = (current?.position ?: lastPosition ?: 0L).coerceIn(0, (updated.size - 1).coerceAtLeast(0)) + rebuildDetachedSurvivors(updated, nearest) + queueRepository.setCursor(lastPosition ?: 0L) + refresh(lastPosition) } private fun trimToCurrent(player: PlayerHandle) { @@ -1585,7 +1585,7 @@ class PlaybackController( ): Boolean { if (!detachedWindow || currentPosition() != null) return false queueRepository.setCursor(position) - rebuildWindow(queueRepository.window(snapshot, position, QUEUE_WINDOW_RADIUS), position) + rebuildDetachedSurvivors(snapshot, position) refresh(position) return true } @@ -1752,12 +1752,17 @@ class PlaybackController( downloads.awaitLoaded() queueRepository.withMutationLock { if (!endJobIsCurrent(endedSnapshot, endedPosition)) return@withMutationLock + val snapshot = this@PlaybackController.snapshot ?: return@withMutationLock if (detachedWindow && detachedPosition == null) { - stopLocked() + val target = if (repeatMode == RepeatMode.All) downloadedAtOrAfter(snapshot, 0L) else null + if (target != null) { + jumpToLocked(target, requestedVersion) + } else { + stopLocked() + } return@withMutationLock } val current = endedPosition ?: return@withMutationLock - val snapshot = this@PlaybackController.snapshot ?: return@withMutationLock val from = if (detachedWindow) current else current + 1 val target = downloadedAtOrAfter(snapshot, from) diff --git a/app/src/test/kotlin/com/subtracks/playback/FakePlayer.kt b/app/src/test/kotlin/com/subtracks/playback/FakePlayer.kt index e5ca0fba..eff21d40 100644 --- a/app/src/test/kotlin/com/subtracks/playback/FakePlayer.kt +++ b/app/src/test/kotlin/com/subtracks/playback/FakePlayer.kt @@ -45,6 +45,9 @@ class FakePlayerHandle : PlayerHandle { @Volatile var expectedThread: Thread? = null + @Volatile + var beforeRemoveFirst: (() -> Unit)? = null + private fun checkThread() { val expected = expectedThread ?: return check(Thread.currentThread() == expected) { "Expected $expected, was ${Thread.currentThread()}" } @@ -131,6 +134,7 @@ class FakePlayerHandle : PlayerHandle { } override fun removeFirst() { + beforeRemoveFirst?.invoke() operations += "removeFirst" items.removeAt(0) index-- diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 6d354826..e07af03b 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -3931,6 +3931,96 @@ class PlaybackControllerTest { assertEquals("s4", handle.currentItem?.id) } + @Test + fun aTransitionWaitsForARemovalToRebuildThePlayerWindow() { + seedAlbum(2, sourceId = 1) + seedSong("a1", "al2") + seedSong("b1", "al2") + controller.playAlbum(1, "al1", 0) + await { handle.currentItem?.id == "s1" && controller.state.value.isPlaying } + controller.addToQueue(1, QueueKind.Song, "a1") + controller.addToQueue(1, QueueKind.Song, "b1") + await { runBlocking { queues.snapshot().upNextSize } == 2L } + controller.next() + await { handle.currentItem?.id == "a1" && controller.state.value.position == 1L } + val dispatcherBlocked = CountDownLatch(1) + val releaseDispatcher = CountDownLatch(1) + controllerScope.launch { + dispatcherBlocked.countDown() + releaseDispatcher.await() + } + assertTrue(dispatcherBlocked.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)) + handle.advanceTo(handle.currentIndex + 1) + val rebuildStarted = CountDownLatch(1) + val releaseRebuild = CountDownLatch(1) + handle.beforeRemoveFirst = { + handle.beforeRemoveFirst = null + rebuildStarted.countDown() + releaseRebuild.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS) + } + val removal = repoScope.async { controller.removeAt(1) } + + try { + assertTrue(rebuildStarted.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)) + releaseDispatcher.countDown() + Thread.sleep(100) + } finally { + releaseDispatcher.countDown() + releaseRebuild.countDown() + } + runBlocking { withTimeout(TEST_TIMEOUT_MS) { removal.await() } } + Thread.sleep(200) + + assertEquals(listOf("s1", "b1", "s2"), resolvedIds()) + assertEquals("b1", handle.currentItem?.id) + assertEquals(1L, controller.state.value.position) + } + + @Test + fun removingTheLastSurvivorKeepsDetachedPlaybackAndUndo() { + seedPlaylist(2) + controller.playPlaylist(1, "p1", 0) + await { handle.currentItem?.id == "s1" && controller.state.value.isPlaying } + val point = runBlocking { queues.captureResetPoint() } + setPlaylist("s2") + runBlocking { queues.resetChangedOrigins(setOf(QueueOrigin(QueueKind.Playlist, 1, "p1")), point) } + await { handle.items.map { it.id } == listOf("s1", "s2") } + + runBlocking { controller.removeAt(0) } + + assertEquals(listOf("s1"), handle.items.map { it.id }) + assertEquals("s1", handle.currentItem?.id) + assertNull(controller.state.value.position) + assertEquals(0L, runBlocking { queues.cursor() }) + runBlocking { controller.undo() } + assertEquals(listOf("s1", "s2"), handle.items.map { it.id }) + assertEquals("s1", handle.currentItem?.id) + assertEquals(0L, controller.state.value.position) + } + + @Test + fun offlineDetachedPlaybackWithoutAForwardSurvivorWrapsOnManualNext() { + detachOfflineTrackWithoutAForwardSurvivor() + + val upcoming = runBlocking { controller.upcomingItem()?.id } + controller.next() + Thread.sleep(200) + + assertEquals("s1", handle.currentItem?.id) + assertEquals("s1", upcoming) + assertEquals(0L, controller.state.value.position) + } + + @Test + fun offlineDetachedPlaybackWithoutAForwardSurvivorWrapsWhenEnded() { + detachOfflineTrackWithoutAForwardSurvivor() + + handle.finish() + + await { handle.currentItem?.id == "s1" && controller.state.value.isPlaying } + assertEquals(0L, controller.state.value.position) + } + @Test fun aResetRemapsAContextTrackThatMovedToUpNext() { seedPlaylist(5) @@ -4113,6 +4203,21 @@ class PlaybackControllerTest { assertEquals("s0", runBlocking { queues.itemAt(queues.snapshot(), 0) }?.song?.id) } + private fun detachOfflineTrackWithoutAForwardSurvivor() { + seedPlaylist(3) + markDownloaded(1, "s1") + markDownloaded(1, "s2") + setOffline() + controller.playPlaylist(1, "p1", 1) + await { handle.currentItem?.id == "s2" && controller.state.value.isPlaying } + val point = runBlocking { queues.captureResetPoint() } + setPlaylist("s1", "s3") + runBlocking { queues.resetChangedOrigins(setOf(QueueOrigin(QueueKind.Playlist, 1, "p1")), point) } + await { handle.items.map { it.id } == listOf("s2") && controller.state.value.position == null } + controller.cycleRepeat() + assertEquals(RepeatMode.All, controller.state.value.repeat) + } + private fun markDownloaded( sourceId: Long, songId: String,