search

flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.61k stars 27.34k forks source link

[Proposal] make MenuController a ValueListenable<bool> #156570

Open navaronbracke opened 1 week ago

navaronbracke commented 1 week ago

Use case

Users that want to observe the isOpen state of a MenuController, for example when providing an animated icon for the child of the menu anchor.

Proposal

The MenuController should be a ValueListenable<bool> or similar so that consumers get notified when isOpen changes.

Note for triage: I think I remember there being discussion about this feature in the past, but I'm not sure if someone filed a bug for it. I can't find the relevant issue at least.

nate-thegrate commented 1 week ago

I believe a lot of the conversation from https://github.com/flutter/flutter/pull/153056 is relevant here—this would make MenuController objects a bit "heavier", and we'd also suddenly need to start calling dispose().

It's technically not a breaking change by itself, since looking at the ChangeNotifier.dispose() docs, disposing isn't necessary if you never add any listeners.

But inevitably we'll start taking advantage of the API within the menu anchor library, which means that projects prior to this change will start having memory leaks unless they update their code.


My personal opinion: maybe we could see how the code looks with a ValueListenable controller and compare it to the easiest workaround; if the workaround is significantly more ugly then this is probably worth pursuing.

navaronbracke commented 1 week ago

I do agree with your point. What if we introduce a subclass ListenableMenuController, that uses a ValueNotifier internally and implements the ValueListenable interface?

That way, people that don't need it can keep using the regular controller, while if you do need it, you can use the other one.

And if we intend to deprecate the ListenableMenuController, we could perhaps use a typedef? (like how we did for MaterialStateProperty)

nate-thegrate commented 1 week ago

And if we intend to deprecate the ListenableMenuController

Apologies; I'm having a bit of trouble following this.

My best guess is: after we introduce ListenableMenuController, we eventually move the listenable logic into MenuController and deprecate ListenableMenuController with a typedef. Is that right?

navaronbracke commented 1 week ago

Yes that's exactly what I'm thinking of.

nate-thegrate commented 1 week ago

Ah, okay. I will respond in the PR so we aren't splitting one conversation into 2 threads 😄