From 64ec3265b7ee066729969c5a12503ef0ec6c9a82 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 12 Nov 2026 08:09:56 +0000 Subject: [PATCH] ui: reject stale and mixed-generation queue edits --- .../com/subtracks/ui/playback/QueueScreen.kt | 51 +++++-- .../com/subtracks/ui/AppScreenshotTest.kt | 2 +- .../ui/playback/QueueViewModelTest.kt | 131 +++++++++++++++++- 3 files changed, 161 insertions(+), 23 deletions(-) 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 80c57015..0c766863 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -87,6 +87,7 @@ data class QueueRow( val position: Long, val song: SongItem, val upNext: Boolean = false, + val resetVersion: Long = 0, ) // Both must match: the position alone can transiently point at the row above while the list @@ -118,7 +119,10 @@ class QueueViewModel( private var nextId = 0L private val mutex = Mutex() - private fun newRow(item: QueueWindowItem) = QueueRow(nextId++, item.position, item.item, item.upNext) + private fun newRow( + item: QueueWindowItem, + resetVersion: Long, + ) = QueueRow(nextId++, item.position, item.item, item.upNext, resetVersion) private fun visible(item: QueueWindowItem): Boolean = !sourceRepository.offline.value || downloads.localUri(item.item.song.id) != null @@ -128,10 +132,11 @@ class QueueViewModel( private fun rowFor( item: QueueWindowItem, reused: Map>, + resetVersion: Long, ): QueueRow { val ids = reused[item.item.song.id] val id = if (!ids.isNullOrEmpty()) ids.removeAt(0) else nextId++ - return QueueRow(id, item.position, item.item, item.upNext) + return QueueRow(id, item.position, item.item, item.upNext, resetVersion) } fun open() { @@ -147,7 +152,9 @@ class QueueViewModel( }?.coerceIn(0, snapshot.size - 1) ?: 0L val start = (cursor - OLDER_LOAD).coerceAtLeast(0) val end = (start + QUEUE_CHUNK - 1).coerceAtMost(snapshot.size - 1) - rows.addAll(queueRepository.range(snapshot, start, end).filter(::visible).map { rowFor(it, reused) }) + rows.addAll( + queueRepository.range(snapshot, start, end).filter(::visible).map { rowFor(it, reused, snapshot.resetVersion) }, + ) initialIndex = rows.indexOfFirst { it.position == cursor }.coerceAtLeast(0) } ready = true @@ -172,7 +179,7 @@ class QueueViewModel( val end = rawEnd.coerceAtMost(start + MAX_SCAN_POSITIONS - 1) val loaded = queueRepository.range(snapshot, start, end).filter(::visible) val reused = reusedIds() - val updated = loaded.map { rowFor(it, reused) } + val updated = loaded.map { rowFor(it, reused, snapshot.resetVersion) } if (end >= rawEnd) { if (updated.size == rows.size) { updated.forEachIndexed { index, row -> if (rows[index] != row) rows[index] = row } @@ -207,7 +214,7 @@ class QueueViewModel( end = start - 1 } if (collected.isEmpty()) return@withLock - rows.addAll(0, collected.map(::newRow)) + rows.addAll(0, collected.map { newRow(it, snapshot.resetVersion) }) while (rows.size > QUEUE_WINDOW_ROWS) rows.removeAt(rows.size - 1) } } @@ -228,7 +235,7 @@ class QueueViewModel( start = to + 1 } if (collected.isEmpty()) return@withLock - rows.addAll(collected.map(::newRow)) + rows.addAll(collected.map { newRow(it, snapshot.resetVersion) }) while (rows.size > QUEUE_WINDOW_ROWS) rows.removeAt(0) } } @@ -241,23 +248,34 @@ class QueueViewModel( if (fromIndex == toIndex) return if (fromIndex !in rows.indices || toIndex !in rows.indices) return if (rows[fromIndex].upNext != rows[toIndex].upNext) return + if (rows[fromIndex].resetVersion != rows[toIndex].resetVersion) return rows.add(toIndex, rows.removeAt(fromIndex)) } fun play(position: Long) { - viewModelScope.launch { playbackController.playAt(position) } + val resetVersion = rows.firstOrNull { it.position == position }?.resetVersion ?: return + viewModelScope.launch { playbackController.playAt(position, resetVersion) } + } + + private fun rejectMove(): Boolean { + viewModelScope.launch { mutex.withLock { reconcileLocked() } } + return false } fun move( from: Long, to: Long, + targetResetVersion: Long? = null, ): Boolean { if (from == to) return false if (reorderPending) return false + val resetVersion = rows.firstOrNull { it.position == from }?.resetVersion ?: return rejectMove() + val targetResetVersion = targetResetVersion ?: rows.firstOrNull { it.position == to }?.resetVersion ?: resetVersion + if (resetVersion != targetResetVersion) return rejectMove() reorderPending = true viewModelScope.launch { try { - playbackController.move(from, to) + playbackController.move(from, to, resetVersion) mutex.withLock { reconcileLocked() } } finally { reorderPending = false @@ -270,8 +288,9 @@ class QueueViewModel( viewModelScope.launch { mutex.withLock { val index = rows.indexOfFirst { it.position == position } - if (index >= 0) rows.removeAt(index) - playbackController.removeAt(position) + if (index < 0 || (playbackController.state.value.shuffle && !rows[index].upNext)) return@withLock + val row = rows.removeAt(index) + playbackController.removeAt(position, row.resetVersion) reconcileLocked() } } @@ -320,7 +339,7 @@ fun QueueRoute( onPlay = viewModel::play, onRemove = viewModel::remove, onReorder = viewModel::reorder, - onMove = viewModel::move, + onMove = { move -> viewModel.move(move.from, move.to, move.resetVersion) }, onLoadOlder = viewModel::loadOlder, onLoadNewer = viewModel::loadNewer, onUndo = viewModel::undo, @@ -340,7 +359,7 @@ fun QueueScreen( onPlay: (Long) -> Unit, onRemove: (Long) -> Unit, onReorder: (Int, Int) -> Unit, - onMove: (Long, Long) -> Boolean, + onMove: (QueueMove) -> Boolean, onLoadOlder: () -> Unit, onLoadNewer: () -> Unit, onUndo: () -> Unit, @@ -482,7 +501,7 @@ fun QueueScreen( onDragStopped = { dragId?.let { id -> rows.dropMoveFor(id)?.let { move -> - if (onMove(move.from, move.to)) showUndo(queueReordered) + if (onMove(move)) showUndo(queueReordered) } } dragId = null @@ -562,9 +581,10 @@ internal fun dropTarget( return (before - if (before > from) 1 else 0) + 1 } -internal data class QueueMove( +data class QueueMove( val from: Long, val to: Long, + val resetVersion: Long? = null, ) internal fun List.dropMoveFor(draggedId: Long): QueueMove? { @@ -572,7 +592,8 @@ internal fun List.dropMoveFor(draggedId: Long): QueueMove? { if (index < 0) return null val from = this[index].position val to = dropTarget(this, index, from) - return if (to == from) null else QueueMove(from, to) + val resetVersion = this.getOrNull(index + 1)?.resetVersion ?: this.getOrNull(index - 1)?.resetVersion ?: this[index].resetVersion + return if (to == from) null else QueueMove(from, to, resetVersion) } @Suppress("ModifierParameter") diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index 1d501cb2..2389e8a6 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -573,7 +573,7 @@ class AppScreenshotTest { onPlay = {}, onRemove = {}, onReorder = { _, _ -> }, - onMove = { _, _ -> true }, + onMove = { true }, onLoadOlder = {}, onLoadNewer = {}, onUndo = {}, diff --git a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt index 9600974c..37ead9ac 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt @@ -14,6 +14,7 @@ import com.subtracks.data.download.FakeDownloadEngine import com.subtracks.data.model.AlbumSongItem import com.subtracks.data.model.DownloadStatus import com.subtracks.data.model.QueueKind +import com.subtracks.data.model.QueueOrigin import com.subtracks.data.model.Song import com.subtracks.data.model.SongDownload import com.subtracks.data.model.Source @@ -164,7 +165,7 @@ class QueueViewModelTest { } @Test - fun removingWhileShuffledReconcilesTheRows() { + fun removingWhileShuffledIsRejectedWithoutChangingTheRows() { runBlocking { seedSongs(20) } runBlocking { queues.replace(listOf(queues.albumEntry(1, "al1"))) @@ -179,15 +180,13 @@ class QueueViewModelTest { val before = runBlocking { queues.snapshot() } val currentPosition = controller.state.value.position!! val removePosition = (0 until before.size).first { it != currentPosition } + val beforeIds = rowsSnapshot().map { it.song.song.id } viewModel.remove(removePosition) + Thread.sleep(200) - await { - val snapshot = runBlocking { queues.snapshot() } - val rows = rowsSnapshot() - rows.isNotEmpty() && - runBlocking { rows.all { queues.itemAt(snapshot, it.position)?.song?.id == it.song.song.id } } - } + assertEquals(before.contextSize, runBlocking { queues.snapshot() }.contextSize) + assertEquals(beforeIds, rowsSnapshot().map { it.song.song.id }) } @Test @@ -437,6 +436,124 @@ class QueueViewModelTest { private fun assertContiguous() = assertTrue(isContiguous()) + @Test + fun staleRowRemovalIsRejectedAfterControllerReconciliation() { + runBlocking { seedSongs(3) } + controller.playAlbum(1, "al1", 0) + await { controller.state.value.position == 0L } + viewModel.open() + await { viewModel.ready } + resetAlbumWith("s0", 0) + assertEquals("s2", rowsSnapshot()[1].song.song.id) + + viewModel.remove(1) + Thread.sleep(200) + + assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds()) + assertEquals(listOf("s0", "s1", "s2"), rowsSnapshot().map { it.song.song.id }) + } + + @Test + fun staleRowMoveIsRejectedAfterControllerReconciliation() { + runBlocking { seedSongs(3) } + controller.playAlbum(1, "al1", 0) + await { controller.state.value.position == 0L } + viewModel.open() + await { viewModel.ready } + resetAlbumWith("s0", 0) + assertEquals("s2", rowsSnapshot()[1].song.song.id) + + viewModel.move(1, 2) + Thread.sleep(200) + + assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds()) + } + + @Test + fun moveRejectsRowsFromDifferentResetGenerations() { + runOnMain { + viewModel.rows += row(0).copy(resetVersion = 0) + viewModel.rows += row(1).copy(resetVersion = 1) + } + + viewModel.reorder(0, 1) + assertFalse(viewModel.move(0, 1)) + assertEquals(listOf(0L, 1L), rowsSnapshot().map { it.position }) + } + + @Test + fun sparseOfflineDropUsesTheAdjacentRowsGeneration() { + runOnMain { + viewModel.rows += row(1) + viewModel.rows += row(3) + viewModel.rows += row(5) + } + val draggedId = viewModel.rows[0].id + viewModel.reorder(0, 1) + val move = viewModel.rows.dropMoveFor(draggedId)!! + + assertEquals(4L, move.to) + assertTrue(viewModel.move(move.from, move.to, move.resetVersion)) + } + + @Test + fun staleRowPlayIsRejectedAfterControllerReconciliation() { + runBlocking { seedSongs(3) } + controller.playAlbum(1, "al1", 2) + await { controller.state.value.position == 2L } + viewModel.open() + await { viewModel.ready } + resetAlbumWith("s0", 0) + assertEquals("s2", rowsSnapshot()[1].song.song.id) + assertEquals(3L, controller.state.value.position) + + viewModel.play(1) + Thread.sleep(200) + + assertEquals( + "s3", + controller.state.value.item + ?.id, + ) + assertEquals(3L, controller.state.value.position) + } + + private fun resetAlbumWith( + songId: String, + track: Long, + ) { + val layout = controller.state.value.layout + runBlocking { + val point = queues.captureResetPoint() + db.libraryDao().upsertSongs( + listOf( + Song( + sourceId = 1, + id = songId, + albumId = "al1", + artistId = "ar1", + title = "Song $songId", + album = "Album", + artist = "Artist", + duration = 100, + track = track, + disc = 1, + starred = null, + genre = null, + ), + ), + ) + queues.resetChangedOrigins(setOf(QueueOrigin(QueueKind.Album, 1, "al1")), point) + } + await { controller.state.value.layout > layout } + } + + private fun resolvedQueueIds(): List = + runBlocking { + val snapshot = queues.snapshot() + queues.range(snapshot, 0, snapshot.size - 1).map { it.item.song.id } + } + private fun isContiguous(): Boolean { val positions = rowsSnapshot().map { it.position } return positions.isNotEmpty() && positions == (positions.first()..positions.last()).toList()