Cocoa Menus: Refactor QCocoaMenuItem::sync()

And move some logic into detectMenuRole(), where it belongs.

This refactoring will enable fixes for the issues below.

Change-Id: Id03bb5c26d7dd0bb3b94f01e69935e1f3321bb95
Task-number: QTBUG-17291
Task-number: QTBUG-30812
Task-number: QTBUG-38705
Reviewed-by: Morten Johan Sørvig <morten.sorvig@qt.io>

2 files changed, 49 insertions, 50 deletions

diff --git a/src/plugins/platforms/cocoa/messages.cpp b/src/plugins/platforms/cocoa/messages.cpp
index c7e7f82bae..06e3dd454e 100644
--- a/src/plugins/platforms/cocoa/messages.cpp
+++ b/src/plugins/platforms/cocoa/messages.cpp
@@ -39,7 +39,8 @@
 
 #include "messages.h"
 
-#include <QCoreApplication>
+#include <QtCore/qcoreapplication.h>
+#include <QtCore/qregularexpression.h>
 
 // Translatable messages should go into this .cpp file for them to be picked up by lupdate.
 
@@ -77,8 +78,13 @@ QPlatformMenuItem::MenuRole detectMenuRole(const QString &caption)
     QString captionNoAmpersand(caption);
     captionNoAmpersand.remove(QLatin1Char('&'));
     const QString aboutString = QCoreApplication::translate("QCocoaMenuItem", "About");
-    if (captionNoAmpersand.startsWith(aboutString, Qt::CaseInsensitive) || caption.endsWith(aboutString, Qt::CaseInsensitive))
+    if (captionNoAmpersand.startsWith(aboutString, Qt::CaseInsensitive)
+        || captionNoAmpersand.endsWith(aboutString, Qt::CaseInsensitive)) {
+        static const QRegularExpression qtRegExp(QLatin1String("qt$"), QRegularExpression::CaseInsensitiveOption);
+        if (captionNoAmpersand.contains(qtRegExp))
+            return QPlatformMenuItem::AboutQtRole;
         return QPlatformMenuItem::AboutRole;
+    }
     if (captionNoAmpersand.startsWith(QCoreApplication::translate("QCocoaMenuItem", "Config"), Qt::CaseInsensitive)
         || captionNoAmpersand.startsWith(QCoreApplication::translate("QCocoaMenuItem", "Preference"), Qt::CaseInsensitive)
         || captionNoAmpersand.startsWith(QCoreApplication::translate("QCocoaMenuItem", "Options"), Qt::CaseInsensitive)
diff --git a/src/plugins/platforms/cocoa/qcocoamenuitem.mm b/src/plugins/platforms/cocoa/qcocoamenuitem.mm
index 1011d97685..e1572e1800 100644
--- a/src/plugins/platforms/cocoa/qcocoamenuitem.mm
+++ b/src/plugins/platforms/cocoa/qcocoamenuitem.mm
@@ -51,7 +51,6 @@
 #include "qcocoaapplication.h" // for custom application category
 #include "qcocoamenuloader.h"
 #include <QtGui/private/qcoregraphics_p.h>
-#include <QtCore/qregularexpression.h>
 
 #include <QtCore/QDebug>
 
@@ -229,74 +228,68 @@ NSMenuItem *QCocoaMenuItem::sync()
     }
 
     if ((m_role != NoRole && !m_textSynced) || m_merged) {
-        NSMenuItem *mergeItem = nil;
+        QCocoaMenuBar *menubar = nullptr;
+        if (m_role == TextHeuristicRole) {
+            // Recognized menu roles are only found in the first menus below the menubar
+            QObject *p = menuParent();
+            int depth = 1;
+            while (depth < 3 && p && !(menubar = qobject_cast<QCocoaMenuBar *>(p))) {
+                ++depth;
+                QCocoaMenuObject *menuObject = dynamic_cast<QCocoaMenuObject *>(p);
+                Q_ASSERT(menuObject);
+                p = menuObject->menuParent();
+            }
+
+            if (menubar && depth < 3)
+                m_detectedRole = detectMenuRole(m_text);
+            else
+                m_detectedRole = NoRole;
+        }
+
         QCocoaMenuLoader *loader = [QCocoaMenuLoader sharedMenuLoader];
-        switch (m_role) {
-        case ApplicationSpecificRole:
-            mergeItem = [loader appSpecificMenuItem:this];
-            break;
+        NSMenuItem *mergeItem = nil;
+        const auto role = effectiveRole();
+        switch (role) {
         case AboutRole:
             mergeItem = [loader aboutMenuItem];
             break;
         case AboutQtRole:
             mergeItem = [loader aboutQtMenuItem];
             break;
+        case PreferencesRole:
+            mergeItem = [loader preferencesMenuItem];
+            break;
+        case ApplicationSpecificRole:
+            mergeItem = [loader appSpecificMenuItem:this];
+            break;
         case QuitRole:
             mergeItem = [loader quitMenuItem];
             break;
-        case PreferencesRole:
-            mergeItem = [loader preferencesMenuItem];
+        case CutRole:
+        case CopyRole:
+        case PasteRole:
+        case SelectAllRole:
+            if (menubar)
+                mergeItem = menubar->itemForRole(role);
             break;
-        case TextHeuristicRole: {
-            QObject *p = menuParent();
-            int depth = 1;
-            QCocoaMenuBar *menubar = nullptr;
-            while (depth < 3 && p && !(menubar = qobject_cast<QCocoaMenuBar *>(p))) {
-                ++depth;
-                QCocoaMenuObject *menuObject = dynamic_cast<QCocoaMenuObject *>(p);
-                Q_ASSERT(menuObject);
-                p = menuObject->menuParent();
-            }
-            if (depth == 3 || !menubar)
-                break; // Menu item too deep in the hierarchy, or not connected to any menubar
-
-            m_detectedRole = detectMenuRole(m_text);
-            switch (m_detectedRole) {
-            case QPlatformMenuItem::AboutRole:
-                if (m_text.indexOf(QRegularExpression(QString::fromLatin1("qt$"),
-                                                      QRegularExpression::CaseInsensitiveOption)) == -1)
-                    mergeItem = [loader aboutMenuItem];
-                else
-                    mergeItem = [loader aboutQtMenuItem];
-                break;
-            case QPlatformMenuItem::PreferencesRole:
-                mergeItem = [loader preferencesMenuItem];
-                break;
-            case QPlatformMenuItem::QuitRole:
-                mergeItem = [loader quitMenuItem];
-                break;
-            default:
-                if (m_detectedRole >= CutRole && m_detectedRole < RoleCount && menubar)
-                    mergeItem = menubar->itemForRole(m_detectedRole);
-                if (!m_text.isEmpty())
-                    m_textSynced = true;
-                break;
-            }
+        case NoRole:
+            // The heuristic couldn't resolve the menu role
+            m_textSynced = false;
             break;
-        }
-
         default:
-            qWarning() << "Menu item" << m_text << "has unsupported role" << m_role;
+            if (!m_text.isEmpty())
+                m_textSynced = true;
+            break;
         }
 
         if (mergeItem) {
             m_textSynced = true;
             m_merged = true;
             [mergeItem retain];
-            if ([mergeItem isMemberOfClass:[QCocoaNSMenuItem class]])
-                static_cast<QCocoaNSMenuItem *>(mergeItem).platformMenuItem = this;
             [m_native release];
             m_native = mergeItem;
+            if (auto *nativeItem = qt_objc_cast<QCocoaNSMenuItem *>(m_native))
+                nativeItem.platformMenuItem = this;
         } else if (m_merged) {
             // was previously merged, but no longer
             [m_native release];