From 5fadc21455a7a2f6f0f30f19141a9025d89e2962 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 12 Nov 2026 10:15:30 +0000 Subject: [PATCH] ui: roll back rejected queue edits --- .../com/subtracks/ui/playback/QueueScreen.kt | 98 ++++++++++--- .../com/subtracks/ui/AppScreenshotTest.kt | 4 +- .../ui/playback/QueueViewModelTest.kt | 129 ++++++++++++++++-- 3 files changed, 193 insertions(+), 38 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 0c766863..da5272e9 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/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, + val after: List, +) + 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 { - viewModelScope.launch { mutex.withLock { reconcileLocked() } } + private fun rollbackReorder(edit: ReorderRollback?) { + 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 - if (reorderPending) return false - val resetVersion = rows.firstOrNull { it.position == from }?.resetVersion ?: return rejectMove() + val edit = lastReorder + lastReorder = null + 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 index = rows.indexOfFirst { it.position == position } - if (index < 0 || (playbackController.state.value.shuffle && !rows[index].upNext)) return@withLock - val row = rows.removeAt(index) - playbackController.removeAt(position, row.resetVersion) - reconcileLocked() - } + val applied = + mutex.withLock { + if (expected == null) { + reconcileLocked() + return@withLock false + } + 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) - showUndo(queueRemoved) + onRemove(row.position) { applied -> if (applied) showUndo(queueRemoved) } } } else { null diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index 2389e8a6..f559947d 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/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 = {}, 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 37ead9ac..c92d4230 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/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) - Thread.sleep(200) + val completion = CompletableDeferred() + 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() + 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() + 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() + 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) - Thread.sleep(200) + val completion = CompletableDeferred() + 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() + 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() + 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() + 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) - Thread.sleep(200) + val completion = CompletableDeferred() + 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() + 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 - viewModel.reorder(0, 1) - val move = viewModel.rows.dropMoveFor(draggedId)!! + val draggedId = rowsSnapshot()[0].id + runOnMain { + 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() + 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 = + runBlocking { withTimeout(TEST_TIMEOUT_MS) { completion.await() } } + private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected state", predicate) }