Update Ballistic animation & ClampingScrollSimulation #107735
Conversation
Piinks
left a comment
There was a problem hiding this comment.
Checked against internal test suite, this will be able to land without breaking customers. ✅
Piinks
left a comment
There was a problem hiding this comment.
CC-ing interested parties @grouma @MxSoul @goderbauer @nt4f04uNd
I am still following up on a few things here, but you're feedback is welcome. I have merged the ballistic activity update with the clamping simulation update.
They kind of validate each other. See https://github.com/flutter/flutter/pull/77497/files#r705784804 - this test change (the original regression) is no longer reproducing with them combined.
grouma
left a comment
There was a problem hiding this comment.
Fantastic!
It looks like the scrolling is ever so slightly off in your video (but much much better). The scrolling logic should essentially be the same with an equal number of spline samples. I wonder what is causing the minor differences?
Nonetheless, I will be extremely happy when this lands. I'm particularly pleased with the removal of the "snapping" and the addition of this test.
|
Thanks @Piinks |
packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart
Outdated
Show resolved
Hide resolved
| initScrollMetrics, initVelocity); | ||
| if (simulation == null) { | ||
| goIdle(); | ||
| return null; |
There was a problem hiding this comment.
should goIdle really be called here?
I think this check should be on the side of applyNewDimensions
There was a problem hiding this comment.
I think it is called here in updateBallistic.. similar to how it is in goBallistic.
| /// | ||
| /// The current simulation will be replaced with the provided [Simulation]. | ||
| void updateSimulation(Simulation simulation) { | ||
| _simulation = simulation; |
There was a problem hiding this comment.
this should perhaps assert against being called without active simulation, or just be a no-op action
docs should also say what it does in this case
There was a problem hiding this comment.
Interesting, I actually just removed an assertion that it is currently animating based on the original PR, see this comment: https://github.com/flutter/flutter/pull/100133/files#r844471344. @MxSoul you mentioned there that the assertion is not necessary, do you remember why?
There was a problem hiding this comment.
I am adding this assertion back. If we find it is an issue, we can investigate further and remove it if necessary.
|
Should this PR also remove this test? See comments here as to why https://github.com/flutter/flutter/pull/97166/files#diff-0181ef59af5a74e790689c74db9264d831b7fcb957688b0424de0da1bf279ca5 Original PR where this was added https://github.com/flutter/flutter/pull/59623/files |
I don't think so, but I will double check. I remember that test and how it was blocking the page scroll physics update you did @nt4f04uNd. I referenced that above. My plan here is to resolve this, so I can unblock and return to that change. I figure we can revisit that test then, WDYT? |
Sounds good to me |
|
I'll be returning to this next week with updates, I am handling a few fires elsewhere this week. 🔥 😅 |
|
|
Blocked on #109797 |
|
|
😮 super amazing! Thank you Kate! |
[super_sliver_list](https://superlistapp.github.io/super_sliver_list/#/example/item-list) is an alternative to `SliverList` (and `ListView`) with different performance characteristics and ability to reliably jump / animate to any item. It relies heavily on `scrollOffsetCorrection` in a way no other sliver in Flutter does and thus may be somewhat prone to breaking (for example it was broken in past by flutter/flutter#107735, which was reverted later).
Status: WIP, following up on past review comments and concerns
Customer impact: ✅ non-breaking
Related PRs:
Related Issues:
Before
Screen.Recording.2022-07-14.at.3.43.32.PM.mov
After
Screen.Recording.2022-07-14.at.3.23.16.PM.mov
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.