[animations] Adds CompositeAnimatedWidget to ensure shape of entering and exiting subtree are the same.

[grinder15]

Description

Transition widgets like FadeScaleTransition, FadeThroughTransition, and SharedAxisTransition is changing their subtree levels of widgets when transitioning causing the child state object to get recreated.

CompositeAnimatedWidget solves the issue, instead of rebuilding animation widgets in AnimatedBuilder, it builds its transitions, stacked and only changing animation values to achieve different transitions on forward and reverse.

Issue

flutter/flutter#53550
flutter/flutter#55826
flutter/flutter#55874

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[grinder15]

@googlebot I signed it!

…

On Mon, Apr 20, 2020, 10:53 PM googlebot ***@***.***> wrote: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 *Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign.* Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it. ------------------------------ What to do if you already signed the CLA Individual signers - It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. Corporate signers - Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version <https://opensource.google/docs/cla/#troubleshoot>). - The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. - The email used to register you as an authorized contributor must also be attached to your GitHub account <https://github.com/settings/emails>. ℹ️ *Googlers: Go here <https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fflutter%2Fpackages%2Fpull%2F141> for more info*. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#141 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGI5TGFO4U7HAOIFZSG3HTDRNROYFANCNFSM4MMQG5RA> .

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[goderbauer]

this should be a relative import.

[goderbauer]

(Haven't looked at this PR in details yet) Can you please fix the failing Cirrus check: https://github.com/flutter/packages/pull/141/checks?check_run_id=602145199

[grinder15]

I just did a bit of test and I found out that the issue that I created before can be easily fixed using GlobalKeys.

https://gist.github.com/grinder15/fb908c42b5f968b89476b6216d17953d

I also checked the state of widget when transitioning in a route is also fine.
I hope this can also be include in the docs of using keys when using transition widgets.
I'm gonna close this PR and the linked issue,

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[goderbauer]

Some high-level feedback: This is pretty cool!

Maybe we should simplify the CompositeAnimatedWidget a little bit by removing the flip and syncVisibleStates flag. Instead, we define that the animations given to the enterBuilder and exitBuilder will always run from 0.0 to 1.0. The enterBuilder is supposed to make the widget appear as its animation runs from 0.0 to 1.0 and the exit builder is supposed to make the animation disappear as it runs from 0.0 to 1.0. We then adjust the individual enter and exit Builder passed into the CompositeAnimatedWidget by e.g. FadeScaleTransition, FadeThroughTransition, and SharedAxisTransition to match this model.

Last, but not least, can you also add tests to this PR please? Otherwise we will not be able to merge it as per our policy.

[grinder15]

@godofredoc Thanks for the review, I'll apply those changes this coming weekend. No idea though to how to do a test, I think I'll start with CompositeAnimationWidget ensuring animation values 0.0 to 1.0 in both builders, any thoughts? hints?

First time to contribute, so please be nice to me, 😅 Thanks!

[shihaohong]

@grinder15 I think you meant to tag @goderbauer :)

With regards to testing, I think that's a good start for testing the functionality of CompositeAnimationWidget. I would also add a test for each of the FadeScaleTransition, FadeThroughTransition, and SharedAxisTransitions that now use the CompositeAnimatedWidget to make sure that state is not lost after the transition animations complete in forwards, or in reverse. I can try to help out with the tests if you get stuck

[goderbauer]

It's not entirely clear to me why we need this setting for the FadeThroughTransition and SharedAxisTransition. The FadeThroughTransition and SharedAxisTransition should just define their forwardTransitionBuilder/reverseTransitionBuilder properly.

[grinder15]

To better understanding, lets take a look at this table

The use of visibleAtStart explicitly telling the widget that we want to define an "exit"/"disappear" animation, thus the secondary animation part of some page transition widgets. This is also the reason why I renamed those builders to avoid confusion like defining an "exit" animation in enterBuilder and "reenter" animation in exitBuilder.

On the technical side, we want the widget to know where those "values" stays while the other builder is animating. If we don't set the flag correctly, it will end up the builder to animate but we cant see the actual animation since all animation widgets defined are built by composition being the forwardBuilder is the parent and the child is the reverseBuilder. For example, if the reverseBuilder's FadeTransition opacity stays at '0.0' and the forwardBuilder's ScaleTransition is animating, we can't see the animation.

return widget.forwardTransitionBuilder(
widget.reverseTransitionBuilder(
child,
_reverseAnimation,
),
_forwardAnimation,
);

visibleAtStart sets the x values to 0.0 if true, else 1.0.

With your suggestion about reverseBuilder must run forward, I can simply add a flip function to _reverseAnimation .

[goderbauer]

I would also add a test for each of the FadeScaleTransition, FadeThroughTransition, and SharedAxisTransitions that now use the CompositeAnimatedWidget to make sure that state is not lost after the transition animations complete in forwards, or in reverse.

I also like @shihaohong's suggestion of adding a test for this.

[shihaohong]

I tried to write a test for the SharedAxisTransition case: grinder15#1

It seems we only run into this animation problem if we ever use the transition in tandem with the PageTransitionSwitcher. I wonder if there's a more general solution that can be applied to PageTransitionSwitcher and not have to use the CompositeAnimatedWidget for every transition that is ever used with PageTransitionSwitcher. For example, using PageTransitionsBuilder with SharedAxisTransitionsPageBuilder does not cause the problem because the widget tree shape does not alter.

[grinder15]

@shihaohong Thanks for the sample test, I will add these with my later commits.

About the animation problem, I also tried to use page transition widgets to hook it up directly to AnimationController, the same problem occurs with stateful widgets.
I think it is good to think those page transition widgets has the same use cases as the plain old transition widgets in flutter (ScaleTransition, FadeTransition, etc.) like respond to drag events, adding animation in AnimatedList, or animate when it is mounted in ScrollView.

English is not my primary language but I hope you get my point :)

[shihaohong]

@grinder15 I think I sort of get what you mean, but maybe an easy way to demonstrate is with a short code sample?

From what I'm reading, it sounds like if you directly pass in AnimationController into, for example, FadeScaleTransition.animation, and drive the animation forwards/backwards programmatically, you might still get the same issue?

Either way, I get your point about having the transition widgets deal with this by default. This would prevent this kind of problem from existing in use cases other than the PageTransitionSwitcher.

Don't worry, English wasn't my first language either, but I have had a lot of practice :)

[grinder15]

@shihaohong
https://gist.github.com/grinder15/e3cc69ad82a545407f0eaa2c28355e65
And it goes like this..

As you transition in and out, it resets the counter widget which is suppose to persist.

[shihaohong]

@grinder15 That makes sense, thanks for sharing the demo!

[goderbauer]

@grinder15 Inspired by your work I took a stab at this in #160. My main change there is that I eliminated the need for the visibleAtStart flag, which I think makes the widget easier to use and understand. Instead, I am adapting the animation that gets passed into the widget to match the expected model. I also changed some of the names. Let me know what you think about that approach and whether I am missing a use case that doesn't work in my approach.

[shihaohong]

Since #160 has been merged, I will now close this issue. @grinder15 Thank you for the initial solution and investigation! I think this was a really cool idea and helped shape a much cleaner solution to the problem

[zenas]

@shihaohong
https://gist.github.com/grinder15/e3cc69ad82a545407f0eaa2c28355e65
And it goes like this..

As you transition in and out, it resets the counter widget which is suppose to persist.

Meet the same problem，how are things going？

[grinder15]

@xhowe This was already solved in the latest version. Try to update your pubspec.yaml