Menus: Clean popup hide and destroy logic

When a menu popup gets closed it usually needs to be destroyed right away since we don't recycle its contents. There is an exception, however, and it's when he user triggers a menu item. In this case, we need to proceed in three steps. First, we hide the menu popup, then we emit the triggered signal, and when that one returns, the menu contents can be disposed. If we did all in a single step, we may end up with a crash since we don't support deleting a QtQuick item while it's running a signal handler. Delayed deletions don't work either in the case when the triggered handler ends up running the event loop. Task-number: QTBUG-45182 Task-number: QTBUG-47682 Task-number: QTBUG-48382 Change-Id: Ic39717e09f38df602f641250cd81cf4931863db6 Reviewed-by: J-P Nurmi <jpnurmi@theqtcompany.com>
author: Gabriel de Dietrich <gabriel.dedietrich@theqtcompany.com> 2015-09-24 19:56:23 +0200
committer: Gabriel de Dietrich <gabriel.dedietrich@theqtcompany.com> 2015-10-01 09:07:59 +0000
commit: b27a01a86e614207025e569926d0c419857e8965 (patch)
tree: 9d3dc7b339f3bc4ed7d2dcdaca17f091376b5aca /src
parent: 79092d8ab8f5c02c5d880d33f9869c034f0f933d (diff)
8 files changed, 91 insertions, 40 deletions
diff --git a/src/controls/ComboBox.qml b/src/controls/ComboBox.qml
index d09407721..a2eff0b6d 100644
--- a/src/controls/ComboBox.qml
+++ b/src/controls/ComboBox.qml
@@ -626,8 +626,7 @@ Control {
 
         function toggleShow() {
             if (popup.__popupVisible) {
-                popup.__dismissMenu()
-                popup.__destroyAllMenuPopups()
+                popup.__dismissAndDestroy()
             } else {
                 if (items[__selectedIndex])
                     items[__selectedIndex].checked = true
diff --git a/src/controls/MenuBar.qml b/src/controls/MenuBar.qml
index eddd48cd8..444185ccb 100644
--- a/src/controls/MenuBar.qml
+++ b/src/controls/MenuBar.qml
@@ -155,10 +155,8 @@ MenuBarPrivate {
 
                 if (oldIndex !== -1) {
                     var menu = root.menus[oldIndex]
-                    if (menu.__popupVisible) {
-                        menu.__dismissMenu()
-                        menu.__destroyAllMenuPopups()
-                    }
+                    if (menu.__popupVisible)
+                        menu.__dismissAndDestroy()
                 }
 
                 if (openedMenuIndex !== -1) {
diff --git a/src/controls/Private/EditMenu_base.qml b/src/controls/Private/EditMenu_base.qml
index e44ff5992..15fe8d692 100644
--- a/src/controls/Private/EditMenu_base.qml
+++ b/src/controls/Private/EditMenu_base.qml
@@ -161,8 +161,7 @@ Item {
 
             if (control.menu) {
                 var menu = getMenuInstance();
-                menu.__dismissMenu();
-                menu.__destroyAllMenuPopups();
+                menu.__dismissAndDestroy();
                 var menuPos = mapToItem(null, mouse.x, mouse.y)
                 menu.__popup(Qt.rect(menuPos.x, menuPos.y, 0, 0), -1, MenuPrivate.EditMenu);
             }
diff --git a/src/controls/Private/MenuContentItem.qml b/src/controls/Private/MenuContentItem.qml
index 07edea2f2..a0b346c4c 100644
--- a/src/controls/Private/MenuContentItem.qml
+++ b/src/controls/Private/MenuContentItem.qml
@@ -80,16 +80,16 @@ Loader {
         function triggerCurrent() {
             var item = content.menuItemAt(__menu.__currentIndex)
             if (item)
-                content.triggered(item)
+                triggerAndDismiss(item)
         }
 
         function triggerAndDismiss(item) {
-            if (item && item.styleData.type !== MenuItemType.Separator) {
-                __menu.__dismissMenu()
-                if (item.styleData.type !== MenuItemType.Menu)
-                    item.__menuItem.trigger()
-                __menu.__destroyAllMenuPopups()
-            }
+            if (!item)
+                return;
+            if (item.styleData.type === MenuItemType.Separator)
+                __menu.__dismissAndDestroy()
+            else if (item.styleData.type === MenuItemType.Item)
+                item.__menuItem.trigger()
         }
     }
 
@@ -112,7 +112,7 @@ Loader {
         }
     }
 
-    Keys.onEscapePressed: __menu.__dismissMenu()
+    Keys.onEscapePressed: __menu.__dismissAndDestroy()
 
     Keys.onDownPressed: {
         if (__menu.__currentIndex < 0)
@@ -132,10 +132,8 @@ Loader {
     }
 
     Keys.onLeftPressed: {
-        if ((event.accepted = __menu.__parentMenu.hasOwnProperty("title"))) {
-            __menu.__closeMenu()
-            __menu.__destroyMenuPopup()
-        }
+        if ((event.accepted = __menu.__parentMenu.hasOwnProperty("title")))
+            __menu.__closeAndDestroy()
     }
 
     Keys.onRightPressed: {
@@ -246,10 +244,8 @@ Loader {
                 id: closeMenuTimer
                 interval: 1
                 onTriggered: {
-                    if (__menu.__currentIndex !== __menuItemIndex) {
-                        __menuItem.__closeMenu()
-                        __menuItem.__destroyMenuPopup()
-                    }
+                    if (__menu.__currentIndex !== __menuItemIndex)
+                        __menuItem.__closeAndDestroy()
                 }
             }
 
diff --git a/src/controls/qquickmenu.cpp b/src/controls/qquickmenu.cpp
index 617a3d03b..beabe65b5 100644
--- a/src/controls/qquickmenu.cpp
+++ b/src/controls/qquickmenu.cpp
@@ -265,14 +265,15 @@ QQuickMenu::QQuickMenu(QObject *parent)
       m_popupVisible(false),
       m_containersCount(0),
       m_xOffset(0),
-      m_yOffset(0)
+      m_yOffset(0),
+      m_triggerCount(0)
 {
     connect(this, SIGNAL(__textChanged()), this, SIGNAL(titleChanged()));
 
     m_platformMenu = QGuiApplicationPrivate::platformTheme()->createPlatformMenu();
     if (m_platformMenu) {
         connect(m_platformMenu, SIGNAL(aboutToShow()), this, SIGNAL(aboutToShow()));
-        connect(m_platformMenu, SIGNAL(aboutToHide()), this, SLOT(__closeMenu()));
+        connect(m_platformMenu, SIGNAL(aboutToHide()), this, SLOT(hideMenu()));
         if (platformItem())
             platformItem()->setMenu(m_platformMenu);
     }
@@ -415,7 +416,7 @@ void QQuickMenu::popup()
 void QQuickMenu::__popup(const QRectF &targetRect, int atItemIndex, MenuType menuType)
 {
     if (popupVisible()) {
-        __closeMenu();
+        hideMenu();
         // Mac and Windows would normally move the menu under the cursor, so we should not
         // return here. However, very often we want to re-contextualize the menu, and this
         // has to be done at the application level.
@@ -488,14 +489,30 @@ QRect QQuickMenu::popupGeometry() const
     return m_popupWindow->geometry();
 }
 
-void QQuickMenu::__closeMenu()
+void QQuickMenu::prepareItemTrigger(QQuickMenuItem *)
+{
+    m_triggerCount++;
+    __dismissMenu();
+}
+
+void QQuickMenu::concludeItemTrigger(QQuickMenuItem *)
+{
+    if (--m_triggerCount == 0)
+        destroyAllMenuPopups();
+}
+
+/*!
+ * \internal
+ * Close this menu's popup window. Emits aboutToHide and sets __popupVisible to false.
+ */
+void QQuickMenu::hideMenu()
 {
     if (m_popupVisible) {
         emit aboutToHide();
         setPopupVisible(false);
     }
-    if (m_popupWindow)
-        m_popupWindow->setVisible(false);
+    if (m_popupWindow && m_popupWindow->isVisible())
+        m_popupWindow->hide();
     m_parentWindow = 0;
 }
 
@@ -512,6 +529,12 @@ QQuickMenuPopupWindow *QQuickMenu::topMenuPopup() const
     return 0;
 }
 
+/*!
+ * \internal
+ * Dismiss all the menus this menu is attached to, bottom-up.
+ * In QQuickPopupWindow, before closing, dismissPopup() emits popupDismissed()
+ * which is connected to dismissPopup() on any child popup.
+ */
 void QQuickMenu::__dismissMenu()
 {
     if (m_platformMenu) {
@@ -521,14 +544,22 @@ void QQuickMenu::__dismissMenu()
     }
 }
 
+/*!
+ * \internal
+ * Called when the popup window visible property changes.
+ */
 void QQuickMenu::windowVisibleChanged(bool v)
 {
     if (!v) {
-        if (m_popupWindow && qobject_cast<QQuickMenuPopupWindow *>(m_popupWindow->transientParent())) {
-            m_popupWindow->transientParent()->setMouseGrabEnabled(true);
-            m_popupWindow->transientParent()->setKeyboardGrabEnabled(true);
+        if (m_popupWindow) {
+            QQuickMenuPopupWindow *parentMenuPopup = qobject_cast<QQuickMenuPopupWindow *>(m_popupWindow->transientParent());
+            if (parentMenuPopup) {
+                parentMenuPopup->setMouseGrabEnabled(true);
+                parentMenuPopup->setKeyboardGrabEnabled(true);
+            }
         }
-        __closeMenu();
+        if (m_popupVisible)
+            __closeAndDestroy();
     }
 }
 
@@ -538,18 +569,34 @@ void QQuickMenu::clearPopupWindow()
     emit __menuPopupDestroyed();
 }
 
-void QQuickMenu::__destroyMenuPopup()
+void QQuickMenu::destroyMenuPopup()
 {
+    if (m_triggerCount > 0)
+        return;
     if (m_popupWindow)
         m_popupWindow->setToBeDeletedLater();
 }
 
-void QQuickMenu::__destroyAllMenuPopups() {
+void QQuickMenu::destroyAllMenuPopups() {
+    if (m_triggerCount > 0)
+        return;
     QQuickMenuPopupWindow *popup = topMenuPopup();
     if (popup)
         popup->setToBeDeletedLater();
 }
 
+void QQuickMenu::__closeAndDestroy()
+{
+    hideMenu();
+    destroyMenuPopup();
+}
+
+void QQuickMenu::__dismissAndDestroy()
+{
+    __dismissMenu();
+    destroyAllMenuPopups();
+}
+
 void QQuickMenu::itemIndexToListIndex(int itemIndex, int *listIndex, int *containerIndex) const
 {
     *listIndex = -1;
diff --git a/src/controls/qquickmenu_p.h b/src/controls/qquickmenu_p.h
index a16e720a8..a626179ab 100644
--- a/src/controls/qquickmenu_p.h
+++ b/src/controls/qquickmenu_p.h
@@ -89,10 +89,10 @@ public:
     Q_INVOKABLE void __popup(const QRectF &targetRect, int atItemIndex = -1, MenuType menuType = DefaultMenu);
 
 public Q_SLOTS:
-    void __closeMenu();
     void __dismissMenu();
-    void __destroyMenuPopup();
-    void __destroyAllMenuPopups();
+
+    void __closeAndDestroy();
+    void __dismissAndDestroy();
 
 Q_SIGNALS:
     void itemsChanged();
@@ -142,11 +142,17 @@ public:
 
     QRect popupGeometry() const;
 
+    void prepareItemTrigger(QQuickMenuItem *);
+    void concludeItemTrigger(QQuickMenuItem *);
+    void destroyMenuPopup();
+    void destroyAllMenuPopups();
+
 protected Q_SLOTS:
     void updateSelectedIndex();
 
     void setMenuContentItem(QQuickItem *);
     void setPopupVisible(bool);
+    void hideMenu();
     void clearPopupWindow();
 
     void updateText();
@@ -189,6 +195,7 @@ private:
     qreal m_xOffset;
     qreal m_yOffset;
     QFont m_font;
+    int m_triggerCount;
 };
 
 QT_END_NAMESPACE
diff --git a/src/controls/qquickmenuitem.cpp b/src/controls/qquickmenuitem.cpp
index 8093c5fe5..be15a435d 100644
--- a/src/controls/qquickmenuitem.cpp
+++ b/src/controls/qquickmenuitem.cpp
@@ -674,7 +674,12 @@ void QQuickMenuItem::setEnabled(bool enabled)
 
 void QQuickMenuItem::trigger()
 {
+    QPointer<QQuickMenu> menu(parentMenu());
+    if (menu)
+        menu->prepareItemTrigger(this);
     action()->trigger(this);
+    if (menu)
+        menu->concludeItemTrigger(this);
 }
 
 QT_END_NAMESPACE
diff --git a/src/controls/qquickpopupwindow.cpp b/src/controls/qquickpopupwindow.cpp
index 08407e2d8..a8f451673 100644
--- a/src/controls/qquickpopupwindow.cpp
+++ b/src/controls/qquickpopupwindow.cpp
@@ -138,7 +138,7 @@ void QQuickPopupWindow::dismissPopup()
 {
     m_dismissed = true;
     emit popupDismissed();
-    close();
+    hide();
 }
 
 void QQuickPopupWindow::mouseMoveEvent(QMouseEvent *e)
author	Gabriel de Dietrich <gabriel.dedietrich@theqtcompany.com>	2015-09-24 19:56:23 +0200
committer	Gabriel de Dietrich <gabriel.dedietrich@theqtcompany.com>	2015-10-01 09:07:59 +0000
commit	b27a01a86e614207025e569926d0c419857e8965 (patch)
tree	9d3dc7b339f3bc4ed7d2dcdaca17f091376b5aca /src
parent	79092d8ab8f5c02c5d880d33f9869c034f0f933d (diff)