[animations] Introduce layoutBuilder parameter to PageTransitionSwitcher

[federunco]

Fixesflutter/flutter#54585 by introducing layoutBuilder, which allows PageTransitionSwitcher to lay out its children in a custom manner.

[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.

[federunco]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[shihaohong]

@goderbauer This would solve the problem in flutter/flutter#54585 if Alignment.topLeft was passed into Stack.

I'm wondering if this should just be Alignment.topLeft by default, but also warn developers about having the passed child widgets be the same size or provide guidance on how to do so through documentation.

[goderbauer]

This PR is also missing tests. Can you please add some?

[goderbauer]

revert these formatting changes.

[goderbauer]

assert that alignment is not null and mention that in the doc above.

[goderbauer]

nit: its -> it's

[goderbauer]

Suggested change

	/// Defaults to Alignment.center
	/// Defaults to [Alignment.center].

[goderbauer]

It's not clear what "the original alignment" is.

[goderbauer]

revert formatting change

[goderbauer]

revert formatting change

[goderbauer]

I'm wondering if this should just be Alignment.topLeft by default, but also warn developers about having the passed child widgets be the same size or provide guidance on how to do so through documentation.

AlignmentDirectional.topStart may have been an alternative choice for the default.

This widget was heavily inspired by the AnimatedSwitcher (https://master-api.flutter.dev/flutter/widgets/AnimatedSwitcher-class.html), which also uses center as the default. That widget solves this problem by allowing the user to completely configure the layout via a provided layoutBuilder (https://master-api.flutter.dev/flutter/widgets/AnimatedSwitcher/layoutBuilder.html). We should probably do the same for this widget as well to give users maximum flexibility in case they want to change other aspects of the Stack.

[federunco]

Should I implement a layoutBuilder?

[shihaohong]

@federunco that seems like a more flexible way to approach this problem, since it allows devs to have a more fine-tuned control of the layout of the outgoing/incoming widgets.

[goderbauer]

fly-by comment: looks like the analyzer is unhappy. It may require the submission of #173 to fix that.

[shihaohong]

Some comments about the documentation. It also needs a test or two to ensure that the default layout is preserved, as well as another test to ensure that a modified layoutBuilder is properly used.

[shihaohong]

Since alignment had been removed:

Suggested change

	/// The [duration], [reverse], [transitionBuilder]and [alignment] parameters
	/// The [duration], [reverse], and [transitionBuilder] parameters

[shihaohong]

We should add some sample code here for one or two use cases for modifying the layoutBuilder here. Otherwise, developers may not know how to go to this parameter to resolve their problem.

Also, since this property's purpose might be less intuitive from just reading its name, it might be helpful to put a pointer to here from the constructor or class API docs as well. Something like: "If the layout of the transitioning child widgets are not what you are looking for, see layoutBuilder for suggestions on how to configure the layout of the incoming and outgoing child widgets."

[shihaohong]

Hey @federunco, any progress on this PR?

[federunco]

I should update the PR next wednesday, i'm quite busy at the moment

[shihaohong]

Sounds good, thanks for the update.

[shihaohong]

Seems like the formatter is still unhappy with one of the dart files.

[shihaohong]

The implementation itself looks good, there are two main types of comments:

1. Some clarifications for the API just so that our users aren't confused by how to use the new layoutBuilder parameter and the terminology surrounding it.
2. Seeing if there's some way to keep the instantiating ChildEntry private to the library, since we do not want our users getting the wrong idea on how to use it and trying to instantiate ChildEntry in their applications.

[shihaohong]

Since ChildEntry should not really be used as a "public API", but is necessary because it needs to be exposed as an argument for layoutBuilder, I think at the very least we should document how this is used for and that users of the animations package shouldn't be using this class unless they were using it together with PageTransitionSwitcher.layoutBuilder. This is my attempt to clarify this a little bit:

Suggested change

	/// Internal representation of a child that, now or in the past, was set on the
	/// PageTransitionSwitcher.child field, but is now in the process of
	/// transitioning.
	/// An internal representation of a child widget subtree that, now or in the past,
	/// was set on the PageTransitionSwitcher.child field and is now in the process of
	/// transitioning.
	///
	/// This class should not be used outside of the context of
	/// [PageTransitionSwitcher.layoutBuilder], which uses this class in a callback to
	/// customize the layout of the transitioning widgets.
	///
	/// For more information, please see the documentation of
	/// [PageTransitionSwitcher.layoutBuilder].

[shihaohong]

cc/ @goderbauer I've never tried this before, but would it be possible to make this constructor private while keeping the class itself public? That way we don't get users that attempt to create a ChildEntry outside of the context of the PageTransitionSwitcher widget, but they can still use it in the callback for layoutBuilder:

Suggested change

	ChildEntry({
	ChildEntry._({

[goderbauer]

Yes, you can do that :)

[shihaohong]

Use verbiage similar to other api documentation for dispose

Suggested change

	/// Animation disposal method
	/// Release the resources used by this object.
	///
	/// The object is no longer usable after this method is called.

[shihaohong]

Here are some grammatical suggestions. I also removed the usage of "active entries" since users will be more familiar with referring to the new and old child widgets based on prior documentation.

Suggested change

	/// PageTransitionSwitcher uses the property [layoutBuilder] to lay out all
	/// active entries.
	/// If the layout of the transitioning child widgets are not what you are looking
	/// for, see [PageTransitionSwitcher.layoutBuilder] for suggestions on how to
	/// configure the layout of the incoming and outgoing child widgets
	/// PageTransitionSwitcher uses the [layoutBuilder]property to lay out the
	/// old and new child widgets. By default, [defaultLayoutBuilder] is used.
	/// See the documentation for [layoutBuilder] for suggestions on how to
	/// configure the layout of the incoming and outgoing child widgets if
	/// [defaultLayoutBuilder] is not your desired layout.

[shihaohong]

nit:

Suggested change

	/// a [Column], that sizes its height depending on the heights of active entries.
	/// a [Column] that sizes its height depending on the heights of active entries.

[shihaohong]

We should define "active entries" here, since users might not get what these refer to. The only reference they have without peeking into the implementation is the concept of an "old" and "new" child widget, so this concept will need to be clarified.

[shihaohong]

Suggested change

	/// This is an [PageTransitionSwitcherTransitionBuilder] function.
	/// See [PageTransitionSwitcherTransitionBuilder]for more information on the function signature.

[shihaohong]

Suggested change

	/// The layout builder used as the default value of [layoutBuilder].
	/// The default layout builder for [PageTransitionSwitcher].

[shihaohong]

Also, doesn't the [Stack] size itself to match the largest child, not the previous child?

Suggested change

	/// The new child is placed in a [Stack] that sizes itself to match the
	/// largest of the child or a previous child. The children are centered on
	/// each other.
	/// This function is the default way for how the new and old child widgets are placed
	/// during the transition between the two widgets. The new child is placed in a
	/// [Stack] that sizes itself to match the largest of the child or a previous child.
	/// The children are centered on each other.

[federunco]

I've pushed all the suggested changes, how about making ChildEntry constructor private, any news?

[shihaohong]

@federunco I think we should make the constructor private. That way, nobody can accidentally abuse the ChildEntry class.

[shihaohong]

LGTM modulo a doc comment nit.

@goderbauer could you take a look to make sure this looks good?

[shihaohong]

Suggested change

	// An internal representation of a child widget subtree that, now or in the past,
	/// An internal representation of a child widget subtree that, now or in the past,

[goderbauer]

Added to my review queue. Sorry for the delay.

[goderbauer]

Suggested change

	/// was set on the PageTransitionSwitcher.child field and is now in the process of
	/// was set on the [PageTransitionSwitcher.child] field and is now in the process of

[goderbauer]

A comment on a public member should nor reference a private member.

[goderbauer]

_activeEntries -> activeEntries

Or why not just entries?

[goderbauer]

remove the first "is".

[goderbauer]

The see also section should always come last.

[goderbauer]

This seems a little confusing. The layoutBuilder puts all children in a column, not just the new one.

[goderbauer]

What effect is this example trying to achieve? It seems odd to just place them all in a column?

[goderbauer]

not just the new child, all children are placed in a stack.

[goderbauer]

Yes, you can do that :)

[goderbauer]

Why do we hand the full ChildEntry to the layoutBuilder? It seems that handing them just the transition Widgets from the ChildEntries would be enough. The layout builder shouldn't have access to the animation controllers in ChildEntry. Those shouldn't be messed with.

Similarly to AnimatedSwitcher's layoutBuilder it probably also makes sense to hand over the currentEntry and all other entries in separate arguments.

[goderbauer]

In other words, _ChildEntry should stay private.

[goderbauer]

In other words, _ChildEntry should stay private.

[federunco]

I made all the suggested changes, I used most of the AnimatedSwitcher code (just deleted some of the assertions that didn't make sense when the switcher was set in reverse and tweaked the code a bit) but I didn't test it on a real device, all the unit test passed btw.

[goderbauer]

Leave the comment about how we don't want to expose some of these fields?

[goderbauer]

nit: leave this comma as is, https://en.wikipedia.org/wiki/Serial_comma

[goderbauer]

nit:

Suggested change

	Widget currentChild, List<Widget> previousChildren, bool reverse) {
	Widget currentChild, List<Widget> previousChildren, bool reverse,) {

... and then format with flutter format

[goderbauer]

Why is this commented out?

[goderbauer]

use { } around the body of the if and put it on a separate line.

[goderbauer]

leave this as-as

[goderbauer]

use { } around body and put body on new line

[goderbauer]

Suggested change

	void_markChildWidgetCacheAsDirty() => _outgoingWidgets =null;
	void_markChildWidgetCacheAsDirty() {
	_outgoingWidgets =null;
	}

[goderbauer]

This will not work: It will cause the order of widgets to change unexpectedly if _currentEntry was previously shown below all _outgoingEntries instead of on top of it.

[goderbauer]

Maybe the layout builder for this widget should just get a List of widgets without knowledge of the current entry, to avoid this problem?

[federunco]

So the old activeEntries list, right?

[goderbauer]

Also: the analyzer check is unhappy. You may have to rebase this PR to the latest master.

[shihaohong]

It seems to still be failing the analyzer check. Make sure to run flutter analyze on the animations package repo and correct any warnings/errors to make sure that everything looks good.

[shihaohong]

Still LGTM, with a nit on the sample code

[goderbauer]

Generally, this looks good now after the two comments are resolved.

[goderbauer]

This comment seems outdated. It's not used as part of the layoutBuilder anymore?

[goderbauer]

Ideally, examples should show good practices. I can't think of any good use cases where putting the children in a column is a good idea. Maybe show an example where a Stack with a different alignment is used (e.g. topleft.)

[shihaohong]

The latest changes look like they address @goderbauer 's concerns and he's on vacation, so I'll merge the PR for now. If he has any issues with it, we can always just revert it or fix forward. Thank you so much for working through this PR!