From 8e7431cc2eb5d24dd7f29d0647e431230225836c Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Fri, 9 Oct 2026 18:48:38 +0000 Subject: [PATCH] fix: stop a queue drag from moving the wrong rows A drop captured the dragged row's queue position once, at drag start, and committed on a fire-and-forget coroutine. If the queue renumbered while the drag was in flight (up-next consumed, a reconcile) or before a prior move had reconciled, the drop resolved its target against stale coordinates and moved the wrong entry - the "two middle rows swap on release" symptom. Resolve the move from the current rows at drop time, and have move() report whether it was accepted so a refused reorder is not announced as done; a further move is refused while one is still committing. --- .../com/subtracks/ui/playback/QueueScreen.kt | 55 +++++++++++-------- .../com/subtracks/ui/AppScreenshotTest.kt | 2 +- .../ui/playback/QueueViewModelTest.kt | 36 ++++++++++++ 3 files changed, 68 insertions(+), 25 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 2e1e91fd..9c3ed189 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -113,6 +113,8 @@ class QueueViewModel( var generation by mutableIntStateOf(0) private set + private var reorderPending = false + private var nextId = 0L private val mutex = Mutex() @@ -249,12 +251,19 @@ class QueueViewModel( fun move( from: Long, to: Long, - ) { - if (from == to) return + ): Boolean { + if (from == to) return false + if (reorderPending) return false + reorderPending = true viewModelScope.launch { - playbackController.move(from, to) - mutex.withLock { reconcileLocked() } + try { + playbackController.move(from, to) + mutex.withLock { reconcileLocked() } + } finally { + reorderPending = false + } } + return true } fun remove(position: Long) { @@ -331,7 +340,7 @@ fun QueueScreen( onPlay: (Long) -> Unit, onRemove: (Long) -> Unit, onReorder: (Int, Int) -> Unit, - onMove: (Long, Long) -> Unit, + onMove: (Long, Long) -> Boolean, onLoadOlder: () -> Unit, onLoadNewer: () -> Unit, onUndo: () -> Unit, @@ -347,7 +356,6 @@ fun QueueScreen( val snackbarHostState = remember { SnackbarHostState() } val scope = rememberCoroutineScope() var dragId by remember { mutableStateOf(null) } - var dragFrom by remember { mutableStateOf(null) } val undoLabel = stringResource(R.string.undo) val queueReordered = stringResource(R.string.queue_reordered) val queueRemoved = stringResource(R.string.queue_removed) @@ -470,28 +478,14 @@ fun QueueScreen( null } else { Modifier.draggableHandle( - onDragStarted = { - dragId = row.id - dragFrom = row.position - }, + onDragStarted = { dragId = row.id }, onDragStopped = { - val id = dragId - val from = dragFrom - if (id != null && from != null) { - val draggedIndex = rows.indexOfFirst { it.id == id } - val to = - if (draggedIndex < 0) { - from - } else { - dropTarget(rows, draggedIndex, from) - } - if (to != from) { - onMove(from, to) - showUndo(queueReordered) + dragId?.let { id -> + rows.dropMoveFor(id)?.let { move -> + if (onMove(move.from, move.to)) showUndo(queueReordered) } } dragId = null - dragFrom = null }, ) }, @@ -563,6 +557,19 @@ internal fun dropTarget( return (before - if (before > from) 1 else 0) + 1 } +internal data class QueueMove( + val from: Long, + val to: Long, +) + +internal fun List.dropMoveFor(draggedId: Long): QueueMove? { + val index = indexOfFirst { it.id == draggedId } + 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) +} + @Suppress("ModifierParameter") @Composable private fun QueueRowItem( diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index b1e972cb..658ed8d9 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -506,7 +506,7 @@ class AppScreenshotTest { onPlay = {}, onRemove = {}, onReorder = { _, _ -> }, - onMove = { _, _ -> }, + 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 8ab83c3f..6b1b6df1 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt @@ -325,6 +325,42 @@ class QueueViewModelTest { assertEquals((220L..399L).toList(), rowsSnapshot().map { it.position }) } + @Test + fun ignoringASecondMoveWhileTheFirstIsStillCommitting() { + runBlocking { seedSongs(10) } + controller.playAlbum(1, "al1", 0) + await { controller.state.value.position == 0L } + + viewModel.open() + await { viewModel.ready } + val before = rowsSnapshot().map { it.song.song.id } + val from = rowsSnapshot().first().position + + runOnMain { + assertTrue(viewModel.move(from, from + 3)) + assertFalse(viewModel.move(from, from + 1)) + } + + val expected = before.toMutableList().also { it.add(3, it.removeAt(0)) } + await { rowsSnapshot().map { it.song.song.id } == expected } + assertEquals(expected, rowsSnapshot().map { it.song.song.id }) + } + + @Test + fun dropMoveTakesTheDraggedRowsCurrentPosition() { + val rows = + listOf( + row(10).copy(id = 2L), + row(11).copy(id = 3L), + row(9).copy(id = 1L), + ) + + val move = rows.dropMoveFor(1L)!! + + assertEquals(9L, move.from) + assertEquals(11L, move.to) + } + @Test fun removingATrackUnderTheBlockReloadsRows() { runBlocking { seedSongs(10) }