Fix restart ballistic animation while child size change #96512

MxSoul · 2022-01-12T07:44:58Z

Hi, @Piinks
If child size change, performLayout will be call. And goBallistic will be called during performLayout. It will restart ballistic animation. Restarting the animation will cause the ballistic to be inconsistent with the expected curve.

I don't think the animation should be restarted during a ballistic even if its boundaries change. Maybe we should update related variables instead.

【I already do some test. I will post these data later】

Fix #11599 & #24715

This issue #83632 is also caused by it.
So maybe I will reland the #77497 after this PR merge.

cc @knopp @nt4f04uNd
The solution is referenced knopp's comment. #24715 (comment)

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-01-12T07:45:02Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Piinks

This looks like a great PR, thank you for taking a crack at solving these issues! I should be able to give it a thorough review next week, in the meantime, can you take a look at the failing tests? Is this intended to be a breaking change?

packages/flutter/lib/src/widgets/scroll_simulation.dart

MxSoul · 2022-01-24T14:33:43Z

This looks like a great PR, thank you for taking a crack at solving these issues! I should be able to give it a thorough review next week, in the meantime, can you take a look at the failing tests? Is this intended to be a breaking change?

I am tring to fix the test case.
It will not be a breaking change. Because I will do something to make it compatible with other simulation.
I am not sure whether the current way to fix is the best. If we do some breaking, maybe we could have a better plan

…maxScrollExtent.

…tion # Conflicts: # packages/flutter/lib/src/widgets/scroll_simulation.dart

Piinks

Thanks for working on this @MxSoul, it is not a trivial thing to do as a first-time contributor! (Welcome too! 🎉)

As it currently stands, I am not sure if this implementation will work. Although, I may not be following it fully. Have you considered writing a design document? We usually write up large scale changes in a document so that we can review and give feedback on the concept. It also helps in discussing potential breaks, migration and testing plans, the template is available here: https://docs.google.com/document/d/1SFRO8U2toOlAaZ38dsuEU7Wm5fn41wvBCWKiwADqfmw/edit?usp=sharing

If this is a breaking change, the process is documented in our wiki: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

packages/flutter/lib/src/widgets/scroll_activity.dart

packages/flutter/lib/src/widgets/scroll_physics.dart

packages/flutter/lib/src/widgets/scroll_simulation.dart

2. change another way to get scrollPosition

MxSoul · 2022-01-26T13:15:33Z

Thanks for working on this @MxSoul, it is not a trivial thing to do as a first-time contributor! (Welcome too! 🎉)

As it currently stands, I am not sure if this implementation will work. Although, I may not be following it fully. Have you considered writing a design document? We usually write up large scale changes in a document so that we can review and give feedback on the concept. It also helps in discussing potential breaks, migration and testing plans, the template is available here: https://docs.google.com/document/d/1SFRO8U2toOlAaZ38dsuEU7Wm5fn41wvBCWKiwADqfmw/edit?usp=sharing

If this is a breaking change, the process is documented in our wiki: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

@Piinks Thanks for your review.
I optimize some implementation And I am writing the design document right now. Maybe it will finish at tomorrow :)

MxSoul · 2022-01-27T13:44:35Z

@Piinks Hi~ I write a doc. Please take a look when you have time.
I am not native speaker, it depend on Google Translate. So maybe have some grammar problem. :P
https://docs.google.com/document/d/1J39wkBaz1lYn7kmT7wuN8mKAvIad98dSOvxzYrDq28Q/edit?resourcekey=0-hCGe2gHb01bP9CvX31TrRQ#

MxSoul · 2022-02-06T17:03:19Z

Hi @Hixie & @nt4f04uNd ,
Maybe someone could help me to review this PR ? Sorry to bother you.

(*Piinks is out of office right now.)

MxSoul · 2022-02-10T02:47:39Z

@goderbauer Looking forward to your review

goderbauer

@MxSoul looks like some of the feedback and questions that @Piinks had haven't been addressed yet. Could you respond to those before requesting another review?

Also, this PR is missing tests. Please add some.

goderbauer · 2022-02-10T23:41:04Z

packages/flutter/lib/src/widgets/scroll_activity.dart

-    delegate.goBallistic(velocity);
+    final ScrollPosition position = delegate.scrollPosition;
+    final dynamic simulation = _simulation;
+    if (position != null && simulation is ScrollSimulationMixin){


nit: space between ) and {

The is ScrollSimulationMixin is also iffy as was already called out in the previous review, I think?

This has not been addressed yet, and cannot be accepted in its current state.

Piinks · 2022-02-25T00:03:49Z

@MxSoul would you like to continue working on this change?

MxSoul · 2022-02-25T13:01:24Z

@MxSoul would you like to continue working on this change?

Hi, @Piinks. Welcome back.
Sure, I will continue working on it. I am so sorry about last 2 week is too busy. I will finish the test case about next Wednesday(March 2).

Maybe We could discuss the implementation first. Is there anything in the design document that is still unclear? I can add some explanation.

(By the way, this patch is use in our app's(Xianyu) search result page and over 7 million user used it every day. So far, everything seems to be fine. No one has reported any bugs.)

Piinks · 2022-02-25T17:59:57Z

It looks like some of the comments in previous code reviews are unaddressed.

Piinks · 2022-02-25T18:45:49Z

Also, your design document is not open for commenting. Can you update the permissions to allow feedback?

MxSoul · 2022-02-27T15:20:33Z

Also, your design document is not open for commenting. Can you update the permissions to allow feedback?

OK~ I update the permissions.

MxSoul · 2022-02-27T15:22:25Z

It looks like some of the comments in previous code reviews are unaddressed.

This is probably because I fixed some of the issues mentioned in the comments and some of the code was removed

skia-gold · 2022-03-02T13:13:05Z

Gold has detected about 24 new digest(s) on patchset 13.
View them at https://flutter-gold.skia.org/cl/github/96512

skia-gold · 2022-03-02T14:53:05Z

Gold has detected about 30 new digest(s) on patchset 14.
View them at https://flutter-gold.skia.org/cl/github/96512

skia-gold · 2022-03-02T15:23:05Z

Gold has detected about 32 new digest(s) on patchset 15.
View them at https://flutter-gold.skia.org/cl/github/96512

MxSoul · 2022-03-02T15:55:47Z

I addressed comments and add test. But it make some test break. I will fix it tomorrow

Piinks

Thanks for the update, here are just some thoughts for when you return to this. There are still a lot of test failures and analysis issues to resolve before we can review further.

packages/flutter/lib/src/physics/spring_simulation.dart

packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart

packages/flutter/lib/src/widgets/nested_scroll_view.dart

packages/flutter/lib/src/widgets/scroll_activity.dart

packages/flutter/lib/src/widgets/scroll_physics.dart

packages/flutter/lib/src/widgets/scroll_activity.dart

packages/flutter/lib/src/widgets/scroll_simulation.dart

This reverts commit 33665b6.

This reverts commit 32cebd4.

MxSoul · 2022-03-09T14:28:35Z

Hi @Piinks, We may all have some questions to ask each other, because of the time zone, the use of issue communication is a bit inefficient. Could you create a subsection in discord and we use it to communicate. My discord id is MxSoul#7735
I will be online at GMT-8 2022.03.09 16:30 (GMT+8 2022.03.10 08:30).

If this time is not suitable, you can leave a message and tell me a time when you are free. (I am at GMT+8).

…tion # Conflicts: # packages/flutter/lib/src/widgets/scroll_simulation.dart

Piinks · 2022-03-09T18:06:31Z

Hey @MxSoul please do feel free to share your questions. I can answer them asynchronously if we are not online at the same time. The #hackers-framework channel is probably a good place to chat, you can start a thread there if you like. :)

goderbauer · 2022-03-23T21:43:59Z

(Triage) closing this one in favor of #100133.

do not restart scroll animation while performLayout

70238fb

flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jan 12, 2022

MxSoul changed the title ~~Fix restart ballistic animation while child size change~~ [WIP] Fix restart ballistic animation while child size change Jan 12, 2022

Roll back comment changes

2e84c6b

goderbauer requested a review from Piinks January 12, 2022 22:34

fix Linux analyze exception

bc894b5

Piinks reviewed Jan 22, 2022

View reviewed changes

packages/flutter/lib/src/widgets/scroll_simulation.dart Outdated Show resolved Hide resolved

MxSoul added 2 commits January 24, 2022 23:18

fix list can not scroll back , when currentPixel is greater than new …

507ae4f

…maxScrollExtent.

Merge remote-tracking branch 'origin/master' into fix_ballistic_anima…

5d70812

…tion # Conflicts: # packages/flutter/lib/src/widgets/scroll_simulation.dart

Piinks added a: quality A truly polished experience c: performance Relates to speed or footprint issues (see "perf:" labels) labels Jan 24, 2022

Piinks requested changes Jan 24, 2022

View reviewed changes

MxSoul added 2 commits January 25, 2022 11:27

fix RefreshIndicator test case

e6d343b

1. fix some style problem.

e2ef298

2. change another way to get scrollPosition

MxSoul changed the title ~~[WIP] Fix restart ballistic animation while child size change~~ Fix restart ballistic animation while child size change Jan 25, 2022

flutter-dashboard bot added the a: text input Entering text in a text field or keyboard related problems label Jan 25, 2022

revert format

f7320da

MxSoul requested a review from Piinks January 26, 2022 02:59

re-new ScrollSpringSimulation because it use boundary values

cea05c0

MxSoul force-pushed the fix_ballistic_animation branch from 687817f to cea05c0 Compare January 26, 2022 12:22

MxSoul added 3 commits January 27, 2022 11:02

add doc and optimize the impl

bdc07be

fix complie

565ecf7

update doc

b5e83e0

goderbauer reviewed Feb 10, 2022

View reviewed changes

make BallisticScrollActivity._simulation be a definite type

32cebd4

fix bad import

33665b6

add test

3dbd8ea

MxSoul requested a review from goderbauer March 2, 2022 15:19

Piinks reviewed Mar 2, 2022

View reviewed changes

MxSoul force-pushed the fix_ballistic_animation branch 2 times, most recently from 607c5fc to 3dbd8ea Compare March 4, 2022 08:45

MxSoul added 2 commits March 4, 2022 16:46

Revert "fix bad import"

fc2a64e

This reverts commit 33665b6.

Revert "make BallisticScrollActivity._simulation be a definite type"

ab9dc92

This reverts commit 32cebd4.

MxSoul added 3 commits March 9, 2022 22:32

Merge remote-tracking branch 'origin/master' into fix_ballistic_anima…

4b49d95

…tion # Conflicts: # packages/flutter/lib/src/widgets/scroll_simulation.dart

fix test after revert

5175cad

remove unused import

1f92492

MxSoul mentioned this pull request Mar 15, 2022

Update animation simulation to fix restart ballistic animation #100133

Closed

8 tasks

goderbauer closed this Mar 23, 2022

MxSoul mentioned this pull request Mar 25, 2022

[Proposal] Update animation simulation to fix restart ballistic animation which is used by NestedScrollView #100748

Closed

Piinks mentioned this pull request Jul 15, 2022

Update Ballistic animation & ClampingScrollSimulation #107735

Merged

8 tasks

Fix restart ballistic animation while child size change #96512

Fix restart ballistic animation while child size change #96512

Uh oh!

Conversation

MxSoul commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 12, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MxSoul commented Jan 24, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MxSoul commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MxSoul commented Jan 27, 2022

Uh oh!

MxSoul commented Feb 6, 2022

Uh oh!

MxSoul commented Feb 10, 2022

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

goderbauer Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks commented Feb 25, 2022

Uh oh!

MxSoul commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Feb 25, 2022

Uh oh!

Piinks commented Feb 25, 2022

Uh oh!

MxSoul commented Feb 27, 2022

Uh oh!

MxSoul commented Feb 27, 2022

Uh oh!

skia-gold commented Mar 2, 2022

Uh oh!

skia-gold commented Mar 2, 2022

Uh oh!

skia-gold commented Mar 2, 2022

Uh oh!

MxSoul commented Mar 2, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MxSoul commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MxSoul commented Jan 12, 2022 •

edited

Loading

MxSoul commented Jan 26, 2022 •

edited

Loading

MxSoul commented Feb 25, 2022 •

edited

Loading

MxSoul commented Mar 9, 2022 •

edited

Loading