mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-10 23:21:59 +01:00
keep the up-next block reachable when the playing context track is removed while shuffled
Review found that removing the playing context track while shuffled with a
non-empty up-next block stranded the block behind the cursor: the
repository moves `upNextAnchor` to the preceding track, but the cursor's
re-derived slot position can land after the block, so the queued tracks
were skipped. Re-anchor the block to the track the cursor lands on, same
as `playAt`/`toggleShuffle`.
- regression test loops the sequence over fresh random seeds and asserts
the block stays reachable; fails without the fix ("stranded behind the
cursor").
- `removingTheCurrentTrackWhileShuffled...` now asserts the canonical slot
the current track occupies instead of recomputing the production position
formula (which only checked the implementation against itself).
- `QueueRepository.removeAt` returns Unit; every caller discarded the
snapshot and re-read it, so it no longer builds one.
This commit is contained in:
@@ -384,17 +384,17 @@ class QueueRepository(
|
|||||||
suspend fun removeAt(
|
suspend fun removeAt(
|
||||||
snapshot: QueueSnapshot,
|
snapshot: QueueSnapshot,
|
||||||
position: Long,
|
position: Long,
|
||||||
): QueueSnapshot {
|
) {
|
||||||
if (snapshot.isUpNext(position)) {
|
if (snapshot.isUpNext(position)) {
|
||||||
removeUpNextAt(snapshot, snapshot.upNextIndex(position))
|
removeUpNextAt(snapshot, snapshot.upNextIndex(position))
|
||||||
return snapshot()
|
return
|
||||||
}
|
}
|
||||||
val play = snapshot.contextPlay(position)
|
val play = snapshot.contextPlay(position)
|
||||||
if (snapshot.shuffled) {
|
if (snapshot.shuffled) {
|
||||||
// The permutation is derived from (seed, size), so removing a track re-derives it at
|
// The permutation is derived from (seed, size), so removing a track re-derives it at
|
||||||
// the smaller size; keep the same seed and store the new size so the queue stays
|
// the smaller size; keep the same seed and store the new size so the queue stays
|
||||||
// shuffled. There is no stable "next" to preserve here.
|
// shuffled. There is no stable "next" to preserve here.
|
||||||
val flat = snapshot.flatContext(play) ?: return snapshot
|
val flat = snapshot.flatContext(play) ?: return
|
||||||
write(compact(removeEntry(snapshot.entries, flat)).map { it.entry })
|
write(compact(removeEntry(snapshot.entries, flat)).map { it.entry })
|
||||||
adjustAnchorOnRemove(flat)
|
adjustAnchorOnRemove(flat)
|
||||||
setShuffleSize(contextSize())
|
setShuffleSize(contextSize())
|
||||||
@@ -403,7 +403,6 @@ class QueueRepository(
|
|||||||
write(removed.map { it.entry })
|
write(removed.map { it.entry })
|
||||||
adjustAnchorOnRemove(play)
|
adjustAnchorOnRemove(play)
|
||||||
}
|
}
|
||||||
return snapshot()
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private suspend fun adjustAnchorOnRemove(removedFlat: Long) =
|
private suspend fun adjustAnchorOnRemove(removedFlat: Long) =
|
||||||
|
|||||||
@@ -323,7 +323,7 @@ class PlaybackController(
|
|||||||
val wasUpNext = snapshot.isUpNext(current)
|
val wasUpNext = snapshot.isUpNext(current)
|
||||||
val removed = if (position == current) null else queueRepository.itemAt(snapshot, position)?.toQueueItem()
|
val removed = if (position == current) null else queueRepository.itemAt(snapshot, position)?.toQueueItem()
|
||||||
queueRepository.removeAt(snapshot, position)
|
queueRepository.removeAt(snapshot, position)
|
||||||
val updated = readSnapshot()
|
var updated = readSnapshot()
|
||||||
this.snapshot = updated
|
this.snapshot = updated
|
||||||
windowJob?.cancel()
|
windowJob?.cancel()
|
||||||
if (updated.size == 0L) {
|
if (updated.size == 0L) {
|
||||||
@@ -348,12 +348,23 @@ class PlaybackController(
|
|||||||
} else {
|
} else {
|
||||||
(if (position < current) current - 1 else current)
|
(if (position < current) current - 1 else current)
|
||||||
}
|
}
|
||||||
val target =
|
var target =
|
||||||
(
|
(
|
||||||
currentId?.let {
|
currentId?.let {
|
||||||
if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it)
|
if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it)
|
||||||
} ?: fallback
|
} ?: fallback
|
||||||
).coerceIn(0, updated.size - 1)
|
).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.
|
||||||
|
if (snapshot.shuffled && !wasUpNext && position == current && updated.upNextSize > 0L) {
|
||||||
|
val reanchored = queueRepository.reanchorFor(updated, target)
|
||||||
|
if (reanchored.changed) {
|
||||||
|
updated = readSnapshot()
|
||||||
|
this.snapshot = updated
|
||||||
|
target = reanchored.position
|
||||||
|
}
|
||||||
|
}
|
||||||
lastPosition = target
|
lastPosition = target
|
||||||
lastEdit =
|
lastEdit =
|
||||||
if (removed == null) {
|
if (removed == null) {
|
||||||
|
|||||||
@@ -17,7 +17,6 @@ import com.subtracks.data.prefs.StreamQuality
|
|||||||
import com.subtracks.data.prefs.UserPreferences
|
import com.subtracks.data.prefs.UserPreferences
|
||||||
import com.subtracks.data.prefs.fakeUserPreferences
|
import com.subtracks.data.prefs.fakeUserPreferences
|
||||||
import com.subtracks.data.repo.QueueRepository
|
import com.subtracks.data.repo.QueueRepository
|
||||||
import com.subtracks.data.repo.Shuffle
|
|
||||||
import com.subtracks.data.repo.SourceRepository
|
import com.subtracks.data.repo.SourceRepository
|
||||||
import kotlinx.coroutines.Dispatchers
|
import kotlinx.coroutines.Dispatchers
|
||||||
import kotlinx.coroutines.asCoroutineDispatcher
|
import kotlinx.coroutines.asCoroutineDispatcher
|
||||||
@@ -1595,17 +1594,55 @@ class PlaybackControllerTest {
|
|||||||
|
|
||||||
val after = runBlocking { queues.snapshot() }
|
val after = runBlocking { queues.snapshot() }
|
||||||
val newContextSize = (before.contextSize - 1).coerceAtLeast(0)
|
val newContextSize = (before.contextSize - 1).coerceAtLeast(0)
|
||||||
val slot = removedFlat.coerceIn(0, (newContextSize - 1).coerceAtLeast(0))
|
val slot = removedFlat.coerceIn(0, newContextSize - 1)
|
||||||
val expected = after.combined(Shuffle.toSequence(before.shuffleSeed!!, newContextSize, slot))
|
|
||||||
assertEquals(5L, after.size)
|
assertEquals(5L, after.size)
|
||||||
assertEquals(expected, controller.state.value.position)
|
|
||||||
assertEquals(
|
assertEquals(
|
||||||
runBlocking { queues.itemAt(after, expected)?.song?.id },
|
// The song now occupying the removed track's canonical slot is the one playing.
|
||||||
|
slot,
|
||||||
|
after.flatContext(after.contextPlay(controller.state.value.position!!)),
|
||||||
|
)
|
||||||
|
assertEquals(
|
||||||
|
runBlocking { queues.itemAt(after, controller.state.value.position!!)?.song?.id },
|
||||||
controller.state.value.item
|
controller.state.value.item
|
||||||
?.id,
|
?.id,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun removingThePlayingTrackWhileShuffledLeavesTheBlockReachable() {
|
||||||
|
seedAlbum(6, sourceId = 1)
|
||||||
|
seedSong("x1", "al2")
|
||||||
|
|
||||||
|
repeat(30) {
|
||||||
|
if (controller.state.value.shuffle) {
|
||||||
|
controller.toggleShuffle()
|
||||||
|
await { !controller.state.value.shuffle }
|
||||||
|
}
|
||||||
|
runBlocking { controller.clearUpNext() }
|
||||||
|
controller.playAlbum(1, "al1", 2)
|
||||||
|
await {
|
||||||
|
controller.state.value.item
|
||||||
|
?.id == "s3" && !controller.state.value.shuffle
|
||||||
|
}
|
||||||
|
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) }
|
||||||
|
|
||||||
|
val snapshot = runBlocking { queues.snapshot() }
|
||||||
|
val position = controller.state.value.position!!
|
||||||
|
assertTrue(
|
||||||
|
"queued track stranded behind the cursor at $position (block at ${snapshot.anchorPlay})",
|
||||||
|
position <= snapshot.anchorPlay || snapshot.isUpNext(position),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun movingContextTracksWhileShuffledIsIgnored() {
|
fun movingContextTracksWhileShuffledIsIgnored() {
|
||||||
seedAlbum(6, sourceId = 1)
|
seedAlbum(6, sourceId = 1)
|
||||||
|
|||||||
Reference in New Issue
Block a user