-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Make CupertinoRadio's mouseCursor a WidgetStateProperty
#151910
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
Make CupertinoRadio's mouseCursor a WidgetStateProperty
#151910
Conversation
CupertinoRadio mouseCursor a WidgetStatePropertyCupertinoRadio's mouseCursor a WidgetStateProperty
justinmc
left a comment
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.
Some quick questions, otherwise looks good.
| selected: accessibilitySelected, | ||
| child: buildToggleable( | ||
| mouseCursor: effectiveMouseCursor, | ||
| mouseCursor: widget.mouseCursor ?? _defaultMouseCursor, |
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.
Can you set this as the actual default value for CupertinoRadio.mosueCursor or is it more complicated than that?
| mouseCursor: WidgetStateProperty.all(widget.mouseCursor | ||
| ?? (kIsWeb && widget.onChanged != null | ||
| ? SystemMouseCursors.click : SystemMouseCursors.basic)), |
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.
Is this duplicating CupertinoRadio._defaultMouseCursor? Could you make that public and use it here?
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 added a static method defaultMouseCursor in CupertinoRadio that I just call here to get the default mouse cursor.
|
Just sharing here, we synced offline that the original change was cut into the beta release that is about to go out. This should be CP'd into beta after it lands. 👍 |
justinmc
left a comment
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.
Some quick questions.
| /// If null, then [SystemMouseCursors.basic] is used when this radio button is | ||
| /// disabled. When this radio button is enabled, [SystemMouseCursors.click] is | ||
| /// used on Web, and [SystemMouseCursors.basic] is used on other platforms. |
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: You might just say defaults to [defaultMouseCursor] and let people look that up themselves. Or maybe it's more useful for users to see it written out like this, up to you.
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.
True. Then I could move these docs to defaultMouseCursor.
| /// `WidgetStateProperty` which is used in APIs that need to accept | ||
| /// either a [MouseCursor] or a [WidgetStateProperty<MouseCursor>]. | ||
| final MouseCursor? mouseCursor; | ||
| /// either a [MouseCursor] or a [WidgetStateProperty]. |
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.
Why did you remove the type annotation on WidgetStateProperty?
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 think @Piinks mentioned previously that it doesn't work with dartdoc? I might be wrong though.
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.
Ah makes sense, that's probably right 👍
|
|
||
| bool get _selected => value == groupValue; | ||
|
|
||
| /// The default mouse cursor of a [CupertinoRadio]. |
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: "mouse cursor" => "[mouseCursor]"
| /// either a [MouseCursor] or a [WidgetStateProperty<MouseCursor>]. | ||
| final MouseCursor? mouseCursor; | ||
| /// either a [MouseCursor] or a [WidgetStateProperty]. | ||
| final WidgetStateProperty<MouseCursor>? mouseCursor; |
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.
What happens if you explicitly pass null here?
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 think the null coalescing operator should make it resolve to WidgetStateProperty.all(CupertinoRadio.defaultMouseCursor(widget.onChanged)).
| bool get _selected => value == groupValue; | ||
|
|
||
| /// The default mouse cursor of a [CupertinoRadio]. | ||
| static MouseCursor defaultMouseCursor(Function? onChanged) { |
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.
Could this be an actual WidgetStateProperty<MouseCursor> instead of a function that returns a MouseCursor?
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.
The mouseCursor property of Material Radio is nullable, so to pass it to WidgetStateProperty<MouseCursor>? which is the type of CupertinoRadio's mouseCursor it would need to have resolved to a non-null MouseCursor which we can then wrap with WidgetStateProperty.all. If the defaultMouseCursor function returns a WidgetStateProperty<MouseCursor>, there would be an extra step to resolve that into a plain non-null MouseCursor which would then be wrapped with WidgetStateProperty.all.
There might be a better way to do all this though that I am missing.
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.
If I'm understanding correctly, try this below and then see my other comment about Material Radio using this.
static WidgetStateProperty<MouseCursor> defaultMouseCursor(Function? onChanged) {
final MouseCursor mouseCursor = (onChanged != null && kIsWeb)
? SystemMouseCursors.click
: SystemMouseCursors.basic;
return WidgetStateProperty.all<MouseCursor>(mouseCursor);
}| selected: accessibilitySelected, | ||
| child: buildToggleable( | ||
| mouseCursor: effectiveMouseCursor, | ||
| mouseCursor: widget.mouseCursor ?? WidgetStateProperty.all(CupertinoRadio.defaultMouseCursor(widget.onChanged)), |
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.
Instead of doing this here, can you set the default in the parameter list of the constructor?
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.
defaultMouseCursor takes widget.onChanged as an argument. Since widget.onChanged is non-const, defaultMouseCursor can't be set as the default mouseCursor in the parameter list.
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.
Ah it's a const constructor. Sounds good as-is then.
| mouseCursor: WidgetStateProperty.all(widget.mouseCursor | ||
| ?? CupertinoRadio.defaultMouseCursor(widget.onChanged)), |
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.
Same here, can you do this in the constructor parameter list?
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.
Some ideas about how to make defaultMouseCursor be a WidgetStateProperty, but LGTM 👍 either way.
| selected: accessibilitySelected, | ||
| child: buildToggleable( | ||
| mouseCursor: effectiveMouseCursor, | ||
| mouseCursor: widget.mouseCursor ?? WidgetStateProperty.all(CupertinoRadio.defaultMouseCursor(widget.onChanged)), |
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.
Ah it's a const constructor. Sounds good as-is then.
| mouseCursor: WidgetStateProperty.resolveWith((Set<MaterialState> states) { | ||
| return MaterialStateProperty.resolveAs<MouseCursor?>(widget.mouseCursor, states) | ||
| ?? CupertinoRadio.defaultMouseCursor(widget.onChanged); | ||
| }), |
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.
If CupertinoRadio.defaultMouseCursor returns a WidgetStateProperty then could you do this instead?
mouseCursor: widget.mouseCursor == null
? CupertinoRadio.defaultMouseCursor(widget.onChanged)
: WidgetStateProperty.all<MouseCursor>(widget.mouseCursor!),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.
This works, I forgot we could just use ! to keep the analyzer quiet. Thanks for the insight!
| bool get _selected => value == groupValue; | ||
|
|
||
| /// The default mouse cursor of a [CupertinoRadio]. | ||
| static MouseCursor defaultMouseCursor(Function? onChanged) { |
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.
If I'm understanding correctly, try this below and then see my other comment about Material Radio using this.
static WidgetStateProperty<MouseCursor> defaultMouseCursor(Function? onChanged) {
final MouseCursor mouseCursor = (onChanged != null && kIsWeb)
? SystemMouseCursors.click
: SystemMouseCursors.basic;
return WidgetStateProperty.all<MouseCursor>(mouseCursor);
}|
Failed to create CP due to merge conflicts. |
…r#151910) flutter#149681 introduced `mouseCursor `to `CupertinoRadio` as a `MouseCursor` instead of a `WidgetStateProperty` to match Material Radio's `mouseCursor` property for `.adaptive`. This PR changes `mouseCursor` to be of type `WidgetStateProperty<MouseCursor>` as per review comments in flutter#151788 (comment). PR bringing `mouseCursor` into `CupertinoRadio`: flutter#149681. Part of flutter#58192
…r#151910) flutter#149681 introduced `mouseCursor `to `CupertinoRadio` as a `MouseCursor` instead of a `WidgetStateProperty` to match Material Radio's `mouseCursor` property for `.adaptive`. This PR changes `mouseCursor` to be of type `WidgetStateProperty<MouseCursor>` as per review comments in flutter#151788 (comment). PR bringing `mouseCursor` into `CupertinoRadio`: flutter#149681. Part of flutter#58192
|
@Piinks should this PR be reverted until final discussions on |
|
Let's revert this, and leave the mouseCursor property that made into beta as is. 👍 |
|
Time to revert pull request flutter/flutter/151910 has elapsed. |
|
Reason for revert: Needs further discussion on the pros and cons of adding new properties as |
|
Time to revert pull request flutter/flutter/151910 has elapsed. |
…r#151910) flutter#149681 introduced `mouseCursor `to `CupertinoRadio` as a `MouseCursor` instead of a `WidgetStateProperty` to match Material Radio's `mouseCursor` property for `.adaptive`. This PR changes `mouseCursor` to be of type `WidgetStateProperty<MouseCursor>` as per review comments in flutter#151788 (comment). PR bringing `mouseCursor` into `CupertinoRadio`: flutter#149681. Part of flutter#58192
flutter#152254) Reverts flutter#151910, awaiting further discussion on `WidgetStateProperty`.
…r#151910) flutter#149681 introduced `mouseCursor `to `CupertinoRadio` as a `MouseCursor` instead of a `WidgetStateProperty` to match Material Radio's `mouseCursor` property for `.adaptive`. This PR changes `mouseCursor` to be of type `WidgetStateProperty<MouseCursor>` as per review comments in flutter#151788 (comment). PR bringing `mouseCursor` into `CupertinoRadio`: flutter#149681. Part of flutter#58192
flutter#152254) Reverts flutter#151910, awaiting further discussion on `WidgetStateProperty`.
#149681 introduced
mouseCursortoCupertinoRadioas aMouseCursorinstead of aWidgetStatePropertyto match Material Radio'smouseCursorproperty for.adaptive.This PR changes
mouseCursorto be of typeWidgetStateProperty<MouseCursor>as per review comments in #151788 (comment).PR bringing
mouseCursorintoCupertinoRadio: #149681.Part of #58192
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.