Cocoa: Improve text heuristic menu item syncing logic

Don't try anything after the original syncing, particularly after
changing the menu item's text. Also, don't try anything if the menu
item cannot be linked up to a menubar (see QTBUG-30756). This latter
point requires extra syncing after adding a menu in the menubar.

Finally, to be able to find the menubar, we need to clean the code for
moc's eyes.

Task-number: QTBUG-30756
Task-number: QTBUG-30812
Change-Id: I88fad663f1c35d03a0cb167d1723d16f590918c0
Reviewed-by: Morten Johan Sørvig <morten.sorvig@digia.com>

6 files changed, 51 insertions, 13 deletions

diff --git a/src/plugins/platforms/cocoa/qcocoamenu.h b/src/plugins/platforms/cocoa/qcocoamenu.h
index 65f31a33c2..9100b9b15f 100644
--- a/src/plugins/platforms/cocoa/qcocoamenu.h
+++ b/src/plugins/platforms/cocoa/qcocoamenu.h
@@ -47,10 +47,6 @@
 #include <qpa/qplatformmenu.h>
 #include "qcocoamenuitem.h"
 
-@class NSMenuItem;
-@class NSMenu;
-@class NSObject;
-
 QT_BEGIN_NAMESPACE
 
 class QCocoaMenu : public QPlatformMenu
@@ -89,6 +85,7 @@ public:
     virtual QPlatformMenuItem *menuItemAt(int position) const;
     virtual QPlatformMenuItem *menuItemForTag(quintptr tag) const;
 
+    QList<QCocoaMenuItem *> items() const;
     QList<QCocoaMenuItem *> merged() const;
 private:
     QCocoaMenuItem *itemOrNull(int index) const;
diff --git a/src/plugins/platforms/cocoa/qcocoamenu.mm b/src/plugins/platforms/cocoa/qcocoamenu.mm
index df338a1109..9020aef600 100644
--- a/src/plugins/platforms/cocoa/qcocoamenu.mm
+++ b/src/plugins/platforms/cocoa/qcocoamenu.mm
@@ -271,8 +271,11 @@ void QCocoaMenu::syncSeparatorsCollapsible(bool enable)
         NSArray *itemArray = [m_nativeMenu itemArray];
         for (unsigned int i = 0; i < [itemArray count]; ++i) {
             NSMenuItem *item = reinterpret_cast<NSMenuItem *>([itemArray objectAtIndex:i]);
-            if ([item isSeparatorItem])
+            if ([item isSeparatorItem]) {
+                QCocoaMenuItem *cocoaItem = reinterpret_cast<QCocoaMenuItem *>([item tag]);
+                cocoaItem->setVisible(!previousIsSeparator);
                 [item setHidden:previousIsSeparator];
+            }
 
             if (![item isHidden]) {
                 previousItem = item;
@@ -281,8 +284,11 @@ void QCocoaMenu::syncSeparatorsCollapsible(bool enable)
         }
 
         // We now need to check the final item since we don't want any separators at the end of the list.
-        if (previousItem && previousIsSeparator)
+        if (previousItem && previousIsSeparator) {
+            QCocoaMenuItem *cocoaItem = reinterpret_cast<QCocoaMenuItem *>([previousItem tag]);
+            cocoaItem->setVisible(false);
             [previousItem setHidden:YES];
+        }
     } else {
         foreach (QCocoaMenuItem *item, m_menuItems) {
             if (!item->isSeparator())
@@ -378,6 +384,11 @@ QPlatformMenuItem *QCocoaMenu::menuItemForTag(quintptr tag) const
     return 0;
 }
 
+QList<QCocoaMenuItem *> QCocoaMenu::items() const
+{
+    return m_menuItems;
+}
+
 QList<QCocoaMenuItem *> QCocoaMenu::merged() const
 {
     QList<QCocoaMenuItem *> result;
diff --git a/src/plugins/platforms/cocoa/qcocoamenubar.h b/src/plugins/platforms/cocoa/qcocoamenubar.h
index 2db2abcaaf..8086676cc5 100644
--- a/src/plugins/platforms/cocoa/qcocoamenubar.h
+++ b/src/plugins/platforms/cocoa/qcocoamenubar.h
@@ -47,14 +47,13 @@
 #include <qpa/qplatformmenu.h>
 #include "qcocoamenu.h"
 
-@class NSMenu;
-
 QT_BEGIN_NAMESPACE
 
 class QCocoaWindow;
 
 class QCocoaMenuBar : public QPlatformMenuBar
 {
+    Q_OBJECT
 public:
     QCocoaMenuBar();
     virtual ~QCocoaMenuBar();
diff --git a/src/plugins/platforms/cocoa/qcocoamenubar.mm b/src/plugins/platforms/cocoa/qcocoamenubar.mm
index b0a53244f4..b112e40549 100644
--- a/src/plugins/platforms/cocoa/qcocoamenubar.mm
+++ b/src/plugins/platforms/cocoa/qcocoamenubar.mm
@@ -110,6 +110,7 @@ void QCocoaMenuBar::insertMenu(QPlatformMenu *platformMenu, QPlatformMenu *befor
     }
 
     platformMenu->setParent(this);
+    syncMenu(platformMenu);
     [m_nativeMenu setSubmenu: menu->nsMenu() forItem: menu->nsMenuItem()];
 }
 
@@ -132,7 +133,9 @@ void QCocoaMenuBar::removeMenu(QPlatformMenu *platformMenu)
 
 void QCocoaMenuBar::syncMenu(QPlatformMenu *menu)
 {
-    Q_UNUSED(menu);
+    QCocoaMenu *cocoaMenu = static_cast<QCocoaMenu *>(menu);
+    Q_FOREACH (QCocoaMenuItem *item, cocoaMenu->items())
+        cocoaMenu->syncMenuItem(item);
 }
 
 void QCocoaMenuBar::handleReparent(QWindow *newParentWindow)
diff --git a/src/plugins/platforms/cocoa/qcocoamenuitem.h b/src/plugins/platforms/cocoa/qcocoamenuitem.h
index 0e6d17343d..1e69ed5a4b 100644
--- a/src/plugins/platforms/cocoa/qcocoamenuitem.h
+++ b/src/plugins/platforms/cocoa/qcocoamenuitem.h
@@ -48,8 +48,16 @@
 
 //#define QT_COCOA_ENABLE_MENU_DEBUG
 
-@class NSMenuItem;
-@class NSMenu;
+#ifdef __OBJC__
+#define QT_FORWARD_DECLARE_OBJC_CLASS(__KLASS__) @class __KLASS__
+#else
+#define QT_FORWARD_DECLARE_OBJC_CLASS(__KLASS__) typedef struct objc_object __KLASS__
+#endif
+
+QT_FORWARD_DECLARE_OBJC_CLASS(NSMenuItem);
+QT_FORWARD_DECLARE_OBJC_CLASS(NSMenu);
+QT_FORWARD_DECLARE_OBJC_CLASS(NSObject);
+
 
 QT_BEGIN_NAMESPACE
 
@@ -96,6 +104,7 @@ private:
 
     NSMenuItem *m_native;
     QString m_text;
+    bool m_textSynced;
     QIcon m_icon;
     QCocoaMenu *m_menu;
     bool m_isVisible;
diff --git a/src/plugins/platforms/cocoa/qcocoamenuitem.mm b/src/plugins/platforms/cocoa/qcocoamenuitem.mm
index 51d9a83345..1255f75eb7 100644
--- a/src/plugins/platforms/cocoa/qcocoamenuitem.mm
+++ b/src/plugins/platforms/cocoa/qcocoamenuitem.mm
@@ -42,6 +42,7 @@
 #include "qcocoamenuitem.h"
 
 #include "qcocoamenu.h"
+#include "qcocoamenubar.h"
 #include "messages.h"
 #include "qcocoahelpers.h"
 #include "qcocoaautoreleasepool.h"
@@ -90,6 +91,7 @@ NSUInteger keySequenceModifierMask(const QKeySequence &accel)
 
 QCocoaMenuItem::QCocoaMenuItem() :
     m_native(NULL),
+    m_textSynced(false),
     m_menu(NULL),
     m_isVisible(true),
     m_enabled(true),
@@ -156,6 +158,8 @@ void QCocoaMenuItem::setFont(const QFont &font)
 
 void QCocoaMenuItem::setRole(MenuRole role)
 {
+    if (role != m_role)
+        m_textSynced = false; // Changing role deserves a second chance.
     m_role = role;
 }
 
@@ -193,7 +197,7 @@ NSMenuItem *QCocoaMenuItem::sync()
         }
     }
 
-    if ((m_role != NoRole) || m_merged) {
+    if ((m_role != NoRole && !m_textSynced) || m_merged) {
         NSMenuItem *mergeItem = nil;
         QT_MANGLE_NAMESPACE(QCocoaMenuLoader) *loader = getMenuLoader();
         switch (m_role) {
@@ -212,7 +216,17 @@ NSMenuItem *QCocoaMenuItem::sync()
         case PreferencesRole:
             mergeItem = [loader preferencesMenuItem];
             break;
-        case TextHeuristicRole:
+        case TextHeuristicRole: {
+            QObject *p = parent();
+            int depth = 1;
+            QCocoaMenuBar *menubar = 0;
+            while (depth < 3 && p && !(menubar = qobject_cast<QCocoaMenuBar *>(p))) {
+                ++depth;
+                p = p->parent();
+            }
+            if (depth == 3 || !menubar)
+                break; // Menu item too deep in the hierarchy, or not connected to any menubar
+
             switch (detectMenuRole(m_text)) {
             case QPlatformMenuItem::AboutRole:
                 if (m_text.indexOf(QRegExp(QString::fromLatin1("qt$"), Qt::CaseInsensitive)) == -1)
@@ -227,15 +241,18 @@ NSMenuItem *QCocoaMenuItem::sync()
                 mergeItem = [loader quitMenuItem];
                 break;
             default:
+                m_textSynced = true;
                 break;
             }
             break;
+        }
 
         default:
             qWarning() << Q_FUNC_INFO << "unsupported role" << (int) m_role;
         }
 
         if (mergeItem) {
+            m_textSynced = true;
             m_merged = true;
             [mergeItem retain];
             [m_native release];
@@ -247,6 +264,8 @@ NSMenuItem *QCocoaMenuItem::sync()
             m_native = nil; // create item below
             m_merged = false;
         }
+    } else {
+        m_textSynced = true; // NoRole, and that was set explicitly. So, nothing to do anymore.
     }
 
     if (!m_native) {