Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
distinctdan opened this issue Aug 3, 2023 · 3 comments
Open

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

distinctdan opened this issue Aug 3, 2023 · 3 comments

Comments

@distinctdan
Copy link

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:

  • Pass a list to LazyList.items that isn't the list updated in onMove, I'm generating it in a remember block based on the sort order list.
  • Drag the first item down.
  • Observe that the item jumps 3 spaces down instead of just 1.

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
Copy link

yap had the same issue

@distinctdan distinctdan changed the title Isn't stable when keeping track of sort order in a separate list Isn't stable if the parent component recomposes during a drag Nov 20, 2023
@distinctdan
Copy link
Author

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
Copy link
Author

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
                        }
                    }
                }
            }
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants