seems a bit weird the use the word layout, maybe just build. also maybe mention where the header and body comes from. e.g. are the the same widget returned from headerBuilder and bodyBuilder?

also since this allows arbiturary position of header and body, should we just have one ExpansibleBuilder and remove the header and body builder?

also since this allows arbiturary position of header and body, should we just have one ExpansibleBuilder and remove the header and body builder?

That would be nice if the header and body were just outputs of headerBuilder and bodyBuilder. But internally the result of headerBuilder is wrapped with a GestureDetector to toggle the expansion, and the result of bodyBuilder is wrapped in Offstage and also used for the static part of the internal AnimatedBuilder.

Is it still the case? I don't see any GestureDetectors in ExpansibleState any more. Does it allow a better name now?

Since this is existing API, so no immediate action item in this PR. This is something that may potentially be dangerous, if a widget somehow relies on this to build, there isn't a way to listen for change.

Is there a generally accepted solution for these kinds of situations?

IMO no, either this controller be a changenotifier so that people can listen to this property, or we should remove this property getter

In a bigger picture we should remove duplicate API in Expandible and this controller.

1. Either the controller is stateful and changenotifier where it is the single source of truth. We should remove the initialExpand and onExpandChange callback on this widget. If people want to listen to onExpandChange they should just listen to the controller.

2. remove this controller and keeps initialExpand and onExpandChange, and expose an API on ExpandibleState to toggle expand. If developer want to toggle the expand, they need to provide a globalkey to this widget and use it to call the toggle api on the state

I was able to remove onExpansionChanged by making the controller extend ChangeNotifier. But I can't remove initiallyExpanded because the controller state object is empty until it is set in Expansible's initState. So we can't call controller.expand in ExpansionTile's initState. Is there a way to work around this?

Also if we are maximizing using the controller since it is required, can we remove the isExpanded parameter from the builder callbacks and have consumers use controller.isExpanded so that would be the single source of truth?

I can't remove initiallyExpanded because the controller state object is empty until it is set in Expansible's initState. So we can't call controller.expand in ExpansionTile's initState. Is there a way to work around this?

The isExpand should be property stored in controller instead of ExpansibleState. ExpansibleState will build based on the controller's property. right now it is the other way around.

For developer who wants set the initial state, they should set it on the controller and pass it into the Expandible

Also if we are maximizing using the controller since it is required, can we remove the isExpanded parameter from the builder callbacks and have consumers use controller.isExpanded so that would be the single source of truth?

I think having a expand here is fine as long as the build method will be call every time this value change. If not, then we should remove this property and ask people to build based on the controller's state

For developer who wants set the initial state, they should set it on the controller and pass it into the Expandible

I'm having an annoying bug here. If we add onExpansionChanged as a listener to isExpanded, when the developer sets the initial state, it calls onExpansionChanged even though the expansion didn't actually change.

Edit: Fixed, I didn't set up my setter correctly.

Duplicating animation controller by hooking them up in this method feels error prone.

I don't think passing in the controller from material/cupertino and mutate it in Expandible is a concern. We do that all the time with TextEditingController and FocusNode. as long as the controller do not get disposed before the expandible is disposed, we should be fine

The main issue I have with passing the animation controller from material/cupertino into Expansible is that we already require the ExpansibleController which is fully controlled by Expansible. So passing it in would mean Expansible requires two controllers, the ExpansibleController and the animation controller which will then be mutated in the expansible. ExpansionTile needs the animation controller to sync other animations to the expansion animation (e.g the icon turning), but other consumers of Expansible may not have any extra animations that need the animation controller. But in those cases, they would still need to create and dispose one just to pass it in to Expansible. It just seems like a cleaner API surface if users manage any extra animations outside Expansible, but are still able to sync them up to change when the expansion state changes.

For possible errors with this approach, the one that comes to mind is if the duplicate animation controller is somehow different from the internal one (in duration for example). But that's not really an issue since the duration gets passed into Expansible anyways. And it could even be a benefit if the consumer of Expansible wants to add an extra animation that is different (e.g faster/slower) but still syncs up with the expansion animation.

Or maybe there could be more errors I haven't thought about.

Expansible requires two controllers, the ExpansibleController and the animation controller

I don't see this as a problem.

but other consumers of Expansible may not have any extra animations that need the animation controller

We generally won't expect people to directly use building block widget like this one., so having this restriction in widget layer is fine. One way to mitigate this is to make animationcontroller a optional parameter and autogenerate internal one in ExpansibleState if not provided. but I am also oppose this as this is a building block widget, and can be error prone when disposing resource

We generally won't expect people to directly use building block widget like this one

I'm not entirely sure about this. For example, RawAutocomplete is supposed to be another building block widget. But many use cases I've seen (e.g Zulip) ditch Autocomplete and just use RawAutocomplete because of more customization opportunities and less ties to material.

This expanding/collapsing behavior is very common. There is a main Cupertino variant (CupertinoCollapsible with the rotating icon), but there are also other places in Cupertino where this widget would be used directly (e.g inline date/time pickers). And extrapolating to other design languages as well.

One way to mitigate this is to make animationcontroller a optional parameter and autogenerate internal one in ExpansibleState if not provided.

But this will require creating a fallback controller with it's own issues.

But then on the flip side we are already requiring the ExpansibleController anyways. And an extra animation controller in material/cupertino feels like a hack. Although I'm still wondering about other potential cons.

But many use cases I've seen (e.g Zulip) ditch Autocomplete and just use RawAutocomplete because of more customization opportunities and less ties to material.

I think if customer decide to build their own widget on top of Expandible, creating their own animationcontroller feels like a ok compromise.

If we really want to make it easier to use, I would choose a fallback controller instead of attempting to sync two controllers. We have example of attempting to sync two controller (tab controller and pagecontroller) before in TabBar and TabBarView, and it is very messy. It is a more extreme case compare to this one, but I would like to avoid any future case where this widget spiral into another tabbar and tabbarview.

I would like to avoid any future case where this widget spiral into another tabbar and tabbarview.

I see. Although looking at TabBar and TabBarView, the complexity comes from the controllers sharing information (e.g. the offset). But in this case, the controllers (and their animations) are completely independent, the only connection they have is that they are both called when the expansion state changes (so more like independent listeners to the expansion state vs. synced). Plus the expansible's animation controller is private. Do these change the risk of that type of spiral, or there's probably some future use case that will lead to problems here anyways?

I had some other questions as well:

• If an animation controller is passed into Expansible, should it be disposed in Expansible or in the consumer?
• Is it possible to instead create the animation controller in Expansible and pass it to material/cupertino through a callback? and if it is possible would that also cause problems?

I think most complexity is coming from when one wants to sync animation curve from one to another.

If an animation controller is passed into Expansible, should it be disposed in Expansible or in the consumer?

Who creates it will dispose it, so if it is created by expansiontile, the expansiontile should be responsible for dispose it

Is it possible to instead create the animation controller in Expansible and pass it to material/cupertino through a callback? and if it is possible would that also cause problems?

yeah this may be a better way, but probably not directly as a callback.

Is it possible to do it this way? If the expansion tile is not responsible for initiate the animation, we can consider pass the animation as one of builder's parameters. If it is responsible for initiate the animation, i think it can do so by calling ExpandibleController's api

Applied in cf82ae4

IMO no, either this controller be a changenotifier so that people can listen to this property, or we should remove this property getter

In a bigger picture we should remove duplicate API in Expandible and this controller.

1. Either the controller is stateful and changenotifier where it is the single source of truth. We should remove the initialExpand and onExpandChange callback on this widget. If people want to listen to onExpandChange they should just listen to the controller.

2. remove this controller and keeps initialExpand and onExpandChange, and expose an API on ExpandibleState to toggle expand. If developer want to toggle the expand, they need to provide a globalkey to this widget and use it to call the toggle api on the state

if the page storage value is different from the controller, we should call expand/collapse on it to modify it.

The controller defaults to false, so really the only relevant case is if the page storage value is explicitly set to false and the controller value is explicitly set to true. Can that happen? And in that case, I'm wondering if/why the page storage value should take precedence.

this is when you have a pageview with expansiontile and then you switch between different pages. The pagestorage are used for memorizing state when you navigate away. If the controller was not owned by the developer it will recreate with default value every time user navigate back to the page.

This means you can have all sorts of combination possible, and i think letting the pagestorage to take precedence is the right choice

So should we just call controller.expand if initiallyExpanded and controller.collapse otherwise?

yes

So should we just call controller.expand if initiallyExpanded and controller.collapse otherwise?

Will that mean it will initialize animating the expansion, rather than just being open?

We should do it before ExpandableState listen to the ExpansibleController.

Hey Victor, feel free to ignore/submit because I came across this just now. Is there a reason for disposing the controller after super.dispose() is called?

No particular reason, I can change that!

init and dispose should be in reversed order. Otherwise, you may run into weird corner cases

e.g.

init() {
  super.init();
  initA();
  doSomethingToA();
  initB();
}

// the dispose should be the reverse of init
dispose() {
   disposeB();
   unDoSomethingToA();
   disposeA();
   super.dispose()

}

init and dispose should be in reversed order.

I'm a bit confused, what does reversed order mean?

Oh got it!

What happens if the user updates the ExpansionTileController? I think the old controller would need to be disposed/new controller created.

in other place we usually only dispose internal controller in State.dispose even if the a controller has passed in during the life cycle of the state.

This reduce recreate/dispose the controller mutliple time if developer switch back and forth for some reasons. It is also a lot easier to manage and less error prone.

I was trying to do this but the logic got complicated really fast.