mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-11 15:29:15 +01:00
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.
This commit is contained in:
@@ -113,6 +113,8 @@ class QueueViewModel(
|
|||||||
var generation by mutableIntStateOf(0)
|
var generation by mutableIntStateOf(0)
|
||||||
private set
|
private set
|
||||||
|
|
||||||
|
private var reorderPending = false
|
||||||
|
|
||||||
private var nextId = 0L
|
private var nextId = 0L
|
||||||
private val mutex = Mutex()
|
private val mutex = Mutex()
|
||||||
|
|
||||||
@@ -249,12 +251,19 @@ class QueueViewModel(
|
|||||||
fun move(
|
fun move(
|
||||||
from: Long,
|
from: Long,
|
||||||
to: Long,
|
to: Long,
|
||||||
) {
|
): Boolean {
|
||||||
if (from == to) return
|
if (from == to) return false
|
||||||
|
if (reorderPending) return false
|
||||||
|
reorderPending = true
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
playbackController.move(from, to)
|
try {
|
||||||
mutex.withLock { reconcileLocked() }
|
playbackController.move(from, to)
|
||||||
|
mutex.withLock { reconcileLocked() }
|
||||||
|
} finally {
|
||||||
|
reorderPending = false
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
fun remove(position: Long) {
|
fun remove(position: Long) {
|
||||||
@@ -331,7 +340,7 @@ fun QueueScreen(
|
|||||||
onPlay: (Long) -> Unit,
|
onPlay: (Long) -> Unit,
|
||||||
onRemove: (Long) -> Unit,
|
onRemove: (Long) -> Unit,
|
||||||
onReorder: (Int, Int) -> Unit,
|
onReorder: (Int, Int) -> Unit,
|
||||||
onMove: (Long, Long) -> Unit,
|
onMove: (Long, Long) -> Boolean,
|
||||||
onLoadOlder: () -> Unit,
|
onLoadOlder: () -> Unit,
|
||||||
onLoadNewer: () -> Unit,
|
onLoadNewer: () -> Unit,
|
||||||
onUndo: () -> Unit,
|
onUndo: () -> Unit,
|
||||||
@@ -347,7 +356,6 @@ fun QueueScreen(
|
|||||||
val snackbarHostState = remember { SnackbarHostState() }
|
val snackbarHostState = remember { SnackbarHostState() }
|
||||||
val scope = rememberCoroutineScope()
|
val scope = rememberCoroutineScope()
|
||||||
var dragId by remember { mutableStateOf<Long?>(null) }
|
var dragId by remember { mutableStateOf<Long?>(null) }
|
||||||
var dragFrom by remember { mutableStateOf<Long?>(null) }
|
|
||||||
val undoLabel = stringResource(R.string.undo)
|
val undoLabel = stringResource(R.string.undo)
|
||||||
val queueReordered = stringResource(R.string.queue_reordered)
|
val queueReordered = stringResource(R.string.queue_reordered)
|
||||||
val queueRemoved = stringResource(R.string.queue_removed)
|
val queueRemoved = stringResource(R.string.queue_removed)
|
||||||
@@ -470,28 +478,14 @@ fun QueueScreen(
|
|||||||
null
|
null
|
||||||
} else {
|
} else {
|
||||||
Modifier.draggableHandle(
|
Modifier.draggableHandle(
|
||||||
onDragStarted = {
|
onDragStarted = { dragId = row.id },
|
||||||
dragId = row.id
|
|
||||||
dragFrom = row.position
|
|
||||||
},
|
|
||||||
onDragStopped = {
|
onDragStopped = {
|
||||||
val id = dragId
|
dragId?.let { id ->
|
||||||
val from = dragFrom
|
rows.dropMoveFor(id)?.let { move ->
|
||||||
if (id != null && from != null) {
|
if (onMove(move.from, move.to)) showUndo(queueReordered)
|
||||||
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 = null
|
dragId = null
|
||||||
dragFrom = null
|
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
},
|
},
|
||||||
@@ -563,6 +557,19 @@ internal fun dropTarget(
|
|||||||
return (before - if (before > from) 1 else 0) + 1
|
return (before - if (before > from) 1 else 0) + 1
|
||||||
}
|
}
|
||||||
|
|
||||||
|
internal data class QueueMove(
|
||||||
|
val from: Long,
|
||||||
|
val to: Long,
|
||||||
|
)
|
||||||
|
|
||||||
|
internal fun List<QueueRow>.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")
|
@Suppress("ModifierParameter")
|
||||||
@Composable
|
@Composable
|
||||||
private fun QueueRowItem(
|
private fun QueueRowItem(
|
||||||
|
|||||||
@@ -506,7 +506,7 @@ class AppScreenshotTest {
|
|||||||
onPlay = {},
|
onPlay = {},
|
||||||
onRemove = {},
|
onRemove = {},
|
||||||
onReorder = { _, _ -> },
|
onReorder = { _, _ -> },
|
||||||
onMove = { _, _ -> },
|
onMove = { _, _ -> true },
|
||||||
onLoadOlder = {},
|
onLoadOlder = {},
|
||||||
onLoadNewer = {},
|
onLoadNewer = {},
|
||||||
onUndo = {},
|
onUndo = {},
|
||||||
|
|||||||
@@ -325,6 +325,42 @@ class QueueViewModelTest {
|
|||||||
assertEquals((220L..399L).toList(), rowsSnapshot().map { it.position })
|
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
|
@Test
|
||||||
fun removingATrackUnderTheBlockReloadsRows() {
|
fun removingATrackUnderTheBlockReloadsRows() {
|
||||||
runBlocking { seedSongs(10) }
|
runBlocking { seedSongs(10) }
|
||||||
|
|||||||
Reference in New Issue
Block a user