-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Added MaterialStatesController, updated InkWell et al. #103167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added MaterialStatesController, updated InkWell et al. #103167
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: most other examples have the main at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…)" (#105138) (#105352) This reverts commit 180566f. Co-authored-by: Hans Muller <[email protected]>
…er#103167)" (flutter#105138) This reverts commit 180566f.
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.