From 43f0721d26efea84a2463904eaa5c14722bc7d76 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:20:21 +0000 Subject: [PATCH] playback: keep the queue and player in sync across up-next A context drag that crosses the up-next block moves the block's anchor in the repository but the old player.move used the combined drop position, leaving the two in different orders. Rebuild the window for context moves that happen while a block exists, in both the in-window and window-offset paths. Also drop the redundant rebuildWindow snapshot parameter, the unused media-item context read-back, and add coverage for shuffled album context, ranged album moves and ranged up-next moves. --- .../subtracks/playback/Media3PlayerHandle.kt | 1 - .../subtracks/playback/PlaybackController.kt | 24 +++--- .../data/repo/QueueRepositoryTest.kt | 82 +++++++++++++++++++ .../playback/PlaybackControllerTest.kt | 45 +++++++++- 4 files changed, 133 insertions(+), 19 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt b/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt index 8c2a20b8..ce33c723 100644 --- a/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt +++ b/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt @@ -235,7 +235,6 @@ class Media3PlayerHandle( ?.takeIf { it.containsKey(EXTRA_DURATION_MS) } ?.getLong(EXTRA_DURATION_MS), replayGain = mediaItem.replayGain(), - albumContext = mediaItem.albumContext(), ) } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 9449b2a5..1e6a0603 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -456,7 +456,7 @@ class PlaybackController( if (snapshot.upNextSize > 0L || updated.upNextSize > 0L || snapshot.shuffled) { queueRepository.setCursor(target) if (position != current) { - rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target, updated) + rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target) refresh(target) } else { loadWindow(target, autoplay = playing, startPositionMs = 0) @@ -546,13 +546,13 @@ class PlaybackController( return@withLock } when { - fromInWindow && toInWindow -> { + fromInWindow && toInWindow && (snapshot.upNextSize == 0L || snapshot.isUpNext(from)) -> { updating = true player.move((from - windowStart).toInt(), (to - windowStart).toInt()) updating = false } - !fromInWindow && !toInWindow -> { + !fromInWindow && !toInWindow && (snapshot.upNextSize == 0L || snapshot.isUpNext(from)) -> { // The moved track never enters the window, so the playlist is unchanged; only // the window's positions shift, and a move that spans it shifts them by one. when { @@ -569,7 +569,7 @@ class PlaybackController( } else -> { - rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target, updated) + rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target) } } shiftWindowLocked(target) @@ -768,7 +768,7 @@ class PlaybackController( if (wasUpNext) { loadWindow(target, autoplay = true) } else { - rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target, updated) + rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target) refresh(target) } } @@ -864,7 +864,7 @@ class PlaybackController( layoutVersion++ queueRepository.setCursor(position) windowJob?.cancel() - rebuildWindow(queueRepository.window(updated, position, QUEUE_WINDOW_RADIUS), position, updated) + rebuildWindow(queueRepository.window(updated, position, QUEUE_WINDOW_RADIUS), position) refresh(position) } @@ -913,11 +913,7 @@ class PlaybackController( if (anchor.changed) layoutVersion++ this@PlaybackController.snapshot = finalSnapshot queueRepository.setCursor(anchor.position) - rebuildWindow( - queueRepository.window(finalSnapshot, anchor.position, QUEUE_WINDOW_RADIUS), - anchor.position, - finalSnapshot, - ) + rebuildWindow(queueRepository.window(finalSnapshot, anchor.position, QUEUE_WINDOW_RADIUS), anchor.position) endedHandled = false refresh(anchor.position) } @@ -1282,7 +1278,7 @@ class PlaybackController( (currentId?.let { queueRepository.combinedIndexOf(snapshot, it) } ?: center) .coerceIn(0, snapshot.size - 1) queueRepository.setCursor(position) - rebuildWindow(queueRepository.window(snapshot, position, QUEUE_WINDOW_RADIUS), position, snapshot) + rebuildWindow(queueRepository.window(snapshot, position, QUEUE_WINDOW_RADIUS), position) refresh(position) return } @@ -1318,9 +1314,9 @@ class PlaybackController( private suspend fun rebuildWindow( window: List, position: Long, - snapshot: QueueSnapshot, ) { val player = player ?: return + val snapshot = this.snapshot ?: return downloads.awaitLoaded() val ordered = if (offline) window.filter { downloads.localUri(it.item.song.id) != null } else window @@ -1420,7 +1416,7 @@ class PlaybackController( if (target == current) { // Keep the playing item in place and only add/remove its neighbours, so a downloaded // track does not re-prepare (and audibly gap) when offline mode is toggled. - rebuildWindow(queueRepository.window(snapshot, target, QUEUE_WINDOW_RADIUS), target, snapshot) + rebuildWindow(queueRepository.window(snapshot, target, QUEUE_WINDOW_RADIUS), target) refresh(target) } else { loadWindow(target, autoplay = player.playWhenReady, startPositionMs = 0) 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 605052dc..775b6eee 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt @@ -505,6 +505,43 @@ class QueueRepositoryTest { assertEquals(1L, entry.rangeEnd) } + @Test + fun movingARangedAlbumTrackInUpNextKeepsTheAbsoluteOffsets() = + runTest { + seedLibrary() + db.libraryDao().upsertSongs( + listOf( + song("s6", "al1", track = 4, album = "First Album"), + song("s7", "al1", track = 5, album = "First Album"), + song("x1", "al3", track = 1), + ), + ) + val snapshot = repository.snapshotAfter(listOf(repository.albumEntry(1, "al2"))) + repository.addUpNext(snapshot, currentPosition = 0, entry = albumRange(1, 4), playNext = false) + repository.addUpNext( + repository.snapshot(), + currentPosition = 0, + entry = repository.songEntry(1, "x1"), + playNext = false, + ) + assertEquals(listOf("s4", "s2", "s3", "s6", "s7", "x1", "s5"), resolveAll(repository.snapshot())) + + repository.move(repository.snapshot(), from = 4, to = 5) + + var updated = repository.snapshot() + assertEquals(listOf("s4", "s2", "s3", "s6", "x1", "s7", "s5"), resolveAll(updated)) + val moved = updated.locate(5)!!.first + assertEquals(QueueKind.Album, moved.kind) + assertEquals(4L, moved.rangeStart) + assertEquals(4L, moved.rangeEnd) + + repository.move(repository.snapshot(), from = 5, to = 4) + + updated = repository.snapshot() + assertEquals(listOf("s4", "s2", "s3", "s6", "s7", "x1", "s5"), resolveAll(updated)) + assertTrue(updated.isAlbumContext(4)) + } + @Test fun movingAPlaylistTrackKeepsItAPinnedSong() = runTest { @@ -897,6 +934,51 @@ class QueueRepositoryTest { assertEquals("First Album", repository.sourceName(entry.kind, entry.sourceId, entry.refId)) } + @Test + fun movingWithinARangedAlbumKeepsTheAbsoluteOffsets() = + runTest { + seedLibrary() + db.libraryDao().upsertSongs( + listOf( + song("s6", "al1", track = 4, album = "First Album"), + song("s7", "al1", track = 5, album = "First Album"), + ), + ) + repository.snapshotAfter(listOf(albumRange(1, 4))) + assertEquals(listOf("s2", "s3", "s6", "s7"), resolveAll(repository.snapshot())) + + repository.move(repository.snapshot(), from = 0, to = 2) + + val snapshot = repository.snapshot() + assertEquals(listOf("s3", "s6", "s2", "s7"), resolveAll(snapshot)) + val (entry, _) = snapshot.locate(2)!! + assertEquals(QueueKind.Album, entry.kind) + assertEquals(1L, entry.rangeStart) + assertEquals(1L, entry.rangeEnd) + } + + @Test + fun movingBackwardWithinARangedAlbumKeepsTheAbsoluteOffsets() = + runTest { + seedLibrary() + db.libraryDao().upsertSongs( + listOf( + song("s6", "al1", track = 4, album = "First Album"), + song("s7", "al1", track = 5, album = "First Album"), + ), + ) + repository.snapshotAfter(listOf(albumRange(1, 4))) + + repository.move(repository.snapshot(), from = 3, to = 0) + + val snapshot = repository.snapshot() + assertEquals(listOf("s7", "s2", "s3", "s6"), resolveAll(snapshot)) + val (entry, _) = snapshot.locate(0)!! + assertEquals(QueueKind.Album, entry.kind) + assertEquals(4L, entry.rangeStart) + assertEquals(4L, entry.rangeEnd) + } + @Test fun movingAnAlbumTrackBackCompactsTheAlbumEntry() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 5f42d339..1796f4bc 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -508,18 +508,26 @@ class PlaybackControllerTest { assertTrue(handle.items.first { it.id == "x1" }.albumContext) } + @Test + fun shuffledAlbumTracksKeepAlbumContext() { + seedShuffledQueue(seed = 5L) + + assertTrue(handle.items.isNotEmpty()) + assertTrue(handle.items.all { it.albumContext }) + } + @Test fun movingAnAlbumTrackKeepsItsAlbumGainContext() { - seedAlbum(4, sourceId = 1) + seedAlbum(60, sourceId = 1) controller.playAlbum(1, "al1", 0) await { handle.items.any { it.id == "s1" } } - runBlocking { controller.move(0, 2) } - await { controller.state.value.position == 2L } + runBlocking { controller.move(0, 40) } + await { controller.state.value.position == 40L } assertTrue(handle.items.first { it.id == "s1" }.albumContext) - val (entry, _) = runBlocking { queues.snapshot().locate(2)!! } + val (entry, _) = runBlocking { queues.snapshot().locate(40)!! } assertEquals(QueueKind.Album, entry.kind) assertEquals("al1", entry.refId) } @@ -540,6 +548,35 @@ class PlaybackControllerTest { assertTrue(handle.items.first { it.id == "s2" }.albumContext) } + @Test + fun movingAContextTrackAcrossUpNextKeepsPlayerAndQueueInSync() { + seedAlbum(3, sourceId = 1) + seedSong("x1", "al2") + + controller.playAlbum(1, "al1", 0) + await { + controller.state.value.item + ?.id == "s1" + } + controller.addToQueue(1, QueueKind.Song, "x1") + await { handle.items.any { it.id == "x1" } } + + runBlocking { controller.move(0, 3) } + await { controller.state.value.position == 2L } + + val expected = + runBlocking { + val snapshot = queues.snapshot() + (0 until snapshot.size).mapNotNull { queues.itemAt(snapshot, it)?.song?.id } + } + assertEquals(expected, handle.items.map { it.id }) + assertEquals( + "s1", + controller.state.value.item + ?.id, + ) + } + @Test fun tappingAContextTrackWhileAQueuedTrackPlaysKeepsTheQueue() { seedAlbum(3, sourceId = 1)