From de9145dcd904068256e28e130fdfda2e8014efe8 Mon Sep 17 00:00:00 2001 From: Gabriel de Dietrich Date: Tue, 24 Mar 2015 20:56:48 +0100 Subject: Menu: Separate dismiss and destroy actions This avoids issues when the action is triggered from a Menu and the slot ends up processing pending events, effectively deleting an object while one of its QML signal handlers is being executed. The reason being that we used to call deleteLater() on the menu popup window while still in the mouse event handler. Now, we do the same thing in three separate steps. 1. Close/dismiss the menu popups, 2. trigger the action, and 3. delete the popups. This keeps the menu popups and their contents alive until we return from the action triggered handler. We also need to take care of manually destroying any popup we may create. Finally, the menu content creation in Menu.qml had to be tweaked since we shouldn't rely on the popup visibility anymore. Task-number: QTBUG-45182 Change-Id: I9f1155bbf74dd3353c6c4066a24abf1cd2c3a283 Reviewed-by: Caroline Chao Reviewed-by: J-P Nurmi --- src/controls/ComboBox.qml | 1 + src/controls/Menu.qml | 17 +++++++++--- src/controls/MenuBar.qml | 5 ++-- src/controls/Private/EditMenu_base.qml | 6 +++-- src/controls/Private/MenuContentItem.qml | 13 ++++++--- src/controls/qquickmenu.cpp | 46 ++++++++++++++++++++++++-------- src/controls/qquickmenu_p.h | 5 ++++ src/controls/qquickmenupopupwindow.cpp | 13 +++++++-- src/controls/qquickmenupopupwindow_p.h | 6 +++++ 9 files changed, 88 insertions(+), 24 deletions(-) diff --git a/src/controls/ComboBox.qml b/src/controls/ComboBox.qml index 4dc8ca39..7891cd44 100644 --- a/src/controls/ComboBox.qml +++ b/src/controls/ComboBox.qml @@ -628,6 +628,7 @@ Control { function toggleShow() { if (popup.__popupVisible) { popup.__dismissMenu() + popup.__destroyAllMenuPopups() } else { if (items[__selectedIndex]) items[__selectedIndex].checked = true diff --git a/src/controls/Menu.qml b/src/controls/Menu.qml index 48407488..5374a599 100644 --- a/src/controls/Menu.qml +++ b/src/controls/Menu.qml @@ -148,13 +148,24 @@ MenuPrivate { property int __currentIndex: -1 /*! \internal */ on__MenuClosed: __currentIndex = -1 + on__MenuPopupDestroyed: contentLoader.active = false + onPopupVisibleChanged: { + if (__popupVisible) + contentLoader.active = true + } /*! \internal */ __contentItem: Loader { - sourceComponent: MenuContentItem { - __menu: root + id: contentLoader + Component { + id: menuContent + MenuContentItem { + __menu: root + } } - active: !root.__isNative && root.__popupVisible + + sourceComponent: root.__isNative ? null : menuContent + active: false focus: true Keys.forwardTo: item ? [item, root.__parentContentItem] : [] property bool altPressed: root.__parentContentItem ? root.__parentContentItem.altPressed : false diff --git a/src/controls/MenuBar.qml b/src/controls/MenuBar.qml index 272ed444..711d25c2 100644 --- a/src/controls/MenuBar.qml +++ b/src/controls/MenuBar.qml @@ -256,8 +256,9 @@ MenuBarPrivate { menuBarLoader.height - d.heightPadding, 0, 0), 0) if (d.preselectMenuItem) __menuItem.__currentIndex = 0 - } else { - __menuItem.__closeMenu() + } else if (__menuItem.__popupVisible) { + __menuItem.__dismissMenu() + __menuItem.__destroyAllMenuPopups() } } } diff --git a/src/controls/Private/EditMenu_base.qml b/src/controls/Private/EditMenu_base.qml index b11cbf25..cda4d2b3 100644 --- a/src/controls/Private/EditMenu_base.qml +++ b/src/controls/Private/EditMenu_base.qml @@ -102,9 +102,11 @@ Item { input.activate() if (control.menu) { - getMenuInstance().__dismissMenu(); + var menu = getMenuInstance(); + menu.__dismissMenu(); + menu.__destroyAllMenuPopups(); var menuPos = mapToItem(null, mouse.x, mouse.y) - getMenuInstance().__popup(Qt.rect(menuPos.x, menuPos.y, 0, 0), -1, MenuPrivate.EditMenu); + 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 d926ff75..1633a81f 100644 --- a/src/controls/Private/MenuContentItem.qml +++ b/src/controls/Private/MenuContentItem.qml @@ -45,7 +45,7 @@ import QtQuick.Controls.Styles 1.1 Loader { id: menuFrameLoader - property var __menu: root + property var __menu visible: status === Loader.Ready width: content.width + (d.style ? d.style.padding.left + d.style.padding.right : 0) @@ -89,6 +89,7 @@ Loader { __menu.__dismissMenu() if (item.styleData.type !== MenuItemType.Menu) item.__menuItem.trigger() + __menu.__destroyAllMenuPopups() } } } @@ -132,8 +133,10 @@ Loader { } Keys.onLeftPressed: { - if ((event.accepted = __menu.__parentMenu.hasOwnProperty("title"))) - __closeMenu() + if ((event.accepted = __menu.__parentMenu.hasOwnProperty("title"))) { + __menu.__closeMenu() + __menu.__destroyMenuPopup() + } } Keys.onRightPressed: { @@ -231,8 +234,10 @@ Loader { id: closeMenuTimer interval: 1 onTriggered: { - if (__menu.__currentIndex !== __menuItemIndex) + if (__menu.__currentIndex !== __menuItemIndex) { __menuItem.__closeMenu() + __menuItem.__destroyMenuPopup() + } } } diff --git a/src/controls/qquickmenu.cpp b/src/controls/qquickmenu.cpp index 1a125135..80b5607e 100644 --- a/src/controls/qquickmenu.cpp +++ b/src/controls/qquickmenu.cpp @@ -428,6 +428,7 @@ void QQuickMenu::__popup(const QRectF &targetRect, int atItemIndex, MenuType men connect(m_popupWindow, SIGNAL(visibleChanged(bool)), this, SLOT(windowVisibleChanged(bool))); connect(m_popupWindow, SIGNAL(geometryChanged()), this, SIGNAL(__popupGeometryChanged())); + connect(m_popupWindow, SIGNAL(willBeDeletedLater()), this, SLOT(clearPopupWindow())); m_popupWindow->setPosition(targetRect.x() + m_xOffset + renderOffset.x(), targetRect.y() + targetRect.height() + m_yOffset + renderOffset.y()); @@ -468,34 +469,57 @@ void QQuickMenu::__closeMenu() emit __menuClosed(); } +QQuickMenuPopupWindow *QQuickMenu::topMenuPopup() const +{ + QQuickMenuPopupWindow *topMenuWindow = m_popupWindow; + while (topMenuWindow) { + QQuickMenuPopupWindow *pw = qobject_cast(topMenuWindow->transientParent()); + if (!pw) + return topMenuWindow; + topMenuWindow = pw; + } + + return 0; +} + void QQuickMenu::__dismissMenu() { if (m_platformMenu) { m_platformMenu->dismiss(); - } else { - QQuickMenuPopupWindow *topMenuWindow = m_popupWindow; - while (topMenuWindow) { - QQuickMenuPopupWindow *pw = qobject_cast(topMenuWindow->transientParent()); - if (!pw) - topMenuWindow->dismissPopup(); - topMenuWindow = pw; - } + } else if (QQuickMenuPopupWindow *topPopup = topMenuPopup()) { + topPopup->dismissPopup(); } } void QQuickMenu::windowVisibleChanged(bool v) { if (!v) { - if (qobject_cast(m_popupWindow->transientParent())) { + if (m_popupWindow && qobject_cast(m_popupWindow->transientParent())) { m_popupWindow->transientParent()->setMouseGrabEnabled(true); m_popupWindow->transientParent()->setKeyboardGrabEnabled(true); } - m_popupWindow->deleteLater(); - m_popupWindow = 0; __closeMenu(); } } +void QQuickMenu::clearPopupWindow() +{ + m_popupWindow = 0; + emit __menuPopupDestroyed(); +} + +void QQuickMenu::__destroyMenuPopup() +{ + if (m_popupWindow) + m_popupWindow->setToBeDeletedLater(); +} + +void QQuickMenu::__destroyAllMenuPopups() { + QQuickMenuPopupWindow *popup = topMenuPopup(); + if (popup) + popup->setToBeDeletedLater(); +} + 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 f21f2ce0..03e8bd43 100644 --- a/src/controls/qquickmenu_p.h +++ b/src/controls/qquickmenu_p.h @@ -88,6 +88,8 @@ public: public Q_SLOTS: void __closeMenu(); void __dismissMenu(); + void __destroyMenuPopup(); + void __destroyAllMenuPopups(); Q_SIGNALS: void itemsChanged(); @@ -95,6 +97,7 @@ Q_SIGNALS: void __selectedIndexChanged(); void __menuClosed(); + void __menuPopupDestroyed(); void popupVisibleChanged(); void __popupGeometryChanged(); void menuContentItemChanged(); @@ -140,6 +143,7 @@ protected Q_SLOTS: void setMenuContentItem(QQuickItem *); void setPopupVisible(bool); + void clearPopupWindow(); void updateText(); void windowVisibleChanged(bool); @@ -147,6 +151,7 @@ protected Q_SLOTS: private: QQuickWindow *findParentWindow(); void syncParentMenuBar(); + QQuickMenuPopupWindow *topMenuPopup() const; int itemIndexForListIndex(int listIndex) const; void itemIndexToListIndex(int, int *, int *) const; diff --git a/src/controls/qquickmenupopupwindow.cpp b/src/controls/qquickmenupopupwindow.cpp index bdc8fc25..b8c10b66 100644 --- a/src/controls/qquickmenupopupwindow.cpp +++ b/src/controls/qquickmenupopupwindow.cpp @@ -80,12 +80,21 @@ void QQuickMenuPopupWindow::setParentWindow(QWindow *effectiveParentWindow, QQui setTransientParent(effectiveParentWindow); m_logicalParentWindow = parentWindow; if (parentWindow) { - connect(parentWindow, SIGNAL(destroyed()), this, SLOT(dismissPopup())); - if (QQuickMenuPopupWindow *pw = qobject_cast(parentWindow)) + if (QQuickMenuPopupWindow *pw = qobject_cast(parentWindow)) { connect(pw, SIGNAL(popupDismissed()), this, SLOT(dismissPopup())); + connect(pw, SIGNAL(willBeDeletedLater()), this, SLOT(setToBeDeletedLater())); + } else { + connect(parentWindow, SIGNAL(destroyed()), this, SLOT(deleteLater())); + } } } +void QQuickMenuPopupWindow::setToBeDeletedLater() +{ + deleteLater(); + emit willBeDeletedLater(); +} + void QQuickMenuPopupWindow::setGeometry(int posx, int posy, int w, int h) { QWindow *pw = transientParent(); diff --git a/src/controls/qquickmenupopupwindow_p.h b/src/controls/qquickmenupopupwindow_p.h index 83216373..e5138073 100644 --- a/src/controls/qquickmenupopupwindow_p.h +++ b/src/controls/qquickmenupopupwindow_p.h @@ -50,10 +50,16 @@ public: void setParentItem(QQuickItem *); +public Q_SLOTS: + void setToBeDeletedLater(); + protected Q_SLOTS: void updateSize(); void updatePosition(); +Q_SIGNALS: + void willBeDeletedLater(); + protected: void exposeEvent(QExposeEvent *); -- cgit v1.2.1