Added MaterialStatesController, updated InkWell et al.#103167
Added MaterialStatesController, updated InkWell et al.#103167HansMuller merged 11 commits intoflutter:masterfrom
Conversation
b451246 to
0f72a46
Compare
e7bc391 to
14bd49a
Compare
|
Currently waiting for #103548 to land |
|
@willlockwood - I don't think that this PR addresses any of the issues you brought up in #73163 however I thought it would be worth bringing to your attention, given all of the thought you've given to InkWell. |
7ca404c to
0cacdbd
Compare
|
@HansMuller Gotcha, thank you for the heads up! I'll take a look. Probably also a good time for me to actually set some boundaries at my day job and push forward those ink ripple fixes too lol, I'll start getting back in contact on those |
There was a problem hiding this comment.
Love this! Seems like a great way to add this configurability without making things any more confusing. I know you didn't explicitly ask for my review on this, but I read through it and had a handful of minor thoughts pretty much on just typos + how the API could be tweaked to be even more straightforward and easily understood.
Again, thanks for the heads up! And apologies if my comments just added unnecessary pollution to the PR lol
0cacdbd to
25c96f7
Compare
There was a problem hiding this comment.
nit: extra blank line
There was a problem hiding this comment.
nit: most other examples have the main at the top of the file.
There was a problem hiding this comment.
I assume that it's up to the caller to make sure that they don't call update on a MaterialStateController unless they are currently allowed to rebuild this widget (either inside this widget or a parent currently building)?
Also, maybe checking to see if this widget is still mounted might be helpful? Although I guess we're unregistered in dispose, so it probably can't happen.
There was a problem hiding this comment.
Yes, it must be safe to call setState() when the ButtonStyleButton's controller's states are changed.
If setState() is called at the wrong time the error message is pretty self-explanatory
There was a problem hiding this comment.
I don't think you want to call dispose on this unless you allocated it here. Isn't the caller responsible for calling dispose if they supply one?
There's a pattern I use for this in several places:
class FooManager {
@mustCallSuper
void dispose() {}
}
class Foo extends StatefulWidget {
const Foo({this.manager});
final FooManager? manager;
@override
State<Foo> createState() => _FooState();
}
class _FooState extends State<Foo> {
FooManager? _internalManager;
FooManager get manager => widget.manager ?? _internalManager!;
@override
void initState() {
super.initState();
if (widget.manager == null) {
_internalManager = FooManager();
}
}
@override
void dispose() {
_internalManager?.dispose();
super.dispose();
}
@override
void didUpdateWidget(Foo oldWidget) {
super.didUpdateWidget(oldWidget);
if (widget.manager != oldWidget.manager) {
if (widget.manager != null) {
_internalManager?.dispose();
_internalManager = null;
} else {
_internalManager ??= FooManager();
}
}
}
@override
Widget build(BuildContext context) {
return const SizedBox();
}
}And then just call the manager getter in the rest of the State code.
There was a problem hiding this comment.
controller?.dispose(); disposes the button's AnimationController, not its MaterialStatesController. I wasn't disposing the statesController, but I suppose I should.
ea0920c to
d6f8b62
Compare
The new MaterialStatesController class is just a trivial ValueNotifier that manages a set of MaterialStates. InkWell and the ButtonStyle button classes use the statesController they're given to track state changes. If one is not provided then they create it.
A stateful widget that wants to track button or InkWell state changes itself, or add support for additional states, can do so by creating a private MaterialStatesController and passing it along. The example included in this PR demonstrates how support for MaterialState.selected can be added to a TextButton to turn it into a "toggle" button.