From e7a1927ccdeb8c00d3b19de4c2767f67d43c1cbe Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sat, 26 Sep 2026 20:23:34 +0000 Subject: [PATCH] address review: re-anchoring and consume edge cases - jumpTo no longer returns early on a stale current position after a re-anchor, which could leave the player and snapshot unsynchronised. - playNext from inside the block inserts after the entry holding the current track, not always at index 1. - expose a layout version in PlaybackState so the queue view reloads when a re-anchor shifts the combined positions under it. - removeAt recomputes the current track's combined position from the refreshed snapshot instead of assuming a one-down shift. - restore keeps the saved resume position when the anchor moves. - next on the last queued track with repeat off no longer consumes it and leaves it playing; repeat wraps instead. - drop a redundant sort in range; revert a rejected cross-section drag by reloading. --- .../subtracks/data/repo/QueueRepository.kt | 35 +++- .../subtracks/playback/PlaybackController.kt | 53 +++-- .../com/subtracks/ui/playback/QueueScreen.kt | 6 +- .../playback/PlaybackControllerTest.kt | 183 ++++++++++++++++++ 4 files changed, 259 insertions(+), 18 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 51070496..11a14ad4 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/QueueRepository.kt @@ -87,6 +87,18 @@ data class QueueSnapshot( fun locateContext(contextPlay: Long): Pair? = locateIn(entries, flatContext(contextPlay) ?: return null) } +private fun entryIndexAfter( + entries: List, + index: Long, +): Int { + var start = 0L + entries.forEachIndexed { position, resolved -> + if (index < start + resolved.length) return position + 1 + start += resolved.length + } + return entries.size +} + private fun locateIn( entries: List, index: Long, @@ -274,6 +286,13 @@ class QueueRepository( ): Long? { val upIndex = idsFor(snapshot.upNext).indexOf(songId) if (upIndex >= 0) return snapshot.anchorPlay + 1 + upIndex + return combinedContextIndexOf(snapshot, songId) + } + + suspend fun combinedContextIndexOf( + snapshot: QueueSnapshot, + songId: String, + ): Long? { val flat = flatIds(snapshot).indexOf(songId) if (flat < 0) return null val play = snapshot.shuffleOrder?.indexOf(flat.toLong())?.toLong() ?: flat.toLong() @@ -299,7 +318,6 @@ class QueueRepository( if (blockFirst <= blockLast) items += blockPlayRange(snapshot, blockFirst - anchor - 1, blockLast - anchor - 1) val afterFirst = maxOf(from, anchor + block + 1) if (afterFirst <= to) items += contextPlayRange(snapshot, afterFirst - block, to - block) - items.sortBy { it.position } return items } @@ -319,12 +337,19 @@ class QueueRepository( playNext: Boolean, ): Long { val list = snapshot.upNext.map { it.entry }.toMutableList() - val active = snapshot.isUpNext(currentPosition) val insertIndex = when { - playNext && active -> 1 - playNext -> 0 - else -> list.size + playNext && snapshot.isUpNext(currentPosition) -> { + entryIndexAfter(snapshot.upNext, snapshot.upNextIndex(currentPosition)) + } + + playNext -> { + 0 + } + + else -> { + list.size + } }.coerceIn(0, list.size) list.add(insertIndex, entry.copy(id = 0, position = 0)) val anchor = diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 9aef52a4..329be82d 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -52,6 +52,7 @@ data class PlaybackState( val hasPrevious: Boolean = false, val shuffle: Boolean = false, val repeat: RepeatMode = RepeatMode.Off, + val layout: Long = 0, ) private sealed interface QueueUndo { @@ -130,6 +131,7 @@ class PlaybackController( private var windowStart = 0L private var windowEnd = -1L private var lastPosition: Long? = null + private var layoutVersion = 0L private var updating = false private var lastEdit: QueueUndo? = null private var shuffleEnabled = false @@ -280,6 +282,7 @@ class PlaybackController( if (anchor.changed) { snapshot = readSnapshot() this@PlaybackController.snapshot = snapshot + layoutVersion++ } val target = anchor.position.coerceIn(0, snapshot.size - 1) windowJob?.cancel() @@ -315,18 +318,37 @@ class PlaybackController( return@withLock } val current = currentPosition() ?: return@withLock - val target = (if (position < current) current - 1 else current).coerceIn(0, snapshot.size - 2) val playing = player.playWhenReady + val wasUpNext = snapshot.isUpNext(current) val removed = if (position == current) null else queueRepository.itemAt(snapshot, position)?.toQueueItem() queueRepository.removeAt(snapshot, position) - this.snapshot = readSnapshot() + val updated = readSnapshot() + this.snapshot = updated windowJob?.cancel() + if (updated.size == 0L) { + stopLocked() + lastEdit = ReloadUndo(entries, cursor, snapshot.shuffleOrder, upNext, snapshot.upNextAnchor) + return@withLock + } + val currentId = player.currentItem?.id + val target = + ( + currentId?.let { + if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it) + } ?: (if (position < current) current - 1 else current) + ).coerceIn(0, updated.size - 1) lastEdit = if (removed == null) { ReloadUndo(entries, cursor, snapshot.shuffleOrder, upNext, snapshot.upNextAnchor) } else { RemovedUndo(entries, cursor, target, position, removed, snapshot.shuffleOrder, upNext, snapshot.upNextAnchor) } + if (snapshot.upNextSize > 0L || updated.upNextSize > 0L) { + layoutVersion++ + queueRepository.setCursor(target) + loadWindow(target, autoplay = playing, startPositionMs = if (position == current) 0 else player.currentPositionMs) + return@withLock + } when { position == current -> { loadWindow(target, autoplay = playing, startPositionMs = 0) @@ -521,17 +543,16 @@ class PlaybackController( startLock.withLock { val snapshot = snapshot ?: return@withLock if (!snapshot.isUpNext(position)) return@withLock + if (position >= snapshot.size - 1 && repeatMode == RepeatMode.Off) return@withLock queueRepository.removeUpNextAt(snapshot, snapshot.upNextIndex(position)) val updated = readSnapshot() this.snapshot = updated + layoutVersion++ windowJob?.cancel() endedHandled = false - queueRepository.setCursor(position) - if (position < updated.size) { - loadWindow(position, autoplay = true) - } else { - refresh(position) - } + val next = if (position < updated.size) position else 0L + queueRepository.setCursor(next) + loadWindow(next, autoplay = true) } fun previous() { @@ -595,6 +616,7 @@ class PlaybackController( val position = queueRepository.addUpNext(snapshot, current, entry, playNext) val updated = readSnapshot() this.snapshot = updated + layoutVersion++ queueRepository.setCursor(position) windowJob?.cancel() rebuildWindow(queueRepository.window(updated, position, QUEUE_WINDOW_RADIUS), position) @@ -644,6 +666,7 @@ class PlaybackController( } val anchor = queueRepository.reanchorFor(reordered, position) val finalSnapshot = if (anchor.changed) readSnapshot() else reordered + if (anchor.changed) layoutVersion++ this@PlaybackController.snapshot = finalSnapshot queueRepository.setCursor(anchor.position) rebuildWindow(queueRepository.window(finalSnapshot, anchor.position, QUEUE_WINDOW_RADIUS), anchor.position) @@ -751,15 +774,17 @@ class PlaybackController( return@withLock } this.queueSourceId = queueSourceId + val savedPositionMs = queueRepository.cursorPositionMs().coerceAtLeast(0) val cursor = queueRepository.cursor().coerceIn(0, snapshot.size - 1) val anchor = queueRepository.reanchorFor(snapshot, cursor) if (anchor.changed) { snapshot = queueRepository.snapshot() this.snapshot = snapshot queueRepository.setCursor(anchor.position) + queueRepository.setPosition(savedPositionMs) } - lastSavedPositionMs = queueRepository.cursorPositionMs().coerceAtLeast(0) - loadWindow(anchor.position, autoplay = false, startPositionMs = lastSavedPositionMs) + lastSavedPositionMs = savedPositionMs + loadWindow(anchor.position, autoplay = false, startPositionMs = savedPositionMs) } private suspend fun stop() = startLock.withLock { stopLocked() } @@ -829,12 +854,15 @@ class PlaybackController( var snapshot = readSnapshot() if (snapshot.size == 0L) return@withLock val anchor = queueRepository.reanchorFor(snapshot, target) - if (anchor.changed) snapshot = readSnapshot() + if (anchor.changed) { + snapshot = readSnapshot() + layoutVersion++ + } this@PlaybackController.snapshot = snapshot val current = currentPosition() ?: return@withLock val dest = anchor.position.coerceIn(0, snapshot.size - 1) val wasEnded = player.isEnded - if (dest == current && !wasEnded) return@withLock + if (!anchor.changed && dest == current && !wasEnded) return@withLock windowJob?.cancel() queueRepository.setCursor(dest) if (anchor.changed) { @@ -1056,6 +1084,7 @@ class PlaybackController( hasPrevious = true, shuffle = shuffleEnabled, repeat = repeatMode, + layout = layoutVersion, ) if (player.isPlaying) startPositionTicker() else stopPositionTicker() } diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt index 2cee2b23..d2eb6b53 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -265,7 +265,7 @@ fun QueueRoute( LaunchedEffect(queueContext) { sourceTitle.value = controller.sourceTitle(queueContext)?.takeIf { it.isNotBlank() } ?: fallbackTitle } - LaunchedEffect(playback.shuffle, open) { if (open) viewModel.open() } + LaunchedEffect(playback.shuffle, playback.layout, open) { if (open) viewModel.open() } QueueScreen( rows = viewModel.rows, ready = viewModel.ready, @@ -283,6 +283,7 @@ fun QueueRoute( onLoadOlder = viewModel::loadOlder, onLoadNewer = viewModel::loadNewer, onUndo = viewModel::undo, + onRefresh = viewModel::open, modifier = modifier, ) } @@ -307,6 +308,7 @@ fun QueueScreen( generation: Int = 0, shuffle: Boolean = false, contextTitle: String? = null, + onRefresh: () -> Unit = {}, ) { val listState = rememberLazyListState() val fill = rememberViewportFill(listState) @@ -415,6 +417,8 @@ fun QueueScreen( if (to != from && sameSection) { onMove(from, to) showUndo("Queue reordered") + } else if (to != from) { + onRefresh() } } dragId = null diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 583383b4..6e4d38b1 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -429,6 +429,189 @@ class PlaybackControllerTest { } } + @Test + fun skippingPastTheBlockAfterConsumingKeepsGoing() { + seedAlbum(3, sourceId = 1) + seedSong("x1", "al2") + seedSong("x2", "al2") + + controller.playAlbum(1, "al1", 0) + await { + controller.state.value.item + ?.id == "s1" + } + controller.addToQueue(1, QueueKind.Song, "x1") + controller.addToQueue(1, QueueKind.Song, "x2") + + controller.playAt(2) + await { + controller.state.value.item + ?.id == "x2" + } + controller.next() + await { + controller.state.value.item + ?.id == "s2" + } + + controller.next() + await { + controller.state.value.item + ?.id == "s3" + } + } + + @Test + fun playNextFromInsideTheBlockGoesAfterTheCurrentTrack() { + seedAlbum(3, sourceId = 1) + seedSong("x1", "al2") + seedSong("x2", "al2") + seedSong("x3", "al2") + + controller.playAlbum(1, "al1", 0) + await { + controller.state.value.item + ?.id == "s1" + } + controller.addToQueue(1, QueueKind.Song, "x1") + controller.addToQueue(1, QueueKind.Song, "x2") + controller.playAt(2) + await { + controller.state.value.item + ?.id == "x2" + } + + controller.playNext(1, QueueKind.Song, "x3") + await { + controller.state.value.item + ?.id == "x2" + } + await { runBlocking { controller.upcomingItem()?.id } == "x3" } + + controller.next() + await { + controller.state.value.item + ?.id == "x3" + } + } + + @Test + fun removingAContextTrackUnderTheBlockKeepsTheCursorOnThePlayingTrack() { + 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 { runBlocking { controller.upcomingItem()?.id } == "x1" } + controller.next() + await { + controller.state.value.item + ?.id == "x1" + } + + runBlocking { controller.removeAt(0) } + await { + controller.state.value.item + ?.id == "x1" + } + + assertEquals(1L, runBlocking { queues.cursor() }) + } + + @Test + fun restoringKeepsTheSavedPositionWhenTheAnchorMoves() { + seedAlbum(3, sourceId = 1) + seedSong("x1", "al2") + seedSong("x2", "al2") + + controller.playAlbum(1, "al1", 0) + await { + controller.state.value.item + ?.id == "s1" + } + controller.addToQueue(1, QueueKind.Song, "x1") + controller.addToQueue(1, QueueKind.Song, "x2") + controller.playAt(2) + await { + controller.state.value.item + ?.id == "x2" + } + controller.next() + await { + controller.state.value.item + ?.id == "s2" + } + + runBlocking { queues.setPosition(12_000L) } + + val restoredHandle = FakePlayerHandle() + val restored = PlaybackController(sources, queues, FakePlayerConnection(restoredHandle), dispatcher = dispatcher) + restored.connect() + await { + restored.state.value.item + ?.id == "s2" + } + + assertEquals(12_000L, restored.positionMs.value) + restored.close() + } + + @Test + fun nextOnTheLastQueuedTrackDoesNotStrandIt() { + seedAlbum(3, 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.next() + await { + controller.state.value.item + ?.id == "x1" + } + + controller.next() + Thread.sleep(200) + + assertFalse(runBlocking { queues.snapshot().upNext }.isEmpty()) + assertEquals( + "x1", + controller.state.value.item + ?.id, + ) + } + + @Test + fun reanchoringBumpsTheLayoutVersion() { + seedAlbum(4, sourceId = 1) + seedSong("x1", "al2") + + controller.playAlbum(1, "al1", 1) + await { + controller.state.value.item + ?.id == "s2" + } + controller.addToQueue(1, QueueKind.Song, "x1") + await { runBlocking { controller.upcomingItem()?.id } == "x1" } + val before = controller.state.value.layout + + controller.playAt(4) + await { + controller.state.value.item + ?.id == "s4" + } + + assertTrue(controller.state.value.layout > before) + } + @Test fun theDurationUsesTheNextTracksMetadataWhileItLoads() { seedAlbum(3, sourceId = 1)