mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-11 07:29:14 +01:00
address review: anchor relocation and empty-context positions
- clamp anchorPlay to the context size, so an empty context with a queued block puts the block at combined 0 (previously position 0 was unresolvable and the queue view emptied). - move/removeAt now derive the playing track's new position from its id after an edit, instead of assuming a fixed list delta; moving or removing the anchor relocates the whole block, which movedCursor ignored. - removing the currently playing track reloads at the model's next position (the anchor may have relocated) rather than removing it from the player in place. - the queue view's remove reconciles its rows from the repository instead of decrementing positions by one, so a block relocation stays in sync.
This commit is contained in:
@@ -58,10 +58,12 @@ data class QueueSnapshot(
|
|||||||
val shuffled: Boolean get() = shuffleOrder != null
|
val shuffled: Boolean get() = shuffleOrder != null
|
||||||
|
|
||||||
val anchorPlay: Long =
|
val anchorPlay: Long =
|
||||||
|
(
|
||||||
when (val order = shuffleOrder) {
|
when (val order = shuffleOrder) {
|
||||||
null -> upNextAnchor
|
null -> upNextAnchor
|
||||||
else -> order.indexOf(upNextAnchor).takeIf { it >= 0 }?.toLong() ?: 0L
|
else -> order.indexOf(upNextAnchor).takeIf { it >= 0 }?.toLong() ?: 0L
|
||||||
}
|
}
|
||||||
|
).coerceAtMost(contextSize - 1L)
|
||||||
|
|
||||||
fun isUpNext(position: Long): Boolean = upNextSize > 0L && position > anchorPlay && position <= anchorPlay + upNextSize
|
fun isUpNext(position: Long): Boolean = upNextSize > 0L && position > anchorPlay && position <= anchorPlay + upNextSize
|
||||||
|
|
||||||
|
|||||||
@@ -349,15 +349,8 @@ class PlaybackController(
|
|||||||
if (position != current) {
|
if (position != current) {
|
||||||
rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target)
|
rebuildWindow(queueRepository.window(updated, target, QUEUE_WINDOW_RADIUS), target)
|
||||||
refresh(target)
|
refresh(target)
|
||||||
} else if (target - windowStart in 0 until player.itemCount) {
|
|
||||||
updating = true
|
|
||||||
player.removeAt((target - windowStart).toInt())
|
|
||||||
updating = false
|
|
||||||
windowEnd--
|
|
||||||
shiftWindowLocked(target)
|
|
||||||
refresh(target)
|
|
||||||
} else {
|
} else {
|
||||||
loadWindow(target, autoplay = playing)
|
loadWindow(target, autoplay = playing, startPositionMs = 0)
|
||||||
}
|
}
|
||||||
return@withLock
|
return@withLock
|
||||||
}
|
}
|
||||||
@@ -400,10 +393,17 @@ class PlaybackController(
|
|||||||
val cursor = queueRepository.cursor()
|
val cursor = queueRepository.cursor()
|
||||||
val entries = snapshot.entries.map { it.entry }
|
val entries = snapshot.entries.map { it.entry }
|
||||||
val upNext = snapshot.upNext.map { it.entry }
|
val upNext = snapshot.upNext.map { it.entry }
|
||||||
|
val wasUpNext = snapshot.isUpNext(current)
|
||||||
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 target = movedCursor(current, from, to)
|
val currentId = player.currentItem?.id
|
||||||
|
val target =
|
||||||
|
(
|
||||||
|
currentId?.let {
|
||||||
|
if (wasUpNext) queueRepository.combinedIndexOf(updated, it) else queueRepository.combinedContextIndexOf(updated, it)
|
||||||
|
} ?: movedCursor(current, from, to)
|
||||||
|
).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
|
||||||
|
|||||||
@@ -134,13 +134,15 @@ class QueueViewModel(
|
|||||||
}
|
}
|
||||||
|
|
||||||
fun reconcile() {
|
fun reconcile() {
|
||||||
viewModelScope.launch {
|
viewModelScope.launch { mutex.withLock { reconcileLocked() } }
|
||||||
mutex.withLock {
|
}
|
||||||
if (!ready) return@withLock
|
|
||||||
|
private suspend fun reconcileLocked() {
|
||||||
|
if (!ready) return
|
||||||
val snapshot = queueRepository.snapshot()
|
val snapshot = queueRepository.snapshot()
|
||||||
if (snapshot.size == 0L) {
|
if (snapshot.size == 0L) {
|
||||||
rows.clear()
|
rows.clear()
|
||||||
return@withLock
|
return
|
||||||
}
|
}
|
||||||
val start = (rows.firstOrNull()?.position ?: 0L).coerceIn(0, snapshot.size - 1)
|
val start = (rows.firstOrNull()?.position ?: 0L).coerceIn(0, snapshot.size - 1)
|
||||||
val end = (start + rows.size.coerceAtLeast(QUEUE_CHUNK) - 1).coerceAtMost(snapshot.size - 1)
|
val end = (start + rows.size.coerceAtLeast(QUEUE_CHUNK) - 1).coerceAtMost(snapshot.size - 1)
|
||||||
@@ -154,8 +156,6 @@ class QueueViewModel(
|
|||||||
rows.addAll(updated)
|
rows.addAll(updated)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
fun loadOlder() {
|
fun loadOlder() {
|
||||||
val top = rows.firstOrNull()?.position ?: return
|
val top = rows.firstOrNull()?.position ?: return
|
||||||
@@ -228,24 +228,9 @@ class QueueViewModel(
|
|||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
mutex.withLock {
|
mutex.withLock {
|
||||||
val index = rows.indexOfFirst { it.position == position }
|
val index = rows.indexOfFirst { it.position == position }
|
||||||
if (index >= 0) {
|
if (index >= 0) rows.removeAt(index)
|
||||||
rows.removeAt(index)
|
|
||||||
for (i in index until rows.size) {
|
|
||||||
val row = rows[i]
|
|
||||||
rows[i] = row.copy(position = row.position - 1)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
playbackController.removeAt(position)
|
playbackController.removeAt(position)
|
||||||
val snapshot = queueRepository.snapshot()
|
reconcileLocked()
|
||||||
if (rows.isEmpty()) {
|
|
||||||
reload()
|
|
||||||
return@withLock
|
|
||||||
}
|
|
||||||
val next = rows.last().position + 1
|
|
||||||
if (next <= snapshot.size - 1) {
|
|
||||||
queueRepository.range(snapshot, next, next).firstOrNull()?.let { rows.add(newRow(it)) }
|
|
||||||
}
|
|
||||||
while (rows.size > QUEUE_WINDOW_ROWS) rows.removeAt(0)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -926,6 +926,81 @@ class PlaybackControllerTest {
|
|||||||
assertFalse(handle.operations.any { it.startsWith("setWindow") })
|
assertFalse(handle.operations.any { it.startsWith("setWindow") })
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun removingTheLastContextTrackWithAQueuedTrackKeepsTheQueue() {
|
||||||
|
seedAlbum(1, 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" }
|
||||||
|
|
||||||
|
runBlocking { controller.removeAt(0) }
|
||||||
|
await {
|
||||||
|
controller.state.value.item
|
||||||
|
?.id == "x1"
|
||||||
|
}
|
||||||
|
|
||||||
|
val snapshot = runBlocking { queues.snapshot() }
|
||||||
|
assertEquals(1L, snapshot.size)
|
||||||
|
assertTrue(runBlocking { queues.range(snapshot, 0, 0) }.isNotEmpty())
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun movingTheAnchorKeepsThePlayingTrackPositionInSync() {
|
||||||
|
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" }
|
||||||
|
controller.next()
|
||||||
|
await {
|
||||||
|
controller.state.value.item
|
||||||
|
?.id == "x1"
|
||||||
|
}
|
||||||
|
|
||||||
|
runBlocking { controller.move(1, 0) }
|
||||||
|
await {
|
||||||
|
controller.state.value.item
|
||||||
|
?.id == "x1"
|
||||||
|
}
|
||||||
|
|
||||||
|
val snapshot = runBlocking { queues.snapshot() }
|
||||||
|
assertEquals(runBlocking { queues.combinedIndexOf(snapshot, "x1") }, controller.state.value.position)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun removingThePlayingAnchorTrackPlaysTheModelNextTrack() {
|
||||||
|
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" }
|
||||||
|
|
||||||
|
runBlocking { controller.removeAt(1) }
|
||||||
|
await {
|
||||||
|
controller.state.value.item
|
||||||
|
?.id == "s3"
|
||||||
|
}
|
||||||
|
|
||||||
|
val snapshot = runBlocking { queues.snapshot() }
|
||||||
|
assertEquals(runBlocking { queues.combinedIndexOf(snapshot, "s3") }, controller.state.value.position)
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun nextFollowsTheReorderedQueue() {
|
fun nextFollowsTheReorderedQueue() {
|
||||||
seedAlbum(3, sourceId = 1)
|
seedAlbum(3, sourceId = 1)
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver
|
|||||||
import androidx.test.core.app.ApplicationProvider
|
import androidx.test.core.app.ApplicationProvider
|
||||||
import androidx.test.ext.junit.runners.AndroidJUnit4
|
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||||
import com.subtracks.data.db.SubtracksDatabase
|
import com.subtracks.data.db.SubtracksDatabase
|
||||||
|
import com.subtracks.data.model.QueueKind
|
||||||
import com.subtracks.data.model.Song
|
import com.subtracks.data.model.Song
|
||||||
import com.subtracks.data.model.SongListItem
|
import com.subtracks.data.model.SongListItem
|
||||||
import com.subtracks.data.model.Source
|
import com.subtracks.data.model.Source
|
||||||
@@ -225,6 +226,37 @@ class QueueViewModelTest {
|
|||||||
assertEquals((220L..399L).toList(), viewModel.rows.map { it.position })
|
assertEquals((220L..399L).toList(), viewModel.rows.map { it.position })
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun removingATrackUnderTheBlockReloadsRows() {
|
||||||
|
runBlocking { seedSongs(10) }
|
||||||
|
runBlocking {
|
||||||
|
db.libraryDao().upsertSongs(
|
||||||
|
listOf(Song(1, "x1", "al2", "ar1", "X", "Other Album", "Artist", 100, 1, 1, null, null)),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
controller.playAlbum(1, "al1", 2)
|
||||||
|
await { controller.state.value.position == 2L }
|
||||||
|
controller.addToQueue(1, QueueKind.Song, "x1")
|
||||||
|
await { runBlocking { controller.upcomingItem()?.id } == "x1" }
|
||||||
|
|
||||||
|
viewModel.open()
|
||||||
|
await { viewModel.ready }
|
||||||
|
|
||||||
|
viewModel.remove(2)
|
||||||
|
await {
|
||||||
|
val positions = viewModel.rows.map { it.position }
|
||||||
|
positions.isNotEmpty() && positions == (positions.first()..positions.last()).toList()
|
||||||
|
}
|
||||||
|
|
||||||
|
val snapshot = runBlocking { queues.snapshot() }
|
||||||
|
val expected =
|
||||||
|
runBlocking {
|
||||||
|
(viewModel.rows.first().position..viewModel.rows.last().position)
|
||||||
|
.mapNotNull { queues.itemAt(snapshot, it)?.song?.id }
|
||||||
|
}
|
||||||
|
assertEquals(expected, viewModel.rows.map { it.song.song.id })
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun removeAndUndoKeepTheBoundedWindowConsistent() {
|
fun removeAndUndoKeepTheBoundedWindowConsistent() {
|
||||||
runBlocking { seedSongs(400) }
|
runBlocking { seedSongs(400) }
|
||||||
|
|||||||
Reference in New Issue
Block a user