QCocoaMenu: Force NSMenuValidation when syncing items

When a menu item's enabled state changes after
-[QCocoaMenuDelegate menuWillOpen:] is invoked, i.e.,
during or after QMenu::aboutToShow() is emitted, that
state change may not be taken into account. This is
because the automatic menu validation, upon which Qt
relies, is not made aware of any such change.

By calling -[NSMenu update] when syncing the QPA menu
item, we induce Cocoa to invoke -[QCocoaMenuDelegate
validateMenuItem:] and ensure that previously synced
items, whose state may have changed, will be properly
updated. This, however, has a small side effect, namely
that menu-holding items will also go through the automatic
menu enabling path and may appear disabled since, until
now, they were not properly configured. In order to solve
this, we set the action on those items as well, and make
sure that both of QCocoaMenuDelegate's relevant methods,
validateMenuItem: and itemFired:, properly process
menu-holding items.

Menurama manual test updated accordingly.

Change-Id: I62f955538b8be09b8494ea0ce87fca7910148d38
Task-number: QTBUG-56850
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

src/plugins/platforms/cocoa/qcocoamenu.mm

@@ -147,6 +147,10 @@ QT_NAMESPACE_ALIAS_OBJC_CLASS(QCocoaMenuDelegate);
 - (void) itemFired:(NSMenuItem*) item
 {
     QCocoaMenuItem *cocoaItem = reinterpret_cast<QCocoaMenuItem *>([item tag]);
+    // Menu-holding items also get a target to play nicely
+    // with NSMenuValidation but should not trigger.
+    if (cocoaItem->menu())
+        return;
     QScopedScopeLevelCounter scopeLevelCounter(QGuiApplicationPrivate::instance()->threadData);
     QGuiApplicationPrivate::modifier_buttons = [QNSView convertKeyModifiers:[NSEvent modifierFlags]];
     static QMetaMethod activatedSignal = QMetaMethod::fromSignal(&QCocoaMenuItem::activated);
@@ -156,7 +160,8 @@ QT_NAMESPACE_ALIAS_OBJC_CLASS(QCocoaMenuDelegate);
 - (BOOL)validateMenuItem:(NSMenuItem*)menuItem
 {
     QCocoaMenuItem *cocoaItem = reinterpret_cast<QCocoaMenuItem *>(menuItem.tag);
-    if (!cocoaItem)
+    // Menu-holding items are always enabled, as it's conventional in Cocoa
+    if (!cocoaItem || cocoaItem->menu())
         return YES;
 
     return cocoaItem->isEnabled();
@@ -327,9 +332,9 @@ void QCocoaMenu::insertMenuItem(QPlatformMenuItem *menuItem, QPlatformMenuItem *
 void QCocoaMenu::insertNative(QCocoaMenuItem *item, QCocoaMenuItem *beforeItem)
 {
     item->nsItem().target = m_nativeMenu.delegate;
-    if (!item->menu())
+    item->nsItem().action = @selector(itemFired:);
-        [item->nsItem() setAction:@selector(itemFired:)];
+    // Someone's adding new items after aboutToShow() was emitted
-    else if (isOpen() && item->nsItem()) // Someone's adding new items after aboutToShow() was emitted
+    if (isOpen() && item->menu() && item->nsItem())
         item->menu()->setAttachedItem(item->nsItem());
 
     item->setParentEnabled(isEnabled());
@@ -425,6 +430,10 @@ void QCocoaMenu::syncMenuItem(QPlatformMenuItem *menuItem)
 
         QCocoaMenuItem* beforeItem = itemOrNull(m_menuItems.indexOf(cocoaItem) + 1);
         insertNative(cocoaItem, beforeItem);
+    } else {
+        // Force NSMenuValidation to kick in. This is needed e.g.
+        // when an item's enabled state changes after menuWillOpen:
+        [m_nativeMenu update];
     }
 }
 

tests/manual/cocoa/menurama/mainwindow.cpp

@@ -56,7 +56,13 @@ MainWindow::MainWindow(QWidget *parent) :
     });
 
     connect(ui->menuDynamic_Stuff, &QMenu::aboutToShow, [=] {
-        menuApp->addDynMenu(QLatin1String("Added After aboutToShow()"), ui->menuDynamic_Stuff);
+        menuApp->addDynMenu(QLatin1String("Menu Added After aboutToShow()"), ui->menuDynamic_Stuff);
+
+        const QLatin1String itemTitle = QLatin1String("Disabled Item Added After aboutToShow()");
+        if (QAction *a = menuApp->findAction(itemTitle, ui->menuDynamic_Stuff))
+            ui->menuDynamic_Stuff->removeAction(a);
+        QAction *a = ui->menuDynamic_Stuff->addAction(itemTitle);
+        a->setEnabled(false);
     });
 
     connect(ui->pushButton, &QPushButton::clicked, [=] {

tests/manual/cocoa/menurama/mainwindow.ui

@@ -6,23 +6,40 @@
    <rect>
     <x>0</x>
     <y>0</y>
-    <width>566</width>
+    <width>429</width>
-    <height>300</height>
+    <height>251</height>
    </rect>
   </property>
   <property name="windowTitle">
    <string>MainWindow</string>
   </property>
   <widget class="QWidget" name="centralWidget">
-   <widget class="QCheckBox" name="checkBox">
+   <layout class="QVBoxLayout" name="verticalLayout_2">
-    <property name="geometry">
+    <item>
-     <rect>
+     <layout class="QVBoxLayout" name="verticalLayout">
-      <x>10</x>
+      <property name="spacing">
-      <y>40</y>
+       <number>24</number>
-      <width>151</width>
-      <height>20</height>
-     </rect>
       </property>
+      <item>
+       <widget class="QLabel" name="label">
+        <property name="text">
+         <string>The &quot;Help&quot; menu should NOT be visible.
+
+Click on &quot;Dynamic Stuff&quot; then move left and right to other menus. Disabled items should remain that way.</string>
+        </property>
+        <property name="scaledContents">
+         <bool>false</bool>
+        </property>
+        <property name="alignment">
+         <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignTop</set>
+        </property>
+        <property name="wordWrap">
+         <bool>true</bool>
+        </property>
+       </widget>
+      </item>
+      <item>
+       <widget class="QCheckBox" name="checkBox">
         <property name="text">
          <string>Enable &quot;Stuff&quot; Menu</string>
         </property>
@@ -30,39 +47,30 @@
          <bool>true</bool>
         </property>
        </widget>
-   <widget class="QLabel" name="label">
+      </item>
-    <property name="geometry">
+      <item>
-     <rect>
-      <x>10</x>
-      <y>10</y>
-      <width>321</width>
-      <height>16</height>
-     </rect>
-    </property>
-    <property name="text">
-     <string>The &quot;Help&quot; menu should NOT be visible.</string>
-    </property>
-   </widget>
        <widget class="QPushButton" name="pushButton">
-    <property name="geometry">
+        <property name="sizePolicy">
-     <rect>
+         <sizepolicy hsizetype="Maximum" vsizetype="Fixed">
-      <x>10</x>
+          <horstretch>0</horstretch>
-      <y>80</y>
+          <verstretch>0</verstretch>
-      <width>211</width>
+         </sizepolicy>
-      <height>32</height>
-     </rect>
         </property>
         <property name="text">
          <string>Populate Dynamic Submenu</string>
         </property>
        </widget>
+      </item>
+     </layout>
+    </item>
+   </layout>
   </widget>
   <widget class="QMenuBar" name="menuBar">
    <property name="geometry">
     <rect>
      <x>0</x>
      <y>0</y>
-     <width>566</width>
+     <width>429</width>
      <height>22</height>
     </rect>
    </property>

tests/manual/cocoa/menurama/menuramaapplication.cpp

@@ -69,13 +69,19 @@ void MenuramaApplication::populateMenu(QMenu *menu, bool clear)
 
 void MenuramaApplication::addDynMenu(QLatin1String title, QMenu *parentMenu)
 {
-    foreach (QAction *a, parentMenu->actions())
+    if (QAction *a = findAction(title, parentMenu))
-        if (a->text() == title) {
         parentMenu->removeAction(a);
-            break;
-        }
 
     QMenu *subMenu = new QMenu(title, parentMenu);
     populateMenu(subMenu, false /*clear*/);
     parentMenu->addMenu(subMenu);
 }
+
+QAction *MenuramaApplication::findAction(QLatin1String title, QMenu *parentMenu)
+{
+    foreach (QAction *a, parentMenu->actions())
+        if (a->text() == title)
+            return a;
+
+    return Q_NULLPTR;
+}

tests/manual/cocoa/menurama/menuramaapplication.h

@@ -50,6 +50,7 @@ class MenuramaApplication : public QApplication
 public:
     MenuramaApplication(int argc, char **argv);
     void addDynMenu(QLatin1String title, QMenu *parentMenu);
+    QAction *findAction(QLatin1String title, QMenu *parentMenu);
 
 public slots:
     void populateMenu(QMenu *menu, bool clear);