-
Notifications
You must be signed in to change notification settings - Fork 27.3k
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
Make CupertinoRadio
's mouseCursor
a WidgetStateProperty
#151910
Conversation
CupertinoRadio
mouseCursor
a WidgetStateProperty
CupertinoRadio
's mouseCursor
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.
Some quick questions, otherwise looks good.
@@ -297,7 294,7 @@ class _CupertinoRadioState<T> extends State<CupertinoRadio<T>> with TickerProvid | |||
checked: widget._selected, | |||
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. 👍 |
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
.
/// | ||
/// See also: | ||
/// | ||
/// * [WidgetStateMouseCursor], a [MouseCursor] that implements | ||
/// `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 👍
@@ -210,6 208,13 @@ class CupertinoRadio<T> extends StatefulWidget { | |||
|
|||
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))
.
@@ -210,6 208,13 @@ class CupertinoRadio<T> extends StatefulWidget { | |||
|
|||
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);
}
@@ -297,7 293,7 @@ class _CupertinoRadioState<T> extends State<CupertinoRadio<T>> with TickerProvid | |||
checked: widget._selected, | |||
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.
@@ -297,7 293,7 @@ class _CupertinoRadioState<T> extends State<CupertinoRadio<T>> with TickerProvid | |||
checked: widget._selected, | |||
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!
@@ -210,6 208,13 @@ class CupertinoRadio<T> extends StatefulWidget { | |||
|
|||
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
mouseCursor
toCupertinoRadio
as aMouseCursor
instead of aWidgetStateProperty
to match Material Radio'smouseCursor
property for.adaptive
.This PR changes
mouseCursor
to be of typeWidgetStateProperty<MouseCursor>
as per review comments in #151788 (comment).PR bringing
mouseCursor
intoCupertinoRadio
: #149681.Part of #58192
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.