From 6e6add5406eb5d8daca0992fa3f2362d34b002e5 Mon Sep 17 00:00:00 2001
From: Oliver Eftevaag <oliver.eftevaag@qt.io>
Date: Tue, 19 Nov 2024 10:06:52 +0100
Subject: [PATCH] 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
Change-Id: I6b7e3e3d879454f29c0e8665b59f1eca6a88fea2
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
(cherry picked from commit 79b354e00614ea6d7740815b94b30713652acdf2)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 43923f44d16b438c40e86233a5e68ea4ba499681)
---
 src/quicktemplates/qquickpopupwindow.cpp      | 63 +++++++++++--------
 src/quicktemplates/qquickpopupwindow_p_p.h    |  3 +-
 .../reparentingPopupToDifferentWindows.qml    | 46 ++++++++++++++
 .../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 ea1fbe015e..890c609684 100644
--- a/src/quicktemplates/qquickpopupwindow.cpp
+++ b/src/quicktemplates/qquickpopupwindow.cpp
@@ -32,6 +32,7 @@ class QQuickPopupWindowPrivate : public QQuickWindowQmlImplPrivate
 public:
     QPointer<QQuickItem> m_popupItem;
     QPointer<QQuickPopup> m_popup;
+    QPointer<QWindow> 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<QPoint> 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<QPoint> 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 b4ec0ba696..3e5af71381 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<QQuickPopup *>();
+    QVERIFY(popup);
+    auto *popupPrivate = QQuickPopupPrivate::get(popup);
+    QVERIFY(popupPrivate);
+
+    window->show();
+    QVERIFY(QTest::qWaitForWindowExposed(window));
+    QVERIFY(!popup->isVisible());
+
+    QQuickWindow *childWindow = window->property("childWindow").value<QQuickWindow *>();
+    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)
-- 
GitLab

