QMenuBar: Update title on change

When one of the menubar actions changed, we would omit to
update several properties on the platform menu, most notably
its title.

Manual tested with BigMenuCreator, where the sequence

    menu->addAction(action);    // A-operation
    action->setMenu(submenu);   // S-operation

would result in an "Untitled" menubar item on macOS, and this
regardless of when the submenu is populated.

Change-Id: I43989f36f6bf3f0b7056310ac986c06f8e02f128
Reviewed-by: Dmitry Shachnev <mitya57@gmail.com>
Reviewed-by: Erik Verbruggen <erik.verbruggen@qt.io>
Reviewed-by: Morten Johan Sørvig <morten.sorvig@qt.io>

2 files changed, 34 insertions, 24 deletions

diff --git a/src/widgets/widgets/qmenubar.cpp b/src/widgets/widgets/qmenubar.cpp
index 41b6bf49f8..6dfbb7c8a1 100644
--- a/src/widgets/widgets/qmenubar.cpp
+++ b/src/widgets/widgets/qmenubar.cpp
@@ -1187,7 +1187,7 @@ void QMenuBar::leaveEvent(QEvent *)
         d->setCurrentAction(0);
 }
 
-QPlatformMenu *QMenuBarPrivate::getPlatformMenu(QAction *action)
+QPlatformMenu *QMenuBarPrivate::getPlatformMenu(const QAction *action)
 {
     if (!action || !action->menu())
         return 0;
@@ -1202,6 +1202,29 @@ QPlatformMenu *QMenuBarPrivate::getPlatformMenu(QAction *action)
     return platformMenu;
 }
 
+QPlatformMenu *QMenuBarPrivate::findInsertionPlatformMenu(const QAction *action)
+{
+    Q_Q(QMenuBar);
+    QPlatformMenu *beforeMenu = nullptr;
+    for (int beforeIndex = indexOf(const_cast<QAction *>(action)) + 1;
+         !beforeMenu && (beforeIndex < q->actions().size());
+         ++beforeIndex) {
+        beforeMenu = getPlatformMenu(q->actions().at(beforeIndex));
+    }
+
+    return beforeMenu;
+}
+
+void QMenuBarPrivate::copyActionToPlatformMenu(const QAction *action, QPlatformMenu *menu)
+{
+    const auto tag = reinterpret_cast<quintptr>(action);
+    if (menu->tag() != tag)
+        menu->setTag(tag);
+    menu->setText(action->text());
+    menu->setVisible(action->isVisible());
+    menu->setEnabled(action->isEnabled());
+}
+
 /*!
   \reimp
 */
@@ -1218,16 +1241,9 @@ void QMenuBar::actionEvent(QActionEvent *e)
         if (e->type() == QEvent::ActionAdded) {
             QPlatformMenu *menu = d->getPlatformMenu(e->action());
             if (menu) {
-                QPlatformMenu* beforeMenu = NULL;
-                for (int beforeIndex = d->indexOf(e->action()) + 1;
-                     !beforeMenu && (beforeIndex < actions().size());
-                     ++beforeIndex)
-                {
-                    beforeMenu = d->getPlatformMenu(actions().at(beforeIndex));
-                }
+                d->copyActionToPlatformMenu(e->action(), menu);
 
-                menu->setTag(reinterpret_cast<quintptr>(e->action()));
-                menu->setText(e->action()->text());
+                QPlatformMenu *beforeMenu = d->findInsertionPlatformMenu(e->action());
                 d->platformMenuBar->insertMenu(menu, beforeMenu);
             }
         } else if (e->type() == QEvent::ActionRemoved) {
@@ -1235,7 +1251,7 @@ void QMenuBar::actionEvent(QActionEvent *e)
             if (menu)
                 d->platformMenuBar->removeMenu(menu);
         } else if (e->type() == QEvent::ActionChanged) {
-            QPlatformMenu* cur = d->platformMenuBar->menuForTag(reinterpret_cast<quintptr>(e->action()));
+            QPlatformMenu *cur = d->platformMenuBar->menuForTag(reinterpret_cast<quintptr>(e->action()));
             QPlatformMenu *menu = d->getPlatformMenu(e->action());
 
             // the menu associated with the action can change, need to
@@ -1244,21 +1260,13 @@ void QMenuBar::actionEvent(QActionEvent *e)
                 if (cur)
                     d->platformMenuBar->removeMenu(cur);
                 if (menu) {
-                    menu->setTag(reinterpret_cast<quintptr>(e->action()));
-
-                    QPlatformMenu* beforeMenu = NULL;
-                    for (int beforeIndex = d->indexOf(e->action()) + 1;
-                         !beforeMenu && (beforeIndex < actions().size());
-                         ++beforeIndex)
-                    {
-                        beforeMenu = d->getPlatformMenu(actions().at(beforeIndex));
-                    }
+                    d->copyActionToPlatformMenu(e->action(), menu);
+
+                    QPlatformMenu *beforeMenu = d->findInsertionPlatformMenu(e->action());
                     d->platformMenuBar->insertMenu(menu, beforeMenu);
                 }
             } else if (menu) {
-                menu->setText(e->action()->text());
-                menu->setVisible(e->action()->isVisible());
-                menu->setEnabled(e->action()->isEnabled());
+                d->copyActionToPlatformMenu(e->action(), menu);
                 d->platformMenuBar->syncMenu(menu);
             }
         }
diff --git a/src/widgets/widgets/qmenubar_p.h b/src/widgets/widgets/qmenubar_p.h
index 01d8793a3a..c276a4512d 100644
--- a/src/widgets/widgets/qmenubar_p.h
+++ b/src/widgets/widgets/qmenubar_p.h
@@ -132,7 +132,9 @@ public:
 
     QBasicTimer autoReleaseTimer;
     QPlatformMenuBar *platformMenuBar;
-    QPlatformMenu *getPlatformMenu(QAction *action);
+    QPlatformMenu *getPlatformMenu(const QAction *action);
+    QPlatformMenu *findInsertionPlatformMenu(const QAction *action);
+    void copyActionToPlatformMenu(const QAction *e, QPlatformMenu *menu);
 
     inline int indexOf(QAction *act) const { return q_func()->actions().indexOf(act); }
 };