diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 157f4ada..5ad3b3bc 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -331,43 +331,44 @@ class PlaybackController( lastEdit = ReloadUndo(entries, cursor, snapshot.shuffleSeed, upNext, snapshot.upNextAnchor) return@withLock } - val currentId = player.currentItem?.id - val fallback = - if (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 seed = snapshot.shuffleSeed!! - 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(Shuffle.toSequence(seed, newContextSize, slot)) - } - } else { - (if (position < current) current - 1 else current) - } + // 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 positions throughout; a context removal can move the anchor, so read + // the combined position back from the updated snapshot. var target = - ( - if (position == current) { - // The playing entry was removed; resolving its song id afterwards can match - // an identical song elsewhere (a duplicate in the context or the block), so - // advance from the fallback instead. - fallback - } else { - currentId?.let { - if (wasUpNext) { - queueRepository.combinedIndexOf( - updated, - it, - ) - } else { - queueRepository.combinedContextIndexOf(updated, it) - } - } ?: fallback + 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 seed = snapshot.shuffleSeed!! + 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(Shuffle.toSequence(seed, newContextSize, slot)) + } } - ).coerceIn(0, updated.size - 1) + + position == current -> { + current + } + + wasUpNext -> { + val blockIndex = snapshot.upNextIndex(current) + val newIndex = + if (snapshot.isUpNext(position) && position < current) blockIndex - 1 else blockIndex + updated.anchorPlay + 1 + newIndex + } + + else -> { + val play = snapshot.contextPlay(current) + val newPlay = + if (!snapshot.isUpNext(position) && snapshot.contextPlay(position) < play) play - 1 else play + updated.combined(newPlay) + } + }.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. @@ -439,13 +440,30 @@ class PlaybackController( if (!queueRepository.move(snapshot, from, to)) return@withLock val updated = readSnapshot() this.snapshot = updated - val currentId = player.currentItem?.id + // The current track still exists, so derive its new position from the edit instead of + // resolving its song id, which is ambiguous when the same song appears more than once. + // A context move can also shift the anchor (and with it an up-next block), so read the + // position back from the updated snapshot rather than only shifting `current`. val target = - ( - currentId?.let { - if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it) - } ?: movedCursor(current, from, to) - ).coerceIn(0, updated.size - 1) + if (wasUpNext) { + val blockIndex = snapshot.upNextIndex(current) + val newIndex = + if (snapshot.isUpNext(from) && snapshot.isUpNext(to)) { + movedCursor(blockIndex, snapshot.upNextIndex(from), snapshot.upNextIndex(to)) + } else { + blockIndex + } + updated.anchorPlay + 1 + newIndex + } else { + val play = snapshot.contextPlay(current) + val newPlay = + if (!snapshot.isUpNext(from) && !snapshot.isUpNext(to)) { + movedCursor(play, snapshot.contextPlay(from), snapshot.contextPlay(to)) + } else { + play + } + updated.combined(newPlay) + }.coerceIn(0, updated.size - 1) lastPosition = target windowJob?.cancel() val fromInWindow = from in windowStart..windowEnd 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 cccd0281..20db62c3 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -284,7 +284,7 @@ fun QueueRoute( ready = viewModel.ready, initialIndex = viewModel.initialIndex, generation = viewModel.generation, - currentSongId = playback.item?.id, + currentPosition = playback.position, shuffle = playback.shuffle, contextTitle = sourceTitle.value, onClearUpNext = controller::clearUpNext, @@ -306,7 +306,7 @@ fun QueueRoute( fun QueueScreen( rows: List, ready: Boolean, - currentSongId: String?, + currentPosition: Long?, coverArt: (String?, Boolean) -> CoverArtRef?, onBack: () -> Unit, onPlay: (Long) -> Unit, @@ -434,7 +434,7 @@ fun QueueScreen( ) { isDragging -> QueueRowItem( row = row, - isPlaying = row.song.song.id == currentSongId, + isPlaying = row.position == currentPosition, floating = isDragging, coverArt = coverArt, dragHandle = diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index b15d439f..3f8cb471 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -490,6 +490,42 @@ class PlaybackControllerTest { } } + @Test + fun movingWithADuplicateInTheContextKeepsThePlayingCopy() { + seedAlbum(4, sourceId = 1) + runBlocking { queues.replace(listOf(queues.albumEntry(1, "al1"), queues.songEntry(1, "s2"))) } + controller.connect() + await { controller.state.value.item != null } + // Context is s1,s2,s3,s4,s2; play the second copy. + controller.playAt(4) + await { controller.state.value.position == 4L } + + runBlocking { controller.move(0, 3) } + + val snapshot = runBlocking { queues.snapshot() } + val playing = controller.state.value.position!! + assertTrue(runBlocking { queues.itemAt(snapshot, playing)?.song?.id } == "s2") + // Still the second copy (flat index 4), not the first. + assertEquals(4L, snapshot.flatContext(snapshot.contextPlay(playing))) + } + + @Test + fun removingAnotherTrackWithADuplicateInTheContextKeepsThePlayingCopy() { + seedAlbum(4, sourceId = 1) + runBlocking { queues.replace(listOf(queues.albumEntry(1, "al1"), queues.songEntry(1, "s2"))) } + controller.connect() + await { controller.state.value.item != null } + controller.playAt(4) + await { controller.state.value.position == 4L } + + runBlocking { controller.removeAt(0) } + + val snapshot = runBlocking { queues.snapshot() } + val playing = controller.state.value.position!! + assertTrue(runBlocking { queues.itemAt(snapshot, playing)?.song?.id } == "s2") + assertEquals(3L, snapshot.flatContext(snapshot.contextPlay(playing))) + } + @Test fun playNextFromInsideTheBlockGoesAfterTheCurrentTrack() { seedAlbum(3, sourceId = 1) @@ -1637,6 +1673,29 @@ class PlaybackControllerTest { ) } + @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) diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index fa8ff2f6..962f74b6 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -415,7 +415,7 @@ class AppScreenshotTest { rows = Fixtures.songItems.mapIndexed { index, item -> QueueRow(index.toLong(), index.toLong(), item) }, ready = true, - currentSongId = Fixtures.songItems[1].song.id, + currentPosition = 1L, coverArt = { id, _ -> id?.let { CoverArtRef(it, "test:$it") } }, onBack = {}, onPlay = {},