-
Notifications
You must be signed in to change notification settings - Fork 29.6k
Reimplemented StretchingOverscrollIndicator with Simulation ported from Android 12. #173849
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great improvement to the StretchingOverscrollIndicator, replacing the previous animation logic with a more physically realistic SpringSimulation. This results in a much more natural feel, especially for high-velocity flings, aligning it better with native Android behavior. The code is well-structured, and the addition of a regression test is appreciated.
I've found a critical issue related to animation controller lifecycle management that could lead to race conditions. I've also identified a couple of medium-severity issues related to code clarity and maintainability. Addressing these will make the implementation more robust and easier to understand.
Overall, this is a high-quality contribution.
|
Thank you for this amazing change! My first round of review focuses on the constants for the spring simulation. |
6bad613 to
5cf9c30
Compare
0ccff30 to
7d737d5
Compare
|
Checking in here: there are many failing tests behind the Google testing check here. I am working through them to validate the changes are expected, for example screenshot tests that captured the overscroll state. |
|
@MTtankkeo can you make this update in behavior opt-in? I have determined we won't be able to land this based on some of the failures coming from other dependencies that will have to update first once it gets to stable - https://pub.dev/packages/data_table_2 being one of them. |
@Piinks How should I implement the opt-in behavior? Personally, I can’t think of a good approach right now. Could you provide an example? |
|
One thing we could do is update data table to soften the affected tests, and roll the updated package into google, and then land this change. It looks like it's been 7 months since the package's last update. |
|
@MTtankkeo which do you prefer, updating the package and then returning here, or introducing an opt in? |
@Piinks Sounds good, I’ll look into fixing the failing tests in the data_table_2 package and open a PR soon. |
|
data_table_2 tests have been resolved, the only Google testing failures now are from image diffs. |
|
Looked through the screenshot tests, they all look intended with this PR. |
Fixed an issue #169659
This PR improves the StretchingOverscrollIndicator to better match native Android behavior.
This change ensures that momentum-based gestures produce a more natural and intuitive overscroll experience, especially for users accustomed to native Android scroll views.
Fling
3.mp4
Pull And Reduce
Also, a very similar animation is implemented when pulling as well.
3.mp4
Pre-launch Checklist
///).