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
PlatformMenuBar changes to bring it into line with upcoming MenuBar implementation #104565
PlatformMenuBar changes to bring it into line with upcoming MenuBar implementation #104565
Conversation
d5d7078
to
840addf
Compare
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.
LGTM
@@ -383,7 +459,12 @@ class DefaultPlatformMenuDelegate extends PlatformMenuDelegate { | |||
} | |||
final MenuItem item = _idMap[id]!; | |||
if (call.method == _kMenuSelectedCallbackMethod) { | |||
assert(item.onSelected == null || item.onSelectedIntent == null, |
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 we assert this earlier closer to the creation point of the MenuItem? Currently, you'd only see the assert if you actually selected the item in the menu
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.
Oh, I see below that this is also asserted earlier. Never mind this comment then.
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.
Yes, I did both because they're in separate classes. It's probably overkill.
… MenuBar implementation (flutter/flutter#104565)
…enuBar implementation (flutter/flutter#104565)
…mplementation (flutter#104565) When I was doing the MenuBar implementation, I made some changes to the PlatformMenuBar to allow it to understand shortcuts a little more, and to deprecate the body parameter rename it to child to match most other widgets. These are those changes, separated out because they are separable, and I'm trying to make the MenuBar PR smaller.
… MenuBar implementation (flutter/flutter#104565)
…enuBar implementation (flutter/flutter#104565)
Description
When I was doing the
MenuBar
implementation, I made some changes to thePlatformMenuBar
to allow it to understand shortcuts a little more, and to deprecate thebody
parameter rename it tochild
to match most other widgets.These are those changes, separated out because they are separable, and I'm trying to make the
MenuBar
PR smaller.