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

Fix missing scrolling events #527

Merged
merged 2 commits into from Apr 26, 2023
Merged

Conversation

MatkovIvan
Copy link
Member

@MatkovIvan MatkovIvan commented Apr 25, 2023

Proposed Changes

  • Move awaitPointerEventScope outside the loop

Testing

Test: Use sample from JetBrains/compose-multiplatform#3099

Issues Fixed

Fixes: JetBrains/compose-multiplatform#3099

@MatkovIvan MatkovIvan marked this pull request as ready for review April 25, 2023 15:32
@MatkovIvan MatkovIvan requested a review from eymar April 25, 2023 15:34
@MatkovIvan MatkovIvan merged commit 6fb2531 into jb-main Apr 26, 2023
2 checks passed
@MatkovIvan MatkovIvan deleted the ivan.matkov/fix-multiscroll branch April 26, 2023 14:13
igordmn pushed a commit that referenced this pull request Jun 7, 2023
* Move awaitPointerEventScope outside the loop

* Add test for multi-event regression
igordmn pushed a commit that referenced this pull request Jul 18, 2023
Test: ./gradlew test connectedCheck

This is an imported pull request from androidx#527.

Resolves #527
Github-Pr-Head-Sha: 55cd2d4
GitOrigin-RevId: 7654dca
Change-Id: I37896040ac6cefc3560a0b4ffbebf37abb958ca4
igordmn pushed a commit that referenced this pull request Nov 15, 2023
* Move awaitPointerEventScope outside the loop

* Add test for multi-event regression
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
Labels
None yet
Projects
None yet
2 participants