From 2766affa659a2d2bb012129e88c78e955d1eb65a Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 12 Nov 2026 13:26:16 +0000 Subject: [PATCH] ui: roll back rejected multi-step queue edits --- .../com/subtracks/ui/playback/QueueScreen.kt | 53 ++-- .../com/subtracks/ui/AppScreenshotTest.kt | 3 +- .../subtracks/ui/playback/QueueScreenTest.kt | 6 +- .../ui/playback/QueueViewModelTest.kt | 248 ++++++++++++++++-- 4 files changed, 256 insertions(+), 54 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 da5272e9..d8c0e5de 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -120,7 +120,7 @@ class QueueViewModel( private set private var reorderPending = false - private var lastReorder: ReorderRollback? = null + private var dragRollback: ReorderRollback? = null private var nextId = 0L private val mutex = Mutex() @@ -251,14 +251,13 @@ 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 } + val before = dragRollback?.before ?: rows.map { it.id } rows.add(toIndex, rows.removeAt(fromIndex)) - lastReorder = ReorderRollback(before, rows.map { it.id }) + dragRollback = ReorderRollback(before, rows.map { it.id }) } fun play(position: Long) { @@ -275,7 +274,7 @@ class QueueViewModel( private fun rejectMove( edit: ReorderRollback?, - onComplete: (Boolean) -> Unit, + onComplete: (Boolean) -> Unit = {}, ): Boolean { viewModelScope.launch { mutex.withLock { @@ -287,27 +286,28 @@ class QueueViewModel( return false } + fun cancelMove() { + val edit = dragRollback ?: return + dragRollback = null + rejectMove(edit) + } + fun move( from: Long, to: Long, targetResetVersion: Long? = null, onComplete: (Boolean) -> Unit = {}, ): Boolean { - val edit = lastReorder - lastReorder = null - if (from == to || reorderPending) { - onComplete(false) - return false - } + val edit = dragRollback + dragRollback = null + if (from == to || reorderPending) return rejectMove(edit, onComplete) 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(edit, onComplete) reorderPending = true viewModelScope.launch { try { - val contentVersion = queueRepository.contentVersion() - playbackController.move(from, to, resetVersion) - val applied = queueRepository.contentVersion() != contentVersion + val applied = playbackController.move(from, to, resetVersion) mutex.withLock { if (!applied) rollbackReorder(edit) reconcileLocked() @@ -340,10 +340,14 @@ class QueueViewModel( reconcileLocked() return@withLock false } - val contentVersion = queueRepository.contentVersion() - playbackController.removeAt(expected.position, expected.resetVersion) - val applied = queueRepository.contentVersion() != contentVersion - if (applied) rows.removeAt(index) + val applied = playbackController.removeAt(expected.position, expected.resetVersion) + if (applied) { + val currentIndex = + rows.indexOfFirst { + it.id == expected.id && it.position == expected.position && it.resetVersion == expected.resetVersion + } + if (currentIndex >= 0) rows.removeAt(currentIndex) + } reconcileLocked() applied } @@ -395,6 +399,7 @@ fun QueueRoute( onRemove = { position, onComplete -> viewModel.remove(position, onComplete) }, onReorder = viewModel::reorder, onMove = { move, onComplete -> viewModel.move(move.from, move.to, move.resetVersion, onComplete) }, + onCancelMove = viewModel::cancelMove, onLoadOlder = viewModel::loadOlder, onLoadNewer = viewModel::loadNewer, onUndo = viewModel::undo, @@ -414,7 +419,8 @@ fun QueueScreen( onPlay: (Long) -> Unit, onRemove: (Long, (Boolean) -> Unit) -> Unit, onReorder: (Int, Int) -> Unit, - onMove: (QueueMove, (Boolean) -> Unit) -> Boolean, + onMove: (QueueMove, (Boolean) -> Unit) -> Unit, + onCancelMove: () -> Unit, onLoadOlder: () -> Unit, onLoadNewer: () -> Unit, onUndo: () -> Unit, @@ -554,10 +560,11 @@ fun QueueScreen( Modifier.draggableHandle( onDragStarted = { dragId = row.id }, onDragStopped = { - dragId?.let { id -> - rows.dropMoveFor(id)?.let { move -> - onMove(move) { applied -> if (applied) showUndo(queueReordered) } - } + val move = dragId?.let { id -> rows.dropMoveFor(id) } + if (move == null) { + onCancelMove() + } else { + onMove(move) { applied -> if (applied) showUndo(queueReordered) } } dragId = null }, diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index f559947d..5167a955 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -573,7 +573,8 @@ class AppScreenshotTest { onPlay = {}, onRemove = { _, _ -> }, onReorder = { _, _ -> }, - onMove = { _, _ -> true }, + onMove = { _, _ -> }, + onCancelMove = {}, onLoadOlder = {}, onLoadNewer = {}, onUndo = {}, diff --git a/app/src/test/kotlin/com/subtracks/ui/playback/QueueScreenTest.kt b/app/src/test/kotlin/com/subtracks/ui/playback/QueueScreenTest.kt index a540dfc8..6af1c1e9 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueScreenTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueScreenTest.kt @@ -78,10 +78,7 @@ class QueueScreenTest { visible: List = rows, shuffle: Boolean = false, onRemove: (Long, (Boolean) -> Unit) -> Unit = { _, onComplete -> onComplete(false) }, - onMove: (QueueMove, (Boolean) -> Unit) -> Boolean = { _, onComplete -> - onComplete(false) - true - }, + onMove: (QueueMove, (Boolean) -> Unit) -> Unit = { _, onComplete -> onComplete(false) }, onUndo: () -> Unit = {}, ) { val visibleRows = mutableStateListOf().also { it.addAll(visible) } @@ -99,6 +96,7 @@ class QueueScreenTest { onRemove = onRemove, onReorder = { from, to -> visibleRows.add(to, visibleRows.removeAt(from)) }, onMove = onMove, + onCancelMove = {}, onLoadOlder = {}, onLoadNewer = {}, onUndo = 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 c92d4230..df4f7ca0 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt @@ -29,6 +29,7 @@ import com.subtracks.playback.FakePlayerHandle import com.subtracks.playback.PlaybackController import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.CoroutineStart import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job @@ -36,11 +37,13 @@ import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeout +import kotlinx.coroutines.yield import okhttp3.OkHttpClient import org.junit.After import org.junit.Assert.assertEquals @@ -50,7 +53,9 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import java.io.File +import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit @OptIn(ExperimentalCoroutinesApi::class) @RunWith(AndroidJUnit4::class) @@ -536,6 +541,164 @@ class QueueViewModelTest { assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds()) } + @Test + fun moveWaitingForQueuedResetDoesNotReportTheResetAsApplied() { + runBlocking { seedSongs(3) } + controller.playAlbum(1, "al1", 0) + await { controller.state.value.position == 0L } + viewModel.open() + await { viewModel.ready } + val point = prepareAlbumReset("s0", 0) + val paused = pauseQueueMutation() + val reset = + repoScope.launch(start = CoroutineStart.UNDISPATCHED) { + queues.resetChangedOrigins(setOf(QueueOrigin(QueueKind.Album, 1, "al1")), point) + } + val completion = CompletableDeferred() + try { + runOnMainSuspending { + viewModel.reorder(1, 2) + assertTrue(viewModel.move(1, 2, onComplete = completion::complete)) + yield() + } + } finally { + resumeQueueMutation(paused) + } + + assertFalse(awaitCompletion(completion)) + runBlocking { withTimeout(TEST_TIMEOUT_MS) { reset.join() } } + assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds()) + } + + @Test + fun removalWaitingForQueuedResetDoesNotReportTheResetAsApplied() { + runBlocking { seedSongs(3) } + controller.playAlbum(1, "al1", 0) + await { controller.state.value.position == 0L } + viewModel.open() + await { viewModel.ready } + val point = prepareAlbumReset("s0", 0) + val paused = pauseQueueMutation() + val reset = + repoScope.launch(start = CoroutineStart.UNDISPATCHED) { + queues.resetChangedOrigins(setOf(QueueOrigin(QueueKind.Album, 1, "al1")), point) + } + val completion = CompletableDeferred() + try { + runOnMainSuspending { + viewModel.remove(2, completion::complete) + yield() + } + } finally { + resumeQueueMutation(paused) + } + + assertFalse(awaitCompletion(completion)) + runBlocking { withTimeout(TEST_TIMEOUT_MS) { reset.join() } } + assertEquals(listOf("s0", "s1", "s2", "s3"), resolvedQueueIds()) + } + + @Test + fun rejectedTwoCallbackSparseTailDragRestoresTheDragBaseline() { + openReconciledSparseOfflineQueue(includeFarTail = true) + val canonical = resolvedQueueIds() + val before = rowsSnapshot() + val dragged = before[1] + val completion = CompletableDeferred() + runOnMain { + viewModel.reorder(1, 2) + viewModel.reorder(2, 3) + 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 rejectedMixedGenerationCrossingRestoresTheWholeDrag() { + runOnMain { + viewModel.rows += row(0) + viewModel.rows += row(1) + viewModel.rows += row(2) + } + val before = rowsSnapshot() + val dragged = before[0] + val completion = CompletableDeferred() + runOnMain { + viewModel.reorder(0, 1) + viewModel.rows[2] = viewModel.rows[2].copy(resetVersion = 1) + viewModel.reorder(1, 2) + val move = viewModel.rows.dropMoveFor(dragged.id)!! + assertFalse(viewModel.move(move.from, move.to, move.resetVersion, completion::complete)) + } + + assertFalse(awaitCompletion(completion)) + assertEquals(before.map { it.id }, rowsSnapshot().map { it.id }) + assertEquals(before.map { it.position }, rowsSnapshot().map { it.position }) + } + + @Test + fun secondOptimisticDragRejectedWhileMovePendingRollsBack() { + openSparseOfflineQueue() + val before = rowsSnapshot() + val visibleIds = before.map { it.song.song.id }.toSet() + val expectedFirst = before.toMutableList().also { it.add(1, it.removeAt(0)) }.map { it.song.song.id } + val expectedSecond = + before + .toMutableList() + .also { it.add(1, it.removeAt(0)) } + .also { it.add(1, it.removeAt(2)) } + .map { it.song.song.id } + val paused = pauseQueueMutation() + val firstCompletion = CompletableDeferred() + val secondCompletion = CompletableDeferred() + try { + runOnMainSuspending { + viewModel.reorder(0, 1) + val firstMove = viewModel.rows.dropMoveFor(before[0].id)!! + assertTrue(viewModel.move(firstMove.from, firstMove.to, firstMove.resetVersion, firstCompletion::complete)) + yield() + viewModel.reorder(2, 1) + assertEquals(expectedSecond, viewModel.rows.map { it.song.song.id }) + val secondMove = viewModel.rows.dropMoveFor(before[2].id)!! + assertFalse(viewModel.move(secondMove.from, secondMove.to, secondMove.resetVersion, secondCompletion::complete)) + } + await { rowsSnapshot().map { it.song.song.id } == expectedFirst } + } finally { + resumeQueueMutation(paused) + } + + assertTrue(awaitCompletion(firstCompletion)) + assertFalse(awaitCompletion(secondCompletion)) + assertEquals(expectedFirst, resolvedQueueIds().filter { it in visibleIds }) + } + + @Test + fun removalAfterInterleavedReorderRemovesTheCapturedRow() { + openSparseOfflineQueue() + val before = rowsSnapshot() + val paused = pauseQueueMutation() + val completion = CompletableDeferred() + try { + runOnMainSuspending { + viewModel.remove(before[2].position, completion::complete) + yield() + viewModel.reorder(0, 2) + } + assertEquals(listOf("s801", "s901", "s1"), rowsSnapshot().map { it.song.song.id }) + } finally { + resumeQueueMutation(paused) + } + + assertTrue(awaitCompletion(completion)) + assertEquals(listOf("s1", "s801"), rowsSnapshot().map { it.song.song.id }) + assertEquals(listOf("s1", "s801"), resolvedQueueIds().filter { it in setOf("s1", "s801", "s901") }) + } + @Test fun moveRejectsRowsFromDifferentResetGenerations() { runOnMain { @@ -594,11 +757,12 @@ class QueueViewModelTest { assertEquals(3L, controller.state.value.position) } - private fun openReconciledSparseOfflineQueue() { - runBlocking { seedSongs(901) } + private fun openSparseOfflineQueue(includeFarTail: Boolean = false) { + runBlocking { seedSongs(if (includeFarTail) 1001 else 901) } markDownloaded(1, "s1") markDownloaded(1, "s801") markDownloaded(1, "s901") + if (includeFarTail) markDownloaded(1, "s1001") setOffline() controller.playAlbum(1, "al1", 0) await { @@ -608,12 +772,17 @@ class QueueViewModelTest { viewModel.open() await { viewModel.ready } viewModel.loadNewer() - await { rowsSnapshot().map { it.position } == listOf(0L, 800L, 900L) } + val expected = if (includeFarTail) listOf(0L, 800L, 900L, 1000L) else listOf(0L, 800L, 900L) + await { rowsSnapshot().map { it.position } == expected } + } + private fun openReconciledSparseOfflineQueue(includeFarTail: Boolean = false) { + openSparseOfflineQueue(includeFarTail) resetAlbumWith("s0", 0) viewModel.reconcile() await { rowsSnapshot().firstOrNull()?.resetVersion == 1L } - assertEquals(listOf(1L, 800L, 900L), rowsSnapshot().map { it.position }) + val expected = if (includeFarTail) listOf(1L, 800L, 900L, 1000L) else listOf(1L, 800L, 900L) + assertEquals(expected, rowsSnapshot().map { it.position }) } private fun resetAlbumWith( @@ -621,31 +790,37 @@ class QueueViewModelTest { 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) - } + val point = prepareAlbumReset(songId, track) + runBlocking { queues.resetChangedOrigins(setOf(QueueOrigin(QueueKind.Album, 1, "al1")), point) } await { controller.state.value.layout > layout } } + private fun prepareAlbumReset( + songId: String, + track: Long, + ) = 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, + ), + ), + ) + point + } + private fun resolvedQueueIds(): List = runBlocking { val snapshot = queues.snapshot() @@ -770,12 +945,33 @@ class QueueViewModelTest { ) } + private fun pauseQueueMutation(): Pair { + val locked = CountDownLatch(1) + val release = CountDownLatch(1) + val locker = + repoScope.launch { + queues.withMutationLock { + locked.countDown() + release.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS) + } + } + assertTrue(locked.await(TEST_TIMEOUT_MS, TimeUnit.MILLISECONDS)) + return release to locker + } + + private fun resumeQueueMutation(paused: Pair) { + paused.first.countDown() + runBlocking { withTimeout(TEST_TIMEOUT_MS) { paused.second.join() } } + } + // `rows` is a snapshot list mutated on the main dispatcher, so read it there rather than from // the test thread (otherwise iterating it races the view model and throws or reads torn state). private fun rowsSnapshot(): List = runBlocking { withContext(dispatcher) { viewModel.rows.toList() } } private fun runOnMain(block: () -> Unit) = runBlocking { withContext(dispatcher) { block() } } + private fun runOnMainSuspending(block: suspend () -> Unit) = runBlocking { withContext(dispatcher) { block() } } + private fun awaitCompletion(completion: CompletableDeferred): Boolean = runBlocking { withTimeout(TEST_TIMEOUT_MS) { completion.await() } }