Skip to content

Conversation

@HansMuller
Copy link
Contributor

@HansMuller HansMuller commented May 6, 2022

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.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 6, 2022
@HansMuller HansMuller force-pushed the material_states_controller branch from b451246 to 0f72a46 Compare May 10, 2022 01:48
@HansMuller HansMuller requested a review from keyonghan as a code owner May 10, 2022 01:48
@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation c: contributor-productivity Team-specific productivity, code health, technical debt. labels May 10, 2022
@HansMuller HansMuller force-pushed the material_states_controller branch from e7bc391 to 14bd49a Compare May 11, 2022 00:02
@HansMuller
Copy link
Contributor Author

Currently waiting for #103548 to land

@HansMuller
Copy link
Contributor Author

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

@HansMuller HansMuller force-pushed the material_states_controller branch 2 times, most recently from 7ca404c to 0cacdbd Compare May 13, 2022 23:04
@willlockwood
Copy link
Contributor

@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

@HansMuller HansMuller requested review from gspencergoog and removed request for keyonghan May 18, 2022 15:59
Copy link
Contributor

@willlockwood willlockwood left a 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

@HansMuller HansMuller force-pushed the material_states_controller branch from 0cacdbd to 25c96f7 Compare May 18, 2022 19:08
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra blank line

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@HansMuller HansMuller force-pushed the material_states_controller branch from ea0920c to d6f8b62 Compare May 24, 2022 00:11
godofredoc added a commit that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants