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>

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;

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;

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();

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)

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;

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) {