ui: roll back rejected queue edits

app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt

@@ -97,6 +97,11 @@ internal fun QueueRow.isPlaying(
     currentSongId: String?,
 ): Boolean = position == currentPosition && song.song.id == currentSongId
 
+private data class ReorderRollback(
+    val before: List<Long>,
+    val after: List<Long>,
+)
+
 class QueueViewModel(
     private val queueRepository: QueueRepository,
     private val playbackController: PlaybackController,
@@ -115,6 +120,7 @@ class QueueViewModel(
         private set
 
     private var reorderPending = false
+    private var lastReorder: ReorderRollback? = null
 
     private var nextId = 0L
     private val mutex = Mutex()
@@ -245,11 +251,14 @@ class QueueViewModel(
         fromIndex: Int,
         toIndex: Int,
     ) {
+        lastReorder = null
         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
+        val before = rows.map { it.id }
         rows.add(toIndex, rows.removeAt(fromIndex))
+        lastReorder = ReorderRollback(before, rows.map { it.id })
     }
 
     fun play(position: Long) {
@@ -257,8 +266,24 @@ class QueueViewModel(
         viewModelScope.launch { playbackController.playAt(position, resetVersion) }
     }
 
-    private fun rejectMove(): Boolean {
+    private fun rollbackReorder(edit: ReorderRollback?) {
-        viewModelScope.launch { mutex.withLock { reconcileLocked() } }
+        if (edit == null || rows.map { it.id } != edit.after) return
+        val current = rows.associateBy { it.id }
+        rows.clear()
+        rows.addAll(edit.before.map { current.getValue(it) })
+    }
+
+    private fun rejectMove(
+        edit: ReorderRollback?,
+        onComplete: (Boolean) -> Unit,
+    ): Boolean {
+        viewModelScope.launch {
+            mutex.withLock {
+                rollbackReorder(edit)
+                reconcileLocked()
+            }
+            onComplete(false)
+        }
         return false
     }
 
@@ -266,17 +291,28 @@ class QueueViewModel(
         from: Long,
         to: Long,
         targetResetVersion: Long? = null,
+        onComplete: (Boolean) -> Unit = {},
     ): Boolean {
-        if (from == to) return false
+        val edit = lastReorder
-        if (reorderPending) return false
+        lastReorder = null
-        val resetVersion = rows.firstOrNull { it.position == from }?.resetVersion ?: return rejectMove()
+        if (from == to || reorderPending) {
+            onComplete(false)
+            return false
+        }
+        val resetVersion = rows.firstOrNull { it.position == from }?.resetVersion ?: return rejectMove(edit, onComplete)
         val targetResetVersion = targetResetVersion ?: rows.firstOrNull { it.position == to }?.resetVersion ?: resetVersion
-        if (resetVersion != targetResetVersion) return rejectMove()
+        if (resetVersion != targetResetVersion) return rejectMove(edit, onComplete)
         reorderPending = true
         viewModelScope.launch {
             try {
+                val contentVersion = queueRepository.contentVersion()
                 playbackController.move(from, to, resetVersion)
-                mutex.withLock { reconcileLocked() }
+                val applied = queueRepository.contentVersion() != contentVersion
+                mutex.withLock {
+                    if (!applied) rollbackReorder(edit)
+                    reconcileLocked()
+                }
+                onComplete(applied)
             } finally {
                 reorderPending = false
             }
@@ -284,15 +320,34 @@ class QueueViewModel(
         return true
     }
 
-    fun remove(position: Long) {
+    fun remove(
+        position: Long,
+        onComplete: (Boolean) -> Unit = {},
+    ) {
+        val expected = rows.firstOrNull { it.position == position }
         viewModelScope.launch {
-            mutex.withLock {
+            val applied =
-                val index = rows.indexOfFirst { it.position == position }
+                mutex.withLock {
-                if (index < 0 || (playbackController.state.value.shuffle && !rows[index].upNext)) return@withLock
+                    if (expected == null) {
-                val row = rows.removeAt(index)
+                        reconcileLocked()
-                playbackController.removeAt(position, row.resetVersion)
+                        return@withLock false
-                reconcileLocked()
+                    }
-            }
+                    val index =
+                        rows.indexOfFirst {
+                            it.id == expected.id && it.position == expected.position && it.resetVersion == expected.resetVersion
+                        }
+                    if (index < 0 || (playbackController.state.value.shuffle && !rows[index].upNext)) {
+                        reconcileLocked()
+                        return@withLock false
+                    }
+                    val contentVersion = queueRepository.contentVersion()
+                    playbackController.removeAt(expected.position, expected.resetVersion)
+                    val applied = queueRepository.contentVersion() != contentVersion
+                    if (applied) rows.removeAt(index)
+                    reconcileLocked()
+                    applied
+                }
+            onComplete(applied)
         }
     }
 
@@ -337,9 +392,9 @@ fun QueueRoute(
         coverArt = sourceRepository::coverArt,
         onBack = onBack,
         onPlay = viewModel::play,
-        onRemove = viewModel::remove,
+        onRemove = { position, onComplete -> viewModel.remove(position, onComplete) },
         onReorder = viewModel::reorder,
-        onMove = { move -> viewModel.move(move.from, move.to, move.resetVersion) },
+        onMove = { move, onComplete -> viewModel.move(move.from, move.to, move.resetVersion, onComplete) },
         onLoadOlder = viewModel::loadOlder,
         onLoadNewer = viewModel::loadNewer,
         onUndo = viewModel::undo,
@@ -357,9 +412,9 @@ fun QueueScreen(
     coverArt: (String?, Boolean) -> CoverArtRef?,
     onBack: () -> Unit,
     onPlay: (Long) -> Unit,
-    onRemove: (Long) -> Unit,
+    onRemove: (Long, (Boolean) -> Unit) -> Unit,
     onReorder: (Int, Int) -> Unit,
-    onMove: (QueueMove) -> Boolean,
+    onMove: (QueueMove, (Boolean) -> Unit) -> Boolean,
     onLoadOlder: () -> Unit,
     onLoadNewer: () -> Unit,
     onUndo: () -> Unit,
@@ -501,7 +556,7 @@ fun QueueScreen(
                                                 onDragStopped = {
                                                     dragId?.let { id ->
                                                         rows.dropMoveFor(id)?.let { move ->
-                                                            if (onMove(move)) showUndo(queueReordered)
+                                                            onMove(move) { applied -> if (applied) showUndo(queueReordered) }
                                                         }
                                                     }
                                                     dragId = null
@@ -512,8 +567,7 @@ fun QueueScreen(
                                     onRemove =
                                         if (enabled) {
                                             {
-                                                onRemove(row.position)
+                                                onRemove(row.position) { applied -> if (applied) showUndo(queueRemoved) }
-                                                showUndo(queueRemoved)
                                             }
                                         } else {
                                             null

app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt

@@ -571,9 +571,9 @@ class AppScreenshotTest {
                     coverArt = { id, _ -> id?.let { CoverArtRef(it, "test:$it") } },
                     onBack = {},
                     onPlay = {},
-                    onRemove = {},
+                    onRemove = { _, _ -> },
                     onReorder = { _, _ -> },
-                    onMove = { true },
+                    onMove = { _, _ -> true },
                     onLoadOlder = {},
                     onLoadNewer = {},
                     onUndo = {},

app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt

@@ -27,6 +27,7 @@ import com.subtracks.data.repo.setShuffle
 import com.subtracks.playback.FakePlayerConnection
 import com.subtracks.playback.FakePlayerHandle
 import com.subtracks.playback.PlaybackController
+import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -182,8 +183,9 @@ class QueueViewModelTest {
         val removePosition = (0 until before.size).first { it != currentPosition }
         val beforeIds = rowsSnapshot().map { it.song.song.id }
 
-        viewModel.remove(removePosition)
+        val completion = CompletableDeferred<Boolean>()
-        Thread.sleep(200)
+        viewModel.remove(removePosition, completion::complete)
+        assertFalse(awaitCompletion(completion))
 
         assertEquals(before.contextSize, runBlocking { queues.snapshot() }.contextSize)
         assertEquals(beforeIds, rowsSnapshot().map { it.song.song.id })
@@ -293,7 +295,9 @@ class QueueViewModelTest {
         val from = rowsSnapshot()[10].position
         val to = rowsSnapshot()[13].position
         runOnMain { viewModel.reorder(10, 13) }
-        viewModel.move(from, to)
+        val completion = CompletableDeferred<Boolean>()
+        viewModel.move(from, to, onComplete = completion::complete)
+        assertTrue(awaitCompletion(completion))
         await {
             rowsSnapshot().size == QUEUE_WINDOW_ROWS &&
                 rowsSnapshot().getOrNull(13)?.position == 233L
@@ -382,7 +386,9 @@ class QueueViewModelTest {
         viewModel.open()
         await { viewModel.ready }
 
-        viewModel.remove(2)
+        val completion = CompletableDeferred<Boolean>()
+        viewModel.remove(2, completion::complete)
+        assertTrue(awaitCompletion(completion))
         await {
             val positions = rowsSnapshot().map { it.position }
             positions.isNotEmpty() && positions == (positions.first()..positions.last()).toList()
@@ -409,7 +415,9 @@ class QueueViewModelTest {
             rowsSnapshot()[30]
                 .song.song.id
 
-        viewModel.remove(rowsSnapshot()[30].position)
+        val completion = CompletableDeferred<Boolean>()
+        viewModel.remove(rowsSnapshot()[30].position, completion::complete)
+        assertTrue(awaitCompletion(completion))
         await {
             rowsSnapshot().size == 60 &&
                 rowsSnapshot().firstOrNull()?.position == 70L &&
@@ -446,13 +454,71 @@ class QueueViewModelTest {
         resetAlbumWith("s0", 0)
         assertEquals("s2", rowsSnapshot()[1].song.song.id)
 
-        viewModel.remove(1)
+        val completion = CompletableDeferred<Boolean>()
-        Thread.sleep(200)
+        viewModel.remove(1, completion::complete)
+        assertFalse(awaitCompletion(completion))
 
         assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds())
         assertEquals(listOf("s0", "s1", "s2"), rowsSnapshot().map { it.song.song.id })
     }
 
+    @Test
+    fun removalWaitingForReconciliationKeepsTheCapturedRow() {
+        runBlocking { seedSongs(3) }
+        controller.playAlbum(1, "al1", 0)
+        await { controller.state.value.position == 0L }
+        viewModel.open()
+        await { viewModel.ready }
+        resetAlbumWith("s0", 0)
+        val selected = rowsSnapshot()[1]
+        assertEquals("s2", selected.song.song.id)
+
+        val completion = CompletableDeferred<Boolean>()
+        runOnMain {
+            viewModel.reconcile()
+            viewModel.remove(selected.position, completion::complete)
+        }
+        assertFalse(awaitCompletion(completion))
+
+        assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds())
+        assertEquals(listOf("s0", "s1", "s2"), rowsSnapshot().map { it.song.song.id })
+    }
+
+    @Test
+    fun rejectedSparseTailMoveRestoresTheVisibleOrder() {
+        openReconciledSparseOfflineQueue()
+        val canonical = resolvedQueueIds()
+        val before = rowsSnapshot()
+        val dragged = before[1]
+
+        val completion = CompletableDeferred<Boolean>()
+        runOnMain {
+            viewModel.reorder(1, 2)
+            val move = viewModel.rows.dropMoveFor(dragged.id)!!
+            assertTrue(viewModel.move(move.from, move.to, move.resetVersion, completion::complete))
+        }
+        assertFalse(awaitCompletion(completion))
+
+        assertEquals(canonical, resolvedQueueIds())
+        assertEquals(before.map { it.id }, rowsSnapshot().map { it.id })
+        assertEquals(before.map { it.position }, rowsSnapshot().map { it.position })
+    }
+
+    @Test
+    fun rejectedSparseTailRemovalRestoresTheVisibleOrder() {
+        openReconciledSparseOfflineQueue()
+        val canonical = resolvedQueueIds()
+        val before = rowsSnapshot()
+
+        val completion = CompletableDeferred<Boolean>()
+        viewModel.remove(before[1].position, completion::complete)
+        assertFalse(awaitCompletion(completion))
+
+        assertEquals(canonical, resolvedQueueIds())
+        assertEquals(before.map { it.id }, rowsSnapshot().map { it.id })
+        assertEquals(before.map { it.position }, rowsSnapshot().map { it.position })
+    }
+
     @Test
     fun staleRowMoveIsRejectedAfterControllerReconciliation() {
         runBlocking { seedSongs(3) }
@@ -463,8 +529,9 @@ class QueueViewModelTest {
         resetAlbumWith("s0", 0)
         assertEquals("s2", rowsSnapshot()[1].song.song.id)
 
-        viewModel.move(1, 2)
+        val completion = CompletableDeferred<Boolean>()
-        Thread.sleep(200)
+        assertTrue(viewModel.move(1, 2, onComplete = completion::complete))
+        assertFalse(awaitCompletion(completion))
 
         assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds())
     }
@@ -477,7 +544,9 @@ class QueueViewModelTest {
         }
 
         viewModel.reorder(0, 1)
-        assertFalse(viewModel.move(0, 1))
+        val completion = CompletableDeferred<Boolean>()
+        assertFalse(viewModel.move(0, 1, onComplete = completion::complete))
+        assertFalse(awaitCompletion(completion))
         assertEquals(listOf(0L, 1L), rowsSnapshot().map { it.position })
     }
 
@@ -488,12 +557,19 @@ class QueueViewModelTest {
             viewModel.rows += row(3)
             viewModel.rows += row(5)
         }
-        val draggedId = viewModel.rows[0].id
+        val draggedId = rowsSnapshot()[0].id
-        viewModel.reorder(0, 1)
+        runOnMain {
-        val move = viewModel.rows.dropMoveFor(draggedId)!!
+            viewModel.reorder(0, 1)
+            viewModel.rows[2] = viewModel.rows[2].copy(resetVersion = 1)
+        }
+        val move = rowsSnapshot().dropMoveFor(draggedId)!!
 
         assertEquals(4L, move.to)
-        assertTrue(viewModel.move(move.from, move.to, move.resetVersion))
+        assertEquals(1L, move.resetVersion)
+        val completion = CompletableDeferred<Boolean>()
+        assertFalse(viewModel.move(move.from, move.to, move.resetVersion, completion::complete))
+        assertFalse(awaitCompletion(completion))
+        assertEquals(listOf(1L, 3L, 5L), rowsSnapshot().map { it.position })
     }
 
     @Test
@@ -518,6 +594,28 @@ class QueueViewModelTest {
         assertEquals(3L, controller.state.value.position)
     }
 
+    private fun openReconciledSparseOfflineQueue() {
+        runBlocking { seedSongs(901) }
+        markDownloaded(1, "s1")
+        markDownloaded(1, "s801")
+        markDownloaded(1, "s901")
+        setOffline()
+        controller.playAlbum(1, "al1", 0)
+        await {
+            controller.state.value.item
+                ?.id == "s1"
+        }
+        viewModel.open()
+        await { viewModel.ready }
+        viewModel.loadNewer()
+        await { rowsSnapshot().map { it.position } == listOf(0L, 800L, 900L) }
+
+        resetAlbumWith("s0", 0)
+        viewModel.reconcile()
+        await { rowsSnapshot().firstOrNull()?.resetVersion == 1L }
+        assertEquals(listOf(1L, 800L, 900L), rowsSnapshot().map { it.position })
+    }
+
     private fun resetAlbumWith(
         songId: String,
         track: Long,
@@ -678,5 +776,8 @@ class QueueViewModelTest {
 
     private fun runOnMain(block: () -> Unit) = runBlocking { withContext(dispatcher) { block() } }
 
+    private fun awaitCompletion(completion: CompletableDeferred<Boolean>): Boolean =
+        runBlocking { withTimeout(TEST_TIMEOUT_MS) { completion.await() } }
+
     private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected state", predicate)
 }