stop locating the playing queue entry by song id

The queue can hold the same song more than once (a song in both the context
and the up-next block, a duplicate album track, ...), so resolving the
playing entry by song id could match the wrong copy.

- `PlaybackController.move` and `removeAt` now derive the current track's
  new position from the edit (block index / context play, read back through
  the updated snapshot so a moved anchor is accounted for) instead of
  `combinedIndexOf`/`combinedContextIndexOf`.
- the queue view's playing highlight compares the row position to the
  playback position instead of the song id, so duplicates no longer both
  light up.

Regression tests cover a move and a remove with a duplicate song in the
context; both fail without the change.

Still id-based, deliberately: `undo`'s canMirror guard (already position-
guarded), the shuffle-state-changed branch of `shiftWindowLocked` (rare,
needs the id to find the track after an arbitrary reorder) and the view's
LazyColumn keys (cosmetic only).
This commit is contained in:
austinried
2026-10-04 00:30:01 +00:00
parent 902f556fe6
commit e142148b5a
4 changed files with 122 additions and 45 deletions
@@ -331,9 +331,13 @@ class PlaybackController(
lastEdit = ReloadUndo(entries, cursor, snapshot.shuffleSeed, upNext, snapshot.upNextAnchor) lastEdit = ReloadUndo(entries, cursor, snapshot.shuffleSeed, upNext, snapshot.upNextAnchor)
return@withLock return@withLock
} }
val currentId = player.currentItem?.id // Derive the current track's new position from the edit. Resolving its song id after
val fallback = // the edit can match an identical song elsewhere (a duplicate in the context or the
if (snapshot.shuffled && !wasUpNext) { // block), so use positions throughout; a context removal can move the anchor, so read
// the combined position back from the updated snapshot.
var target =
when {
position == current && snapshot.shuffled && !wasUpNext -> {
// The permutation is re-derived at the smaller size, so there is no stable // The permutation is re-derived at the smaller size, so there is no stable
// "next"; play whatever now fills the removed track's slot. // "next"; play whatever now fills the removed track's slot.
val seed = snapshot.shuffleSeed!! val seed = snapshot.shuffleSeed!!
@@ -345,29 +349,26 @@ class PlaybackController(
val slot = removedFlat.coerceIn(0, newContextSize - 1) val slot = removedFlat.coerceIn(0, newContextSize - 1)
updated.combined(Shuffle.toSequence(seed, newContextSize, slot)) updated.combined(Shuffle.toSequence(seed, newContextSize, slot))
} }
} else {
(if (position < current) current - 1 else current)
} }
var target =
( position == current -> {
if (position == current) { 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
wasUpNext -> {
val blockIndex = snapshot.upNextIndex(current)
val newIndex =
if (snapshot.isUpNext(position) && position < current) blockIndex - 1 else blockIndex
updated.anchorPlay + 1 + newIndex
} }
).coerceIn(0, updated.size - 1)
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 // 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 // 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. // still play next rather than being stranded behind the cursor.
@@ -439,13 +440,30 @@ class PlaybackController(
if (!queueRepository.move(snapshot, from, to)) return@withLock if (!queueRepository.move(snapshot, from, to)) return@withLock
val updated = readSnapshot() val updated = readSnapshot()
this.snapshot = updated 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 = val target =
( if (wasUpNext) {
currentId?.let { val blockIndex = snapshot.upNextIndex(current)
if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it) val newIndex =
} ?: movedCursor(current, from, to) if (snapshot.isUpNext(from) && snapshot.isUpNext(to)) {
).coerceIn(0, updated.size - 1) 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 lastPosition = target
windowJob?.cancel() windowJob?.cancel()
val fromInWindow = from in windowStart..windowEnd val fromInWindow = from in windowStart..windowEnd
@@ -284,7 +284,7 @@ fun QueueRoute(
ready = viewModel.ready, ready = viewModel.ready,
initialIndex = viewModel.initialIndex, initialIndex = viewModel.initialIndex,
generation = viewModel.generation, generation = viewModel.generation,
currentSongId = playback.item?.id, currentPosition = playback.position,
shuffle = playback.shuffle, shuffle = playback.shuffle,
contextTitle = sourceTitle.value, contextTitle = sourceTitle.value,
onClearUpNext = controller::clearUpNext, onClearUpNext = controller::clearUpNext,
@@ -306,7 +306,7 @@ fun QueueRoute(
fun QueueScreen( fun QueueScreen(
rows: List<QueueRow>, rows: List<QueueRow>,
ready: Boolean, ready: Boolean,
currentSongId: String?, currentPosition: Long?,
coverArt: (String?, Boolean) -> CoverArtRef?, coverArt: (String?, Boolean) -> CoverArtRef?,
onBack: () -> Unit, onBack: () -> Unit,
onPlay: (Long) -> Unit, onPlay: (Long) -> Unit,
@@ -434,7 +434,7 @@ fun QueueScreen(
) { isDragging -> ) { isDragging ->
QueueRowItem( QueueRowItem(
row = row, row = row,
isPlaying = row.song.song.id == currentSongId, isPlaying = row.position == currentPosition,
floating = isDragging, floating = isDragging,
coverArt = coverArt, coverArt = coverArt,
dragHandle = dragHandle =
@@ -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 @Test
fun playNextFromInsideTheBlockGoesAfterTheCurrentTrack() { fun playNextFromInsideTheBlockGoesAfterTheCurrentTrack() {
seedAlbum(3, sourceId = 1) 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 @Test
fun removingThePlayingTrackWhileShuffledLeavesTheBlockReachable() { fun removingThePlayingTrackWhileShuffledLeavesTheBlockReachable() {
seedAlbum(6, sourceId = 1) seedAlbum(6, sourceId = 1)
@@ -415,7 +415,7 @@ class AppScreenshotTest {
rows = rows =
Fixtures.songItems.mapIndexed { index, item -> QueueRow(index.toLong(), index.toLong(), item) }, Fixtures.songItems.mapIndexed { index, item -> QueueRow(index.toLong(), index.toLong(), item) },
ready = true, ready = true,
currentSongId = Fixtures.songItems[1].song.id, currentPosition = 1L,
coverArt = { id, _ -> id?.let { CoverArtRef(it, "test:$it") } }, coverArt = { id, _ -> id?.let { CoverArtRef(it, "test:$it") } },
onBack = {}, onBack = {},
onPlay = {}, onPlay = {},