From 8557ffa024f4f31f58d32dad585598982af9ed18 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 6 Mar 2023 22:38:59 -0500 Subject: [PATCH] Fix PlatformMenuItems with onSelectedIntent are never enabled (#121885) Fix PlatformMenuItems with onSelectedIntent are never enabled --- .../lib/src/widgets/platform_menu_bar.dart | 8 +- .../test/widgets/platform_menu_bar_test.dart | 284 ++++++++++-------- 2 files changed, 170 insertions(+), 122 deletions(-) diff --git a/packages/flutter/lib/src/widgets/platform_menu_bar.dart b/packages/flutter/lib/src/widgets/platform_menu_bar.dart index c4840176b7..30e3066f47 100644 --- a/packages/flutter/lib/src/widgets/platform_menu_bar.dart +++ b/packages/flutter/lib/src/widgets/platform_menu_bar.dart @@ -738,7 +738,8 @@ class PlatformMenuItem with Diagnosticable { /// An optional callback that is called when this [PlatformMenuItem] is /// selected. /// - /// If unset, this menu item will be disabled. + /// At most one of [onSelected] and [onSelectedIntent] may be set. If neither + /// field is set, this menu item will be disabled. final VoidCallback? onSelected; /// Returns a callback, if any, to be invoked if the platform menu receives a @@ -760,7 +761,8 @@ class PlatformMenuItem with Diagnosticable { /// An optional intent that is invoked when this [PlatformMenuItem] is /// selected. /// - /// If unset, this menu item will be disabled. + /// At most one of [onSelected] and [onSelectedIntent] may be set. If neither + /// field is set, this menu item will be disabled. final Intent? onSelectedIntent; /// Returns all descendant [PlatformMenuItem]s of this item. @@ -805,7 +807,7 @@ class PlatformMenuItem with Diagnosticable { return { _kIdKey: getId(item), _kLabelKey: item.label, - _kEnabledKey: item.onSelected != null, + _kEnabledKey: item.onSelected != null || item.onSelectedIntent != null, if (shortcut != null)...shortcut.serializeForMenu().toChannelRepresentation(), }; } diff --git a/packages/flutter/test/widgets/platform_menu_bar_test.dart b/packages/flutter/test/widgets/platform_menu_bar_test.dart index 6626c8b1a7..8828115a91 100644 --- a/packages/flutter/test/widgets/platform_menu_bar_test.dart +++ b/packages/flutter/test/widgets/platform_menu_bar_test.dart @@ -15,12 +15,12 @@ void main() { late FakeMenuChannel fakeMenuChannel; late PlatformMenuDelegate originalDelegate; late DefaultPlatformMenuDelegate delegate; - final List activated = []; + final List selected = []; final List opened = []; final List closed = []; - void onActivate(String item) { - activated.add(item); + void onSelected(String item) { + selected.add(item); } void onOpen(String item) { @@ -36,7 +36,7 @@ void main() { delegate = DefaultPlatformMenuDelegate(channel: fakeMenuChannel); originalDelegate = WidgetsBinding.instance.platformMenuDelegate; WidgetsBinding.instance.platformMenuDelegate = delegate; - activated.clear(); + selected.clear(); opened.clear(); closed.clear(); }); @@ -46,117 +46,69 @@ void main() { }); group('PlatformMenuBar', () { - testWidgets('basic menu structure is transmitted to platform', (WidgetTester tester) async { - await tester.pumpWidget( - MaterialApp( - home: Material( - child: PlatformMenuBar( - menus: createTestMenus( - onActivate: onActivate, - onOpen: onOpen, - onClose: onClose, - shortcuts: { - subSubMenu10[0]: const SingleActivator(LogicalKeyboardKey.keyA, control: true), - subSubMenu10[1]: const SingleActivator(LogicalKeyboardKey.keyB, shift: true), - subSubMenu10[2]: const SingleActivator(LogicalKeyboardKey.keyC, alt: true), - subSubMenu10[3]: const SingleActivator(LogicalKeyboardKey.keyD, meta: true), - }, + group('basic menu structure is transmitted to platform', () { + testWidgets('using onSelected', (WidgetTester tester) async { + await tester.pumpWidget( + MaterialApp( + home: Material( + child: PlatformMenuBar( + menus: createTestMenus( + onSelected: onSelected, + onOpen: onOpen, + onClose: onClose, + shortcuts: { + subSubMenu10[0]: const SingleActivator(LogicalKeyboardKey.keyA, control: true), + subSubMenu10[1]: const SingleActivator(LogicalKeyboardKey.keyB, shift: true), + subSubMenu10[2]: const SingleActivator(LogicalKeyboardKey.keyC, alt: true), + subSubMenu10[3]: const SingleActivator(LogicalKeyboardKey.keyD, meta: true), + }, + ), + child: const Center(child: Text('Body')), ), - child: const Center(child: Text('Body')), ), ), - ), - ); + ); - expect(fakeMenuChannel.outgoingCalls.last.method, equals('Menu.setMenus')); - expect( - fakeMenuChannel.outgoingCalls.last.arguments, - equals({ - '0': >[ - { - 'id': 2, - 'label': 'Menu 0', - 'enabled': true, - 'children': >[ - { - 'id': 1, - 'label': 'Sub Menu 00', - 'enabled': true, - }, - ], - }, - { - 'id': 18, - 'label': 'Menu 1', - 'enabled': true, - 'children': >[ - { - 'id': 4, - 'label': 'Sub Menu 10', - 'enabled': true, - }, - {'id': 5, 'isDivider': true}, - { - 'id': 16, - 'label': 'Sub Menu 11', - 'enabled': true, - 'children': >[ - { - 'id': 7, - 'label': 'Sub Sub Menu 110', - 'enabled': true, - 'shortcutTrigger': 97, - 'shortcutModifiers': 8, - }, - {'id': 8, 'isDivider': true}, - { - 'id': 10, - 'label': 'Sub Sub Menu 111', - 'enabled': true, - 'shortcutTrigger': 98, - 'shortcutModifiers': 2, - }, - {'id': 11, 'isDivider': true}, - { - 'id': 12, - 'label': 'Sub Sub Menu 112', - 'enabled': true, - 'shortcutTrigger': 99, - 'shortcutModifiers': 4, - }, - {'id': 13, 'isDivider': true}, - { - 'id': 14, - 'label': 'Sub Sub Menu 113', - 'enabled': true, - 'shortcutTrigger': 100, - 'shortcutModifiers': 1, - }, - ], - }, - { - 'id': 17, - 'label': 'Sub Menu 12', - 'enabled': true, - }, - ], - }, - { - 'id': 20, - 'label': 'Menu 2', - 'enabled': true, - 'children': >[ - { - 'id': 19, - 'label': 'Sub Menu 20', - 'enabled': false, - }, - ], - }, - {'id': 21, 'label': 'Menu 3', 'enabled': false, 'children': >[]}, - ], - }), - ); + expect( + fakeMenuChannel.outgoingCalls.last.method, + equals('Menu.setMenus'), + ); + expect( + fakeMenuChannel.outgoingCalls.last.arguments, + equals(expectedStructure), + ); + }); + testWidgets('using onSelectedIntent', (WidgetTester tester) async { + await tester.pumpWidget( + MaterialApp( + home: Material( + child: PlatformMenuBar( + menus: createTestMenus( + onSelectedIntent: const DoNothingIntent(), + onOpen: onOpen, + onClose: onClose, + shortcuts: { + subSubMenu10[0]: const SingleActivator(LogicalKeyboardKey.keyA, control: true), + subSubMenu10[1]: const SingleActivator(LogicalKeyboardKey.keyB, shift: true), + subSubMenu10[2]: const SingleActivator(LogicalKeyboardKey.keyC, alt: true), + subSubMenu10[3]: const SingleActivator(LogicalKeyboardKey.keyD, meta: true), + }, + ), + child: const Center(child: Text('Body')), + ), + ), + ), + ); + + expect( + fakeMenuChannel.outgoingCalls.last.method, + equals('Menu.setMenus'), + ); + expect( + fakeMenuChannel.outgoingCalls.last.arguments, + equals(expectedStructure), + ); + }); }); testWidgets('asserts when more than one has locked the delegate', (WidgetTester tester) async { await tester.pumpWidget( @@ -287,7 +239,8 @@ const List subMenu2 = [ ]; List createTestMenus({ - void Function(String)? onActivate, + void Function(String)? onSelected, + Intent? onSelectedIntent, void Function(String)? onOpen, void Function(String)? onClose, Map shortcuts = const {}, @@ -301,7 +254,8 @@ List createTestMenus({ menus: [ PlatformMenuItem( label: subMenu0[0], - onSelected: onActivate != null ? () => onActivate(subMenu0[0]) : null, + onSelected: onSelected != null ? () => onSelected(subMenu0[0]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subMenu0[0]], ), ], @@ -315,7 +269,8 @@ List createTestMenus({ members: [ PlatformMenuItem( label: subMenu1[0], - onSelected: onActivate != null ? () => onActivate(subMenu1[0]) : null, + onSelected: onSelected != null ? () => onSelected(subMenu0[0]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subMenu1[0]], ), ], @@ -329,7 +284,8 @@ List createTestMenus({ members: [ PlatformMenuItem( label: subSubMenu10[0], - onSelected: onActivate != null ? () => onActivate(subSubMenu10[0]) : null, + onSelected: onSelected != null ? () => onSelected(subSubMenu10[0]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subSubMenu10[0]], ), ], @@ -338,21 +294,24 @@ List createTestMenus({ members: [ PlatformMenuItem( label: subSubMenu10[1], - onSelected: onActivate != null ? () => onActivate(subSubMenu10[1]) : null, + onSelected: onSelected != null ? () => onSelected(subSubMenu10[1]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subSubMenu10[1]], ), ], ), PlatformMenuItem( label: subSubMenu10[2], - onSelected: onActivate != null ? () => onActivate(subSubMenu10[2]) : null, + onSelected: onSelected != null ? () => onSelected(subSubMenu10[2]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subSubMenu10[2]], ), PlatformMenuItemGroup( members: [ PlatformMenuItem( label: subSubMenu10[3], - onSelected: onActivate != null ? () => onActivate(subSubMenu10[3]) : null, + onSelected: onSelected != null ? () => onSelected(subSubMenu10[3]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subSubMenu10[3]], ), ], @@ -361,7 +320,8 @@ List createTestMenus({ ), PlatformMenuItem( label: subMenu1[2], - onSelected: onActivate != null ? () => onActivate(subMenu1[2]) : null, + onSelected: onSelected != null ? () => onSelected(subMenu1[2]) : null, + onSelectedIntent: onSelectedIntent, shortcut: shortcuts[subMenu1[2]], ), ], @@ -389,6 +349,92 @@ List createTestMenus({ return result; } +const Map expectedStructure = { + '0': >[ + { + 'id': 2, + 'label': 'Menu 0', + 'enabled': true, + 'children': >[ + { + 'id': 1, + 'label': 'Sub Menu 00', + 'enabled': true, + }, + ], + }, + { + 'id': 18, + 'label': 'Menu 1', + 'enabled': true, + 'children': >[ + { + 'id': 4, + 'label': 'Sub Menu 10', + 'enabled': true, + }, + {'id': 5, 'isDivider': true}, + { + 'id': 16, + 'label': 'Sub Menu 11', + 'enabled': true, + 'children': >[ + { + 'id': 7, + 'label': 'Sub Sub Menu 110', + 'enabled': true, + 'shortcutTrigger': 97, + 'shortcutModifiers': 8, + }, + {'id': 8, 'isDivider': true}, + { + 'id': 10, + 'label': 'Sub Sub Menu 111', + 'enabled': true, + 'shortcutTrigger': 98, + 'shortcutModifiers': 2, + }, + {'id': 11, 'isDivider': true}, + { + 'id': 12, + 'label': 'Sub Sub Menu 112', + 'enabled': true, + 'shortcutTrigger': 99, + 'shortcutModifiers': 4, + }, + {'id': 13, 'isDivider': true}, + { + 'id': 14, + 'label': 'Sub Sub Menu 113', + 'enabled': true, + 'shortcutTrigger': 100, + 'shortcutModifiers': 1, + }, + ], + }, + { + 'id': 17, + 'label': 'Sub Menu 12', + 'enabled': true, + }, + ], + }, + { + 'id': 20, + 'label': 'Menu 2', + 'enabled': true, + 'children': >[ + { + 'id': 19, + 'label': 'Sub Menu 20', + 'enabled': false, + }, + ], + }, + {'id': 21, 'label': 'Menu 3', 'enabled': false, 'children': >[]}, + ], +}; + class FakeMenuChannel implements MethodChannel { FakeMenuChannel(this.outgoing);