Fix scrolling when we change LazyListState #769
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes JetBrains/compose-multiplatform#3551 When the new state is passed to LazyList, it goes down to `Modifier.pointerScrollable` as `controller: ScrollableState`, and wraps to `rememberUpdatedState`, which is always the same instance. Because it is the same instance, MouseWheelScrollableElement.update is never called. Passing it is as a value now, as MouseWheelScrollableElement doesn't need a state.
igordmn
force-pushed
the
igor.demin/fix-recreating-list-state-scrolling
branch
from
e2ad8fe
to
e665cce
Compare
eymar
reviewed
Aug 23, 2023
.../foundation/src/skikoTest/kotlin/androidx/compose/foundation/gestures/SkikoScrollableTest.kt
Show resolved
Hide resolved
eymar
approved these changes
Aug 23, 2023
It seems there is test that checks equality of the modifier across compositions. Probably for optimization. Remembering scrolling logic seems logical here, and shouldn't add overhead. In 1.6 it was fully rewritten to modifier node: https://github.com/androidx/androidx/blob/a7e6bdf524be186baca08e9ddf7272b834a14bd2/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#L276
igordmn
commented
Aug 23, 2023
| .nestedScroll(nestedScrollConnection, nestedScrollDispatcher.value) | ||
| } | ||
|
|
||
| // {} isn't being memoized for us, so extract this to make sure we compare equally on recomposition. | ||
| private val NoOpOnDragStarted: suspend CoroutineScope.(startedPosition: Offset) -> Unit = {} | ||
|
|
||
| @Composable | ||
| private fun rememberScrollingLogic( |
There was a problem hiding this comment.
It seems there is test that checks equality of the modifier across compositions. Probably for optimization. remember fixes the test.
Remembering scrolling logic seems logical here, and shouldn't add overhead.
In 1.6 it was fully rewritten to modifier node:
There was a problem hiding this comment.
which test was failing before adding rememberScrollingLogic ?
eymar
approved these changes
Aug 24, 2023
igordmn
added a commit
that referenced
this pull request
Aug 24, 2023
* Fix scrolling when we change LazyListState Fixes JetBrains/compose-multiplatform#3551 When the new state is passed to LazyList, it goes down to `Modifier.pointerScrollable` as `controller: ScrollableState`, and wraps to `rememberUpdatedState`, which is always the same instance. Because it is the same instance, MouseWheelScrollableElement.update is never called. Passing it is as a value now, as MouseWheelScrollableElement doesn't need a state. * Remember ScrollingLogic It seems there is test that checks equality of the modifier across compositions. Probably for optimization. Remembering scrolling logic seems logical here, and shouldn't add overhead. In 1.6 it was fully rewritten to modifier node: https://github.com/androidx/androidx/blob/a7e6bdf524be186baca08e9ddf7272b834a14bd2/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#L276
igordmn
added a commit
that referenced
this pull request
Nov 15, 2023
* Fix scrolling when we change LazyListState Fixes JetBrains/compose-multiplatform#3551 When the new state is passed to LazyList, it goes down to `Modifier.pointerScrollable` as `controller: ScrollableState`, and wraps to `rememberUpdatedState`, which is always the same instance. Because it is the same instance, MouseWheelScrollableElement.update is never called. Passing it is as a value now, as MouseWheelScrollableElement doesn't need a state. * Remember ScrollingLogic It seems there is test that checks equality of the modifier across compositions. Probably for optimization. Remembering scrolling logic seems logical here, and shouldn't add overhead. In 1.6 it was fully rewritten to modifier node: https://github.com/androidx/androidx/blob/a7e6bdf524be186baca08e9ddf7272b834a14bd2/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#L276
igordmn
pushed a commit
that referenced
this pull request
Nov 16, 2023
* Set scroll progress state on mouse events * Animate mouse wheel scroll * Refactor changes * Add helper inline functions * Batch offsets * Filter zero values and apply raw delta only without animation * Use scrollBy inside animation, fix parameter usage in lambda * Fix tests * Add TODOs and comments * Fix more tests * Cleanup mouse hover test helper class * Add test for scroll during animation * Address feedback (part 1) * Make smooth scrolling configurable * Do not animate scroll on events from trackpads * Fix internal type for tests * Make animation scrolling speed density dependant * Fix conditionally applying scroll modifier * Fix cancel animation condition * Cache smooth scrolling flag * Adjust scrolling animation threshold * Add isScrollInProgress TODO * Add test for isSmoothScrollingEnabled flag * Reset flag in finally * Use strict comparison to detect consuming event mouse [bugfix, hard] foundation. mouse. Fix missing scrolling events (#527) * Move awaitPointerEventScope outside the loop * Add test for multi-event regression mouse [bugfix, simple] foundation. mouse. Handle scroll cancellation exception (#696) * Catch scroll cancellation exception * Fix demo mouse [feature, hard] foundation. mouse. Fix scrolling when we change LazyListState (#769) * Fix scrolling when we change LazyListState Fixes JetBrains/compose-multiplatform#3551 When the new state is passed to LazyList, it goes down to `Modifier.pointerScrollable` as `controller: ScrollableState`, and wraps to `rememberUpdatedState`, which is always the same instance. Because it is the same instance, MouseWheelScrollableElement.update is never called. Passing it is as a value now, as MouseWheelScrollableElement doesn't need a state. * Remember ScrollingLogic It seems there is test that checks equality of the modifier across compositions. Probably for optimization. Remembering scrolling logic seems logical here, and shouldn't add overhead. In 1.6 it was fully rewritten to modifier node: https://github.com/androidx/androidx/blob/a7e6bdf524be186baca08e9ddf7272b834a14bd2/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#L276 mouse [bugfix, simple] foundation. mouse. Take into account `enabled` in `scrollable` for mouse input (#880) Fixes JetBrains/compose-multiplatform#3816 ## Testing 1. The new test 2. Manual testing of `Modifier.verticalScroll(enabled = false`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes JetBrains/compose-multiplatform#3551
When the new state is passed to LazyList, it goes down to
Modifier.pointerScrollableascontroller: ScrollableState, and wraps torememberUpdatedState, which is always the same instance. Because it is the same instance, MouseWheelScrollableElement.update is never called.Passing it is as a value now, as MouseWheelScrollableElement doesn't need a state.
Testing