From 43923f44d16b438c40e86233a5e68ea4ba499681 Mon Sep 17 00:00:00 2001 From: Oliver Eftevaag Date: Tue, 19 Nov 2024 10:06:52 +0100 Subject: PopupWindow: Keep better track of the nearest parent item's window Since Popup::{x,y} are relative to the popup's nearest parent item, any movement to the parent window (when using popup windows), must cause the popup to change its x and y properties to reflect this new delta. Since it's possible to change the popup's parent via JavaScript/QML, and a new parent item could exist in another window, we'll need to ensure that we're always connected to the correct window at all times. Keeping a local reference to this window should be the better approach, so that we can properly disconnect when the nearest parent item's window changes, or when we're destroying the popup window. Users are also occasionally experiencing crashes when using popup windows. Based on the backtrace, it seems like these crashes are also related to us not disconnecting correctly when then the popup window's parent window changes. Task-number: QTBUG-131898 Pick-to: 6.8 Change-Id: I6b7e3e3d879454f29c0e8665b59f1eca6a88fea2 Reviewed-by: Richard Moe Gustavsen (cherry picked from commit 79b354e00614ea6d7740815b94b30713652acdf2) Reviewed-by: Qt Cherry-pick Bot --- src/quicktemplates/qquickpopupwindow.cpp | 63 +++++++++++++--------- src/quicktemplates/qquickpopupwindow_p_p.h | 3 +- .../data/reparentingPopupToDifferentWindows.qml | 46 ++++++++++++++++ .../quickcontrols/qquickpopup/tst_qquickpopup.cpp | 57 ++++++++++++++++++++ 4 files changed, 142 insertions(+), 27 deletions(-) create mode 100644 tests/auto/quickcontrols/qquickpopup/data/reparentingPopupToDifferentWindows.qml diff --git a/src/quicktemplates/qquickpopupwindow.cpp b/src/quicktemplates/qquickpopupwindow.cpp index 5cf0a7a3e0..643971a560 100644 --- a/src/quicktemplates/qquickpopupwindow.cpp +++ b/src/quicktemplates/qquickpopupwindow.cpp @@ -32,6 +32,7 @@ class QQuickPopupWindowPrivate : public QQuickWindowQmlImplPrivate public: QPointer m_popupItem; QPointer m_popup; + QPointer m_popupParentItemWindow; bool m_inHideEvent = false; protected: @@ -53,9 +54,11 @@ QQuickPopupWindow::QQuickPopupWindow(QQuickPopup *popup, QWindow *parent) connect(d->m_popup, &QQuickPopup::windowChanged, this, &QQuickPopupWindow::windowChanged); connect(d->m_popup, &QQuickPopup::implicitWidthChanged, this, &QQuickPopupWindow::implicitWidthChanged); connect(d->m_popup, &QQuickPopup::implicitHeightChanged, this, &QQuickPopupWindow::implicitHeightChanged); - connect(d->m_popup->window(), &QWindow::xChanged, this, &QQuickPopupWindow::parentWindowXChanged); - connect(d->m_popup->window(), &QWindow::yChanged, this, &QQuickPopupWindow::parentWindowYChanged); - + if (QQuickWindow *nearestParentItemWindow = d->m_popup->window()) { + d->m_popupParentItemWindow = nearestParentItemWindow; + connect(d->m_popupParentItemWindow, &QWindow::xChanged, this, &QQuickPopupWindow::parentWindowXChanged); + connect(d->m_popupParentItemWindow, &QWindow::yChanged, this, &QQuickPopupWindow::parentWindowYChanged); + } setWidth(d->m_popupItem->implicitWidth()); setHeight(d->m_popupItem->implicitHeight()); @@ -70,18 +73,6 @@ QQuickPopupWindow::QQuickPopupWindow(QQuickPopup *popup, QWindow *parent) qCDebug(lcPopupWindow) << "Created popup window with parent:" << parent << "flags:" << flags; } -QQuickPopupWindow::~QQuickPopupWindow() -{ - Q_D(QQuickPopupWindow); - disconnect(d->m_popup, &QQuickPopup::windowChanged, this, &QQuickPopupWindow::windowChanged); - disconnect(d->m_popup, &QQuickPopup::implicitWidthChanged, this, &QQuickPopupWindow::implicitWidthChanged); - disconnect(d->m_popup, &QQuickPopup::implicitHeightChanged, this, &QQuickPopupWindow::implicitHeightChanged); - if (d->m_popup->window()) { - disconnect(d->m_popup->window(), &QWindow::xChanged, this, &QQuickPopupWindow::parentWindowXChanged); - disconnect(d->m_popup->window(), &QWindow::yChanged, this, &QQuickPopupWindow::parentWindowYChanged); - } -} - QQuickPopup *QQuickPopupWindow::popup() const { Q_D(const QQuickPopupWindow); @@ -124,8 +115,11 @@ void QQuickPopupWindow::resizeEvent(QResizeEvent *e) // does not move the window const auto oldX = popupPrivate->x; const auto oldY = popupPrivate->y; - popupPrivate->x = topLeftFromSystem.x(); - popupPrivate->y = topLeftFromSystem.y(); + + if (Q_LIKELY(topLeftFromSystem)) { + popupPrivate->x = topLeftFromSystem->x(); + popupPrivate->y = topLeftFromSystem->y(); + } const QMarginsF windowInsets = popupPrivate->windowInsets(); d->m_popupItem->setWidth(e->size().width() - windowInsets.left() - windowInsets.right()); @@ -329,18 +323,32 @@ bool QQuickPopupWindow::event(QEvent *e) void QQuickPopupWindow::windowChanged(QWindow *window) { + Q_D(QQuickPopupWindow); + if (!d->m_popupParentItemWindow.isNull()) { + disconnect(d->m_popupParentItemWindow, &QWindow::xChanged, this, &QQuickPopupWindow::parentWindowXChanged); + disconnect(d->m_popupParentItemWindow, &QWindow::yChanged, this, &QQuickPopupWindow::parentWindowYChanged); + } if (window) { + d->m_popupParentItemWindow = window; connect(window, &QWindow::xChanged, this, &QQuickPopupWindow::parentWindowXChanged); connect(window, &QWindow::yChanged, this, &QQuickPopupWindow::parentWindowYChanged); + } else { + d->m_popupParentItemWindow.clear(); } } -QPoint QQuickPopupWindow::global2Local(const QPoint &pos) const +std::optional QQuickPopupWindow::global2Local(const QPoint &pos) const { Q_D(const QQuickPopupWindow); QQuickPopup *popup = d->m_popup; Q_ASSERT(popup); - const QPoint scenePos = popup->window()->mapFromGlobal(pos); + QWindow *mainWindow = d->m_popupParentItemWindow; + if (!mainWindow) + mainWindow = transientParent(); + if (Q_UNLIKELY((!mainWindow || mainWindow != popup->window()))) + return std::nullopt; + + const QPoint scenePos = mainWindow->mapFromGlobal(pos); // Popup's coordinates are relative to the nearest parent item. return popup->parentItem() ? popup->parentItem()->mapFromScene(scenePos).toPoint() : scenePos; } @@ -348,26 +356,31 @@ QPoint QQuickPopupWindow::global2Local(const QPoint &pos) const void QQuickPopupWindow::parentWindowXChanged(int newX) { const auto popupLocalPos = global2Local({x(), y()}); - handlePopupPositionChangeFromWindowSystem({newX + popupLocalPos.x(), y()}); + if (Q_UNLIKELY(!popupLocalPos)) + return; + handlePopupPositionChangeFromWindowSystem({ newX + popupLocalPos->x(), y() }); } void QQuickPopupWindow::parentWindowYChanged(int newY) { const auto popupLocalPos = global2Local({x(), y()}); - handlePopupPositionChangeFromWindowSystem({x(), newY + popupLocalPos.y()}); + if (Q_UNLIKELY(!popupLocalPos)) + return; + handlePopupPositionChangeFromWindowSystem({ x(), newY + popupLocalPos->y() }); } void QQuickPopupWindow::handlePopupPositionChangeFromWindowSystem(const QPoint &pos) { Q_D(QQuickPopupWindow); QQuickPopup *popup = d->m_popup; - if (!popup || !popup->window()) + if (!popup) return; - QQuickPopupPrivate *popupPrivate = QQuickPopupPrivate::get(popup); const auto windowPos = global2Local(pos); - qCDebug(lcPopupWindow) << "A window system event changed the popup's position to be " << windowPos; - popupPrivate->setEffectivePosFromWindowPos(windowPos); + if (Q_LIKELY(windowPos)) { + qCDebug(lcPopupWindow) << "A window system event changed the popup's position to be " << *windowPos; + QQuickPopupPrivate::get(popup)->setEffectivePosFromWindowPos(*windowPos); + } } void QQuickPopupWindow::implicitWidthChanged() diff --git a/src/quicktemplates/qquickpopupwindow_p_p.h b/src/quicktemplates/qquickpopupwindow_p_p.h index 0b9842c059..ae13d57aa5 100644 --- a/src/quicktemplates/qquickpopupwindow_p_p.h +++ b/src/quicktemplates/qquickpopupwindow_p_p.h @@ -30,7 +30,6 @@ class Q_QUICKTEMPLATES2_EXPORT QQuickPopupWindow : public QQuickWindowQmlImpl public: explicit QQuickPopupWindow(QQuickPopup *popup, QWindow *parent = nullptr); - ~QQuickPopupWindow(); QQuickPopup *popup() const; protected: @@ -41,7 +40,7 @@ protected: private: void windowChanged(QWindow *window); - QPoint global2Local(const QPoint& pos) const; + std::optional global2Local(const QPoint &pos) const; void parentWindowXChanged(int newX); void parentWindowYChanged(int newY); void handlePopupPositionChangeFromWindowSystem(const QPoint &pos); diff --git a/tests/auto/quickcontrols/qquickpopup/data/reparentingPopupToDifferentWindows.qml b/tests/auto/quickcontrols/qquickpopup/data/reparentingPopupToDifferentWindows.qml new file mode 100644 index 0000000000..d80459b02a --- /dev/null +++ b/tests/auto/quickcontrols/qquickpopup/data/reparentingPopupToDifferentWindows.qml @@ -0,0 +1,46 @@ +// Copyright (C) 2024 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only + +import QtQuick +import QtQuick.Controls + +Window { + property alias popup: popup + property alias childWindow: window2 + id: window1 + width: 500 + height: 500 + + Text { + anchors.centerIn: parent + text: "window1" + } + + Dialog { + id: popup + popupType: Popup.Window + width: 400 + height: 400 + background: Rectangle { + color: "green" + } + + Text { + anchors.centerIn: parent + text: "popup x " + popup.x + " y " + popup.y + } + } + + Window { + id: window2 + x: 500 + width: 500 + height: 500 + + Text { + anchors.centerIn: parent + text: "window2" + } + } +} + diff --git a/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp b/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp index 3c0ddc2cf2..dc4c70b81e 100644 --- a/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp +++ b/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp @@ -125,6 +125,7 @@ private slots: void initialPopupSize_data(); void initialPopupSize(); void popupWindowChangingParent(); + void popupWindowChangingParentWindow(); void popupWindowFocus(); void popupTypeChangeFromWindowToItem(); void popupTypeChangeFromItemToWindow(); @@ -2905,6 +2906,62 @@ void tst_QQuickPopup::popupWindowChangingParent() VERIFY_LOCAL_POS(popup, initialPos); } +void tst_QQuickPopup::popupWindowChangingParentWindow() +{ + if (!popupWindowsSupported) + QSKIP("The platform doesn't support popup windows. Skipping test."); + + QQuickApplicationHelper helper(this, "reparentingPopupToDifferentWindows.qml"); + QVERIFY2(helper.ready, helper.failureMessage()); + QQuickWindow *window = helper.window; + auto *popup = window->contentItem()->findChild(); + QVERIFY(popup); + auto *popupPrivate = QQuickPopupPrivate::get(popup); + QVERIFY(popupPrivate); + + window->show(); + QVERIFY(QTest::qWaitForWindowExposed(window)); + QVERIFY(!popup->isVisible()); + + QQuickWindow *childWindow = window->property("childWindow").value(); + QVERIFY(childWindow); + + childWindow->show(); + QVERIFY(QTest::qWaitForWindowExposed(childWindow)); + QVERIFY(!popup->isVisible()); + + popup->open(); + QTRY_VERIFY(popup->isOpened()); + QTRY_VERIFY(popupPrivate->popupWindow); + auto *popupWindow = popupPrivate->popupWindow; + QVERIFY(QTest::qWaitForWindowExposed(popupWindow)); + QQuickTest::qWaitForPolish(popupWindow); + + QCOMPARE(popup->parentItem(), window->contentItem()); + // The expected value is 0, but we allow for 1 pixel of leniency, + // similar to VERIFY_GLOBAL_POS. + QCOMPARE_LT(qAbs(popup->x()), 2); + QCOMPARE_LT(qAbs(popup->y()), 2); + + popup->close(); + QVERIFY(!popup->isVisible()); + popup->setParentItem(childWindow->contentItem()); + popup->open(); + QTRY_VERIFY(popup->isOpened()); + QVERIFY(QTest::qWaitForWindowExposed(popupWindow)); + QQuickTest::qWaitForPolish(popupWindow); + QCOMPARE(popup->parentItem(), childWindow->contentItem()); + + QSignalSpy windowMoveSpy(window, &QWindow::yChanged); + window->setY(window->y() + 100); + QTRY_COMPARE(windowMoveSpy.count(), 1); + QCOMPARE_LT(qAbs(popup->x()), 2); + QCOMPARE_LT(qAbs(popup->y()), 2); + + popup->close(); + QTRY_VERIFY(!popup->isVisible()); +} + void tst_QQuickPopup::popupWindowFocus() { #if defined(Q_OS_QNX) -- cgit v1.2.3