Fix PlatformMenuItems with onSelectedIntent are never enabled#121885
Fix PlatformMenuItems with onSelectedIntent are never enabled#121885auto-submit[bot] merged 6 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
the
framework
|
/cc @gspencergoog |
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
|
|
||
| List<PlatformMenuItem> createTestMenus({ | ||
| void Function(String)? onActivate, | ||
| Intent Function(String)? onActivateIntent, |
There was a problem hiding this comment.
This can just pass a nullable Intent.
| child: PlatformMenuBar( | ||
| menus: createTestMenus( | ||
| onActivate: onActivate, | ||
| onActivateIntent: (String item) => TestPlatformMenuBarIntent(item), |
There was a problem hiding this comment.
If you change the argument type in createTestMenus, this can just be:
| onActivateIntent: (String item) => TestPlatformMenuBarIntent(item), | |
| onActivateIntent: TestPlatformMenuBarIntent(subMenu1[0]), |
| PlatformMenuItem( | ||
| label: subMenu1[0], | ||
| onSelected: onActivate != null ? () => onActivate(subMenu1[0]) : null, | ||
| onSelectedIntent: onActivateIntent?.call(subMenu1[0]), |
There was a problem hiding this comment.
I'm a little surprised this doesn't cause the existing test to fail, but maybe it ends up producing the same thing? I'm surprised, because this change prevents the test from ever testing the onActivate case, since it's never used.
Anyhow, I think there need to be two tests: one that tests onSelected, and one that tests onSelectedIntent. In this test menu creation function, you can just put back the line you deleted, and send in the nullable intent that came into the function.
While you're at it, can you rename the "onActivate*" arguments to this function to "onSelected*", since it should have been called that anyhow (it got missed in a name change).
There was a problem hiding this comment.
I'm a little surprised this doesn't cause the existing test to fail, but maybe it ends up producing the same thing?
My understanding is yes, it just produces the same result. Looking at the test, it only has two calls to expect:
- Expect that
Menu.setMenuswas called - Expect that the serialized version of the menu structure matches a hardcoded expected value
The serialized version of the menu items doesn't contain any information about whether a callback or intent was provided, only whether the item is enabled or disabled, so specifying either one satisfies the test.
I'm surprised, because this change prevents the test from ever testing the onActivate case, since it's never used.
As far as I can tell, there are currently no tests that actually test activating menu items. There is code earlier in the test suite that records which items have their callback invoked, but none of the tests ever actually select a menu item or expect that any items were selected. All that onActivate recording is just dead code right now.
Anyhow, I think there need to be two tests: one that tests onSelected, and one that tests onSelectedIntent. In this test menu creation function, you can just put back the line you deleted, and send in the nullable intent that came into the function.
While you're at it, can you rename the "onActivate*" arguments to this function to "onSelected*", since it should have been called that anyhow (it got missed in a name change).
SGTM and SGTM
There was a problem hiding this comment.
Thanks for pointing that out. I'll have to add some more tests, I can tell.
| child: PlatformMenuBar( | ||
| menus: createTestMenus( | ||
| onActivate: onActivate, | ||
| onActivateIntent: (String item) => TestPlatformMenuBarIntent(item), |
There was a problem hiding this comment.
This should be split into two tests, one that tests onActivate, and one that tests onActivateIntent, since they can't both be specified at the same time.
…ng custom intent type from tests
|
Looks like there is an analyzer failure. |
|
Fixed, it should pass analysis now. Sorry about that. |
| })); | ||
| 'shortcutCharacter': '?', | ||
| 'shortcutModifiers': 0, | ||
| })); |
There was a problem hiding this comment.
It's hard to tell on GitHub, but are these indentations intentional? I thought they looked correct beforehand.
There was a problem hiding this comment.
No, I think VSCode's auto format went rogue. I'll see if I can configure it to match the previous formatting.
|
Consider updating the docs for
I'd be fine with doing this in a subsequent pull request. |
|
I'd be fine adding the docs change to this PR if you'd prefer. I was thinking it would be nice to add something to the effect of |
|
@loic-sharma: thoughts on the following for the new docstring? I figure it might be easier to agree on wording in Github comments to cut down on unnecessary CI runs. |
|
@jmatth That looks perfect! |
|
@jmatth Let me know when you're done, and I'll submit it. (seems like you're done, but I didn't want to submit it if you weren't). |
|
@gspencergoog yeah, I'm done. I think I've addressed everyone's comments. |
gspencergoog
added
the
autosubmit
5 participants
Fixes #121805.
The fix is pretty simple: the
PlatformMenuItemserialization logic only considered whetheronSelectedwas non-null when determining whether an item should be enabled. With this change it now checks whetheronSelectedoronSelectedIntentare non-null.I'm less confident about my updates to the tests for this change. The existing tests have code in place to record menu item activations, but then that data isn't actually read anywhere. Based on that observation I didn't implement recording intents sent by activated menu items, since doing so would seemingly go against the style guide's advice to
Avoid implementing features you don’t need.Based on
git blame, CC @gspencergoog