Refactor more logic from radio and cupertino radio #167764
Conversation
github-actions
bot
added
framework
| /// | ||
| /// This getter is used when the build method is called. One is expected | ||
| /// to use the input `state` to update the painter before returning. | ||
| final RadioPainterGetter painterGetter; |
There was a problem hiding this comment.
This API is this way because the UI requires the widget states in ToggeableStateMixin, such as when the state is hovered or selected.. etc.
An alternative is to make this a widget builder, but that will require changing the buildToggeable API.
There was a problem hiding this comment.
What do you think of the name RadioPainterBuilder and painterBuilder instead?
There was a problem hiding this comment.
I felt that builder implies return new object every time. The problem with this api is that CustomPainter is usually a changenotifier, so it has lifecycle. returning a new painter every time is wasteful and can easily forget to dispose.
Getter can be a permanent object that is why i go with this name that hopefully can convey more information.
though I think both are bad in this case.
An other alternative is to make Radio and CupertinoRadio extends RadioBase and RadioBaseState, that way RadioBaseState can leave an abstract painter getter, but I usually prefer composition vs inheritance for widgets
There was a problem hiding this comment.
An other alternative is to make Radio and CupertinoRadio extends RadioBase
Being able to type check for a RadioBase could be useful for a lot of use cases for developers, especially package authors. I had assumed that would be needed for making the radio group widget work for both Material and Cupertino radios, but it looks like that might not be the case.
There was a problem hiding this comment.
right now both Radio and CupertinoRadio build a RadioBase with different config, and RadioGroup interacts with RadioBase.
I am going with the composition approach
There was a problem hiding this comment.
IMO the ideal state here is that this is a widget builder, in which case users can return a CustomPaint if they want to. But I understand that could be a breaking change.
See my comment below in this file about the overlapping responsibilities of ToggleableStateMixin and RadioBase, which I think is related. Let me know if I'm thinking of all this correctly.
| /// and rebuild the radio button with a new [groupValue] to update the visual | ||
| /// appearance of the radio button. | ||
| /// | ||
| /// The all arguments except `key` are required: |
There was a problem hiding this comment.
Consider cutting this comment, I think this is pretty self-evident by reading the parameters :)
| /// Signature for [RadioBase.painterGetter] | ||
| /// | ||
| /// The getter can use `state` to configure the painter before returning. | ||
| typedef RadioPainterGetter = CustomPainter Function(ToggleableStateMixin state); |
There was a problem hiding this comment.
Why make it take a ToggleableStateMixin as an argument instead of just a set of WidgetState?
There was a problem hiding this comment.
It needs additional state like position or downposition and others. we could make into a sets of parameters. maybe that is fine. let me think a bit.
There was a problem hiding this comment.
I decided to go with pass in ToggleableStateMixin mainly because the API and documentation around ToggleablePainter was heavily relies on the ToggleableStateMixin, so it seemed more nature to build a ToggleablePainter with a ToggleableStateMixin directly in the builder method.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Some bigger possible changes that I commented about below:
- Move widget-related responsibilities from ToggleableStateMixin to RadioBase.
- Make painterGetter a widget builder.
I realize that might not be practical but I wonder what you think about those ideas, to help us figure out how to find a non-breaking middleground if needed.
Overall I'm excited about this PR. This is a great case for moving core logic into the widgets library and sharing it in Material and Cupertino.
@victorsanni when you're back maybe you want to take a look too. I think this looks kind of similar to your work on Switch (or something like that?) where also the main difference between the Cupertino and Material versions was a painter.
| /// | ||
| /// This getter is used when the build method is called. One is expected | ||
| /// to use the input `state` to update the painter before returning. | ||
| final RadioPainterGetter painterGetter; |
There was a problem hiding this comment.
IMO the ideal state here is that this is a widget builder, in which case users can return a CustomPaint if they want to. But I understand that could be a breaking change.
See my comment below in this file about the overlapping responsibilities of ToggleableStateMixin and RadioBase, which I think is related. Let me know if I'm thinking of all this correctly.
| autofocus: widget.autofocus, | ||
| mouseCursor: widget.mouseCursor, | ||
| size: widget.size, | ||
| painter: widget.painterGetter(this), |
There was a problem hiding this comment.
So RadioBase is totally unopinionated about visuals. I like it 👍
chunhtai
force-pushed
the
refactor-radio
branch
from
0087d12 to
79cccab
Compare
|
|
||
| @override | ||
| bool get tristate => widget.toggleable; | ||
| return RawRadio<T>( |
There was a problem hiding this comment.
I just think RawRadio is a cool name 🤘
| /// the visual. | ||
| /// | ||
| /// {@macro flutter.widgets.ToggleableStateMixin.buildToggleableWithChild} | ||
| typedef RadioBuilder = Widget Function(ToggleableStateMixin state); |
There was a problem hiding this comment.
I think this should also take a BuildContext as the first parameter, right?
There was a problem hiding this comment.
oh yeah, I totally missed this, thanks for catching this
| /// This method must be called from the [build] method of the [State] class | ||
| /// that uses this mixin. The returned [Widget] must be returned from the | ||
| /// build method - potentially after wrapping it in other widgets. | ||
| Widget buildToggleableWithChild({ |
There was a problem hiding this comment.
Nice way of avoiding a breaking change here. Should buildToggleable be deprecated, or no need?
| toggleable: widget.toggleable, | ||
| focusNode: _effectiveFocusNode, | ||
| autofocus: widget.autofocus, | ||
| builder: (ToggleableStateMixin state) { |
There was a problem hiding this comment.
I like how this fits all together with a plain widget builder here like this, thanks for trying it out 👍
| autofocus: false, | ||
| builder: (ToggleableStateMixin state) { | ||
| actualState = state; | ||
| return CustomPaint(size: const Size(40, 40), painter: TestPainter()); |
There was a problem hiding this comment.
Nit: I think it would be fine to return SizedBox.shrink or Placeholder here if you want to save a few lines.
There was a problem hiding this comment.
It has to have some size in order to respond to the gesture. I think sizedBox only works in certain constaint, e.g. stack with positioned widget. using a CustomPaint seems to be an easier way to have a fixed size.
There was a problem hiding this comment.
Ah ok makes sense then. CustomPaint is also a more realistic test.
| await tester.tap(find.byType(RawRadio<int>)); | ||
| await tester.pump(); | ||
|
|
||
| expect(actualValue, 0); |
There was a problem hiding this comment.
Can you expect actualState.value here too? That way it confirms that builder was called again correctly.
There was a problem hiding this comment.
Nit: You could also consider testing ToggleableStateMixin.buildToggleableWithChild as well if it would be useful. I guess ToggleableStateMixin isn't currently very well tested anyway though.
|
Sorry I meant to type a message before submitting my review, but I think that my comment #167764 (comment) is a blocker if I'm thinking of that right. Otherwise everything is a nit and LGTM 👍 |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 . This is a great refactor.
| import 'toggleable.dart'; | ||
| import 'widget_state.dart'; | ||
|
|
||
| /// Signature for [RawRadio.builder] |
There was a problem hiding this comment.
Nit: Missing a period here.
chunhtai
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/167764, because - The status or check suite Mac framework_tests_impeller has failed. Please fix the issues identified (or deflake) before re-applying this label. |
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> This is in prepare for flutter#167363 The goal for this refactoring is to make sure the new RadioGroup widget can be used for both CupertinoRadio and Radio, and thus the widget has to be in widget layer to avoid duplicating code. Currently CupertinoRadio and Radio shares ToggleableStateMixin in widget layer. but the mixin itself doesn't contains enough data to implement radioGroup. Therefore, I moved more share logic from CupertinoRadio and Radio to widget layer and created a RadioBase widget. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
4 participants
This is in prepare for #167363
The goal for this refactoring is to make sure the new RadioGroup widget can be used for both CupertinoRadio and Radio, and thus the widget has to be in widget layer to avoid duplicating code.
Currently CupertinoRadio and Radio shares ToggleableStateMixin in widget layer. but the mixin itself doesn't contains enough data to implement radioGroup. Therefore, I moved more share logic from CupertinoRadio and Radio to widget layer and created a RadioBase widget.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.