165369 - support other widget states for CupertinoButton#166088
165369 - support other widget states for CupertinoButton#166088auto-submit[bot] merged 7 commits intoflutter:masterfrom srivats22:sv/165369
CupertinoButton#166088Conversation
github-actions
bot
added
framework
dkwingsmt
changed the title
CupertinoButton
| final Set<WidgetState> states = <WidgetState>{if (!enabled) WidgetState.disabled}; | ||
| final Set<WidgetState> states = <WidgetState>{ | ||
| if (!enabled) WidgetState.disabled, | ||
| if (widget.onPressed != null || widget.onLongPress != null) WidgetState.pressed, |
There was a problem hiding this comment.
Are you sure about this? If I understand correctly these are the callbacks, so you're evaluating whether the user wants to listen to the gestures, while the state is about whether the button is pressed.
There was a problem hiding this comment.
I wanted a clarification on that actually... so for the pressed state how should I determine it? Was confused on that...
There was a problem hiding this comment.
I think _tapInProgress is a perfect indicator. Although you might want to wrap every change of _tapInProgress in a setState because they currently aren't and you need setState to reevaluate the cursor.
There was a problem hiding this comment.
I see _tapInProgress was recently introduced... let me sync my local with upstream and use that to determine if the state is pressed or not. That would work right?
Or if I need to wrap everything in setState would that be a bigger change?
There was a problem hiding this comment.
No, just wrap every _tapInProgress = xxx in a setState.
|
Hi, @srivats22 , have you been able to make progress on it? Is there any blocker or anything I can help with? |
Hi @dkwingsmt, |
|
Have one question: await tester.pumpWidget(buildButton(enabled: true, cursor: const _ButtonMouseCursor()));
await tester.tap(find.byType(CupertinoButton));
if(tapInProgress){
// await gesture.moveTo(tester.getCenter(find.byType(CupertinoButton)));
await tester.pump();
expect(
RendererBinding.instance.mouseTracker.debugDeviceActiveCursor(1),
SystemMouseCursors.grab,
);
}class _ButtonMouseCursor extends WidgetStateMouseCursor {
const _ButtonMouseCursor();
@override
MouseCursor resolve(Set<WidgetState> states) {
return const WidgetStateProperty<MouseCursor>.fromMap(<WidgetStatesConstraint, MouseCursor>{
WidgetState.disabled: SystemMouseCursors.forbidden,
WidgetState.pressed: SystemMouseCursors.grab,
WidgetState.focused: SystemMouseCursors.copy,
WidgetState.any: SystemMouseCursors.basic,
}).resolve(states);
}
@override
String get debugDescription => '_ButtonMouseCursor()';
} |
|
Was able to resolve the pressed state test have updated everything now |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
Generally good except for a few minor issues.
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
LGTM once @dkwingsmt's comments are addressed.
|
What seems to have caused the Windows test to fail? I went through the logs and wasn't able to find anything related to cuperino_button.test unless I overlooked something |
|
Seems like timeout with flake. I've rerun. |
dkwingsmt
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/166088, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 2 more review(s) in order to merge this PR.
|
|
Looks like auto submit failed cause of no approvals... |
4 participants
This PR is a continuation of #164196. With this PR other widget states such as pressed, and focused have been added to cupertino button. This will resolve the following issue: #165369
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.