aclassen / ComposeReorderable

Enables reordering by drag and drop in Jetpack Compose (Desktop) LazyList & LazyGrid.
Apache License 2.0
836 stars 89 forks source link

Isn't stable if the parent component recomposes during a drag #276

Open distinctdan opened 1 year ago

distinctdan commented 1 year ago

I'm trying to keep track of the sort order in a separate list, so that if it gets recomposed with new state during a drag, we won't lose the sort order. My app needs to subscribe to a server that emits new state periodically. However, ComposeReorderable appears to require that the same mutableList be passed to LazyColumn's items as the list you update in onMove.

Steps to reproduce:

I've also tested this by updating both a mutableList directly in addition to my derived list. I've verified that both lists have exactly the same contents at every recomposition, so I'm guessing ComposeReorderable might be relying on some internal functionality in LazyList that runs in a different order if you update the list directly.

// Store sort order separately from rows, because the rows can change if we get new state.
// Reset the sort order if rows are added or removed.
val proteinsList = rows.value.map { it.proteinName }
val proteinsSet = proteinsList.toSet()
val sortOrder = remember(proteinsSet) {
    mutableStateOf(proteinsList)
}

// Sort the rows
val sortedRows = remember(rows.value, sortOrder.value) {
    Timber.i("sortedRows remember. sortOrder: ${sortOrder.value}")
    val sortOrderById = sortOrder.value
        .withIndex()
        .associate { (index, value) ->
            value to index
        }

    rows.value.sortedWith { a, b ->
        val aIndex = sortOrderById[a.proteinName]
        val bIndex = sortOrderById[b.proteinName]

        // If a row got added, it won't have an index, we'll keep it at its original location.
        // This shouldn't ever happen, but we're covering our bases.
        if (aIndex == null || bIndex == null) {
            return@sortedWith 0
        }

        aIndex - bIndex
    }
}

val reorderState = rememberReorderableLazyListState(
    onMove = { from, to ->
        Timber.i("ON MOVE: $from, $to")
        // Update our sort order, which will cause the sorted rows to recalculate.
        sortOrder.value = sortOrder.value.toMutableList().apply {
            if (to.index < size - 1 && from.index < size) {
                add(to.index, removeAt(from.index))
            } else {
                Timber.w("Tried to reorder outside of bounds. size: ${sortOrder.value.size}, from: $from, to: $to")
            }
        }
    },
    onDragEnd = { startIndex, endIndex ->
        Timber.i("DRAG END: sortOrder: ${sortOrder.value}")
    }
)

LazyColumn(
    state = reorderState.listState,
    contentPadding = contentPadding,
    verticalArrangement = Arrangement.spacedBy(ThemeConstants.proteinRowSpacing),
    modifier = modifier.reorderable(reorderState)
) {
    items(sortedRows, { it.proteinName }) { row ->
        ReorderableItem(
            reorderableState = reorderState,
            key = row.proteinName,
        ) { isDragging ->
            // Protein row component
        }
    }
}
YoussefHachicha commented 1 year ago

yap had the same issue

distinctdan commented 11 months ago

I've learned more information about this through testing. It appears that if the caller of rememberReorderableLazyListState recomposes when onMove is called, then the drag becomes unstable, even if nothing changed. I'm guessing that the reorderable state is depending internally on some property of the lazy list state, and is counting on only being recomposed at certain times. I'm currently investigating to see if I can wrap it to prevent recompositions while a drag is in progress.

distinctdan commented 11 months ago

Ok, I've written a cache to work around this issue by caching the items while dragging. Example usage:

val reorderCache = rememberReorderableLazyListCache(items = myItems)
val reorderState = rememberReorderableLazyListState(
    onMove = { from, to ->
        // It's safe to directly modify the items in the cache since they only get replaced when
        // a drag starts.
        reorderCache.items.value = reorderCache.items.value.toMutableList().apply {
            if (to.index < size && from.index < size) {
                add(to.index, removeAt(from.index))
            } else {
                Timber.w("Tried to reorder outside of bounds. size: ${reorderCache.items.value.size}, from: $from, to: $to")
            }
        }
    },
    onDragEnd = { startIndex, endIndex ->
        onReorder(reorderCache.items.value.map { it.id })
    }
)
// Keep items updated.
reorderCache.onCompose(reorderState, proteinColumns.value)

LazyRow(
    state = reorderState.listState,
    modifier = modifier.reorderable(reorderState)
) {
    items(reorderCache.items.value, { it.id }) { item ->
        ReorderableItem(
            reorderableState = reorderState,
            key = item.id,
        ) { isDragging ->

        }
    }
}

And the actual implementation:

/**
 * Caches the list of items while dragging. This is necessary in some cases because ReorderableLazyList
 * isn't stable if the list changes while a drag is in progress. Example usage:
 *
 * ```
 * val reorderCache = rememberReorderableLazyListCache(items = myItems)
 * val reorderState = rememberReorderableLazyListState(
 *     onMove = { from, to ->
 *         // It's safe to directly modify the items in the cache since they only get replaced when
 *         // a drag starts.
 *         reorderCache.items.value = reorderCache.items.value.toMutableList().apply {
 *             if (to.index < size && from.index < size) {
 *                 add(to.index, removeAt(from.index))
 *             } else {
 *                 Timber.w("Tried to reorder outside of bounds. size: ${reorderCache.items.value.size}, from: $from, to: $to")
 *             }
 *         }
 *     },
 *     onDragEnd = { startIndex, endIndex ->
 *         onReorder(reorderCache.items.value.map { it.id })
 *     }
 * )
 * // Keep items updated.
 * reorderCache.onCompose(reorderState, proteinColumns.value)
 *
 * LazyRow(
 *     state = reorderState.listState,
 *     modifier = modifier.reorderable(reorderState)
 * ) {
 *     items(reorderCache.items.value, { it.id }) { item ->
 *         ReorderableItem(
 *             reorderableState = reorderState,
 *             key = item.id,
 *         ) { isDragging ->
 *
 *         }
 *     }
 * }
 * ```
 */
@Composable
fun <T> rememberReorderableLazyListCache(
    items: List<T>,
): ReorderableLazyListCache<T> {
    val scope = rememberCoroutineScope()
    val cache = remember { ReorderableLazyListCache<T>(items, scope) }
    return cache
}

@Stable
class ReorderableLazyListCache <T>(
    private val initialItems: List<T>,
    private val scope: CoroutineScope,
) {
    private var state: ReorderableLazyListState? = null
    private var isDragging = false
    private var isDraggingWatchJob: Job? = null
    private var currentItems = initialItems

    /**
     * Will cache the current items when a drag begins.
     */
    val items = mutableStateOf(initialItems)

    /**
     * Must be called each time the parent Composable runs to keep the items up-to-date.
     */
    fun onCompose(newState: ReorderableLazyListState, newItems: List<T>) {
        currentItems = newItems
        if (!isDragging) {
            // It works without this, but it seems safer to not fire observers here since we don't need to,
            // since if the parent composable does a transform on the list, it might technically be a different
            // list each time, which might trigger an infinite loop.
            Snapshot.withoutReadObservation {
                items.value = currentItems
            }
        }

        val oldState = state
        state = newState

        if (oldState != newState) {
            /**
             * Launch a job to observe the draggingItemKey without invalidating our parent scope.
             * When we start dragging, we'll cache the current items while the drag runs.
             */
            isDraggingWatchJob?.cancel()
            isDraggingWatchJob = scope.launch {
                snapshotFlow {
                    newState.draggingItemKey
                }.collect { draggingItemKey ->
                    isDragging = draggingItemKey != null

                    // When we start dragging, update "items" with our current state, then
                    // ignore updates until we stop dragging. Notice that we're not updating them
                    // when a drag ends, which prevents a flash of undragged items and allows our
                    // parent to save and then recompose.
                    if (isDragging) {
                        Snapshot.withoutReadObservation {
                            items.value = currentItems
                        }
                    }
                }
            }
        }
    }
}