QCocoaNSMenuDelegate: Improve key-equivalent logic

By using NSEvent.characters instead of NSEvent.charactersIgnoringModifiers,
we may miss sending ShortcutOverride events.

For example, when the user presses Cmd-Opt-o, characters will be "ø"
(on a US keyboard layout) and therefore we'll be looking for the wrong
key-equivalent among the menu items. We only fall back on the modified
string when the search on the unmodified string fails.

As and addendum, we also skip any submenu when doing the key search.
This is not necessary since each menu delegate will get called eventually.

Change-Id: Id793315293a02c99e99d793ad812cff7b4a47821
Reviewed-by: Frederik Gladhorn <frederik.gladhorn@qt.io>
Reviewed-by: Morten Johan Sørvig <morten.sorvig@qt.io>

2 files changed, 41 insertions, 19 deletions

diff --git a/src/plugins/platforms/cocoa/qcocoansmenu.h b/src/plugins/platforms/cocoa/qcocoansmenu.h
index 89843535e6..8fb0a26f27 100644
--- a/src/plugins/platforms/cocoa/qcocoansmenu.h
+++ b/src/plugins/platforms/cocoa/qcocoansmenu.h
@@ -64,7 +64,9 @@ typedef QPointer<QCocoaMenu> QCocoaMenuPointer;
 
 + (instancetype)sharedMenuDelegate;
 
-- (NSMenuItem *)findItem:(NSMenu *)menu forKey:(NSString *)key forModifiers:(NSUInteger)modifier;
+- (NSMenuItem *)findItemInMenu:(NSMenu *)menu
+                        forKey:(NSString *)key
+                     modifiers:(NSUInteger)modifiers;
 
 - (BOOL)validateMenuItem:(NSMenuItem *)item; // NSMenuValidation
 
diff --git a/src/plugins/platforms/cocoa/qcocoansmenu.mm b/src/plugins/platforms/cocoa/qcocoansmenu.mm
index 2ff207b325..996a4ff194 100644
--- a/src/plugins/platforms/cocoa/qcocoansmenu.mm
+++ b/src/plugins/platforms/cocoa/qcocoansmenu.mm
@@ -197,12 +197,25 @@ static NSString *qt_mac_removePrivateUnicode(NSString* string)
 
     CHECK_MENU_CLASS(menu);
 
-    // Change the private unicode keys to the ones used in setting the "Key Equivalents"
-    NSString *characters = qt_mac_removePrivateUnicode([event characters]);
     // Interested only in Shift, Cmd, Ctrl & Alt Keys, so ignoring masks like, Caps lock, Num Lock ...
-    const NSUInteger mask = NSShiftKeyMask | NSControlKeyMask | NSCommandKeyMask | NSAlternateKeyMask;
-    if (NSMenuItem *menuItem = [self findItem:menu forKey:characters forModifiers:([event modifierFlags] & mask)]) {
-        if (!menuItem.target) {
+    static const NSUInteger mask = NSShiftKeyMask | NSControlKeyMask | NSCommandKeyMask | NSAlternateKeyMask;
+
+    // Change the private unicode keys to the ones used in setting the "Key Equivalents"
+    NSString *characters = qt_mac_removePrivateUnicode(event.charactersIgnoringModifiers);
+    const auto modifiers = event.modifierFlags & mask;
+    NSMenuItem *keyEquivalentItem = [self findItemInMenu:menu
+                                                  forKey:characters
+                                               modifiers:modifiers];
+    if (!keyEquivalentItem) {
+        // Maybe the modified character is what we're looking for after all
+        characters = qt_mac_removePrivateUnicode(event.characters);
+        keyEquivalentItem = [self findItemInMenu:menu
+                                          forKey:characters
+                                       modifiers:modifiers];
+    }
+
+    if (keyEquivalentItem) {
+        if (!keyEquivalentItem.target) {
             // This item was modified by QCocoaMenuBar::redirectKnownMenuItemsToFirstResponder
             // and it looks like we're running a modal session for NSOpenPanel/NSSavePanel.
             // QCocoaFileDialogHelper is actually the only place we use this and we run NSOpenPanel modal
@@ -211,7 +224,7 @@ static NSString *qt_mac_removePrivateUnicode(NSString* string)
             // and do not touch the Qt's focusObject (which is different from some native view
             // having a focus inside NSSave/OpenPanel.
             *target = nil;
-            *action = menuItem.action;
+            *action = keyEquivalentItem.action;
             return YES;
         }
 
@@ -249,25 +262,32 @@ static NSString *qt_mac_removePrivateUnicode(NSString* string)
             }
         }
     }
+
     return NO;
 }
 
-- (NSMenuItem *)findItem:(NSMenu *)menu forKey:(NSString *)key forModifiers:(NSUInteger)modifier
+- (NSMenuItem *)findItemInMenu:(NSMenu *)menu
+                        forKey:(NSString *)key
+                     modifiers:(NSUInteger)modifiers
 {
-    for (NSMenuItem *item in [menu itemArray]) {
-        if (![item isEnabled] || [item isHidden] || [item isSeparatorItem])
+    // Find an item in 'menu' that has the same key equivalent as specified by
+    // 'key' and 'modifiers'. We ignore disabled, hidden and separator items.
+    // In a similar fashion, we don't need to recurse into submenus because their
+    // delegate will have [menuHasKeyEquivalent:...] invoked at some point.
+
+    for (NSMenuItem *item in menu.itemArray) {
+        if (!item.enabled || item.hidden || item.separatorItem)
             continue;
-        if ([item hasSubmenu]) {
-            if (NSMenuItem *nested = [self findItem:[item submenu] forKey:key forModifiers:modifier])
-                return nested;
-        }
 
-        NSString *menuKey = [item keyEquivalent];
-        if (menuKey
-            && NSOrderedSame == [menuKey compare:key]
-            && modifier == [item keyEquivalentModifierMask])
-            return item;
+        if (item.hasSubmenu)
+            continue;
+
+        NSString *menuKey = item.keyEquivalent;
+        if (menuKey && NSOrderedSame == [menuKey compare:key]
+            && modifiers == item.keyEquivalentModifierMask)
+                return item;
     }
+
     return nil;
 }