From 988039729f55a5e62e32b82c6f29ff7457e91d9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tor=20Arne=20Vestb=C3=B8?= Date: Mon, 8 Apr 2024 17:19:10 +0200 Subject: [PATCH] QWindow: Persist foreign winId to support destroy/create cycles We used to set a private _q_foreignWinId property on QWindow when creating foreign windows, and this was the mechanism which we then passed the foreign winId to the platform plugin. With c585802e946d97e7d177ea334a162dc7bc286b84 this was removed, since we now were passing the winId through via explicit QPA APIs, and since 0c6911e5cde24c45d6f2c08b6e71064bdd1eccfa removed the ability to explicitly destroy() a foreign window. But when closing a QWindow, we destroy both the window itself, and all its children, including foreign windows. In this case we still want to support recreating the foreign window, for example when the parent window is shown again. To enable this we restore the _q_foreignWinId private property, but keep the limitation of not being able to explicitly destroy a foreign window. Pick-to: 6.7 6.5 Fixes: QTBUG-124160 Change-Id: Ia885ba9f043e64fb21eedd2b4c344679726f1b5c Reviewed-by: Oliver Wolff --- src/gui/kernel/qwindow.cpp | 22 ++++++++- src/gui/kernel/qwindow_p.h | 2 +- src/plugins/platforms/ios/qioswindow.mm | 3 +- .../platforms/windows/qwindowswindow.cpp | 2 + src/plugins/platforms/xcb/qxcbwindow.cpp | 4 ++ .../gui/kernel/qwindow/tst_foreignwindow.cpp | 48 +++++++++++++++++++ tests/shared/nativewindow.h | 2 +- 7 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/gui/kernel/qwindow.cpp b/src/gui/kernel/qwindow.cpp index 46a787e7064..b40fd7e8e84 100644 --- a/src/gui/kernel/qwindow.cpp +++ b/src/gui/kernel/qwindow.cpp @@ -521,7 +521,9 @@ void QWindowPrivate::setTopLevelScreen(QScreen *newScreen, bool recreate) } } -void QWindowPrivate::create(bool recursive, WId nativeHandle) +static constexpr auto kForeignWindowId = "_q_foreignWinId"; + +void QWindowPrivate::create(bool recursive) { Q_Q(QWindow); if (platformWindow) @@ -549,6 +551,8 @@ void QWindowPrivate::create(bool recursive, WId nativeHandle) setTopLevelScreen(screen, false); } + const WId nativeHandle = q->property(kForeignWindowId).value(); + QPlatformIntegration *platformIntegration = QGuiApplicationPrivate::platformIntegration(); platformWindow = nativeHandle ? platformIntegration->createForeignWindow(q, nativeHandle) : platformIntegration->createPlatformWindow(q); @@ -2059,6 +2063,16 @@ void QWindowPrivate::destroy() QObject *object = childrenWindows.at(i); if (object->isWindowType()) { QWindow *w = static_cast(object); + auto *childPlatformWindow = w->handle(); + if (!childPlatformWindow) + continue; + + // Decouple the foreign window from this window, + // so that destroying our native handle doesn't + // bring down the foreign window as well. + if (childPlatformWindow->isForeignWindow()) + childPlatformWindow->setParent(nullptr); + qt_window_private(w)->destroy(); } } @@ -2986,7 +3000,11 @@ QWindow *QWindow::fromWinId(WId id) } QWindow *window = new QWindow; - qt_window_private(window)->create(false, id); + + // Persist the winId in a private property so that we + // can recreate the window after being destroyed. + window->setProperty(kForeignWindowId, id); + window->create(); if (!window->handle()) { delete window; diff --git a/src/gui/kernel/qwindow_p.h b/src/gui/kernel/qwindow_p.h index 1bcbda6b8fd..a9716847a1e 100644 --- a/src/gui/kernel/qwindow_p.h +++ b/src/gui/kernel/qwindow_p.h @@ -66,7 +66,7 @@ public: void updateSiblingPosition(SiblingPosition); bool windowRecreationRequired(QScreen *newScreen) const; - void create(bool recursive, WId nativeHandle = 0); + void create(bool recursive); void destroy(); void setTopLevelScreen(QScreen *newScreen, bool recreate); void connectToScreen(QScreen *topLevelScreen); diff --git a/src/plugins/platforms/ios/qioswindow.mm b/src/plugins/platforms/ios/qioswindow.mm index 6a25e24c2d6..90e955b90be 100644 --- a/src/plugins/platforms/ios/qioswindow.mm +++ b/src/plugins/platforms/ios/qioswindow.mm @@ -55,7 +55,8 @@ QIOSWindow::QIOSWindow(QWindow *window, WId nativeHandle) connect(qGuiApp, &QGuiApplication::applicationStateChanged, this, &QIOSWindow::applicationStateChanged); - setParent(QPlatformWindow::parent()); + if (QPlatformWindow::parent()) + setParent(QPlatformWindow::parent()); if (!isForeignWindow()) { // Resolve default window geometry in case it was not set before creating the diff --git a/src/plugins/platforms/windows/qwindowswindow.cpp b/src/plugins/platforms/windows/qwindowswindow.cpp index 220c36cc192..c54da57ffe8 100644 --- a/src/plugins/platforms/windows/qwindowswindow.cpp +++ b/src/plugins/platforms/windows/qwindowswindow.cpp @@ -1357,6 +1357,8 @@ QWindowsForeignWindow::QWindowsForeignWindow(QWindow *window, HWND hwnd) , m_hwnd(hwnd) , m_topLevelStyle(0) { + if (QPlatformWindow::parent()) + setParent(QPlatformWindow::parent()); } void QWindowsForeignWindow::setParent(const QPlatformWindow *newParentWindow) diff --git a/src/plugins/platforms/xcb/qxcbwindow.cpp b/src/plugins/platforms/xcb/qxcbwindow.cpp index 09d4cd9833a..d3e4fa95482 100644 --- a/src/plugins/platforms/xcb/qxcbwindow.cpp +++ b/src/plugins/platforms/xcb/qxcbwindow.cpp @@ -522,6 +522,10 @@ QXcbForeignWindow::QXcbForeignWindow(QWindow *window, WId nativeHandle) QRect nativeGeometry(geometry->x, geometry->y, geometry->width, geometry->height); QPlatformWindow::setGeometry(nativeGeometry); } + + // And reparent, if we have a parent already + if (QPlatformWindow::parent()) + setParent(QPlatformWindow::parent()); } QXcbForeignWindow::~QXcbForeignWindow() diff --git a/tests/auto/gui/kernel/qwindow/tst_foreignwindow.cpp b/tests/auto/gui/kernel/qwindow/tst_foreignwindow.cpp index e7b05e70374..526abd6ea37 100644 --- a/tests/auto/gui/kernel/qwindow/tst_foreignwindow.cpp +++ b/tests/auto/gui/kernel/qwindow/tst_foreignwindow.cpp @@ -26,6 +26,9 @@ private slots: void embedForeignWindow(); void embedInForeignWindow(); + + void destroyExplicitly(); + void destroyWhenParentIsDestroyed(); }; void tst_ForeignWindow::fromWinId() @@ -138,5 +141,50 @@ void tst_ForeignWindow::embedInForeignWindow() } } +void tst_ForeignWindow::destroyExplicitly() +{ + NativeWindow nativeWindow; + QVERIFY(nativeWindow); + + std::unique_ptr foreignWindow(QWindow::fromWinId(nativeWindow)); + QVERIFY(foreignWindow->handle()); + + // Explicitly destroying a foreign window is a no-op, as + // the documentation claims that it "releases the native + // platform resources associated with this window.", which + // is not technically true for foreign windows. + auto *windowHandleBeforeDestroy = foreignWindow->handle(); + foreignWindow->destroy(); + QCOMPARE(foreignWindow->handle(), windowHandleBeforeDestroy); +} + +void tst_ForeignWindow::destroyWhenParentIsDestroyed() +{ + QWindow parentWindow; + + NativeWindow nativeWindow; + QVERIFY(nativeWindow); + + std::unique_ptr foreignWindow(QWindow::fromWinId(nativeWindow)); + foreignWindow->setParent(&parentWindow); + QTRY_COMPARE(nativeWindow.parentWinId(), parentWindow.winId()); + + // Reparenting into a window will result in creating it + QVERIFY(parentWindow.handle()); + + // Destroying the parent window of the foreign window results + // in destroying the foreign window as well, as the foreign + // window no longer has a parent it can be embedded in. + QVERIFY(foreignWindow->handle()); + parentWindow.destroy(); + QVERIFY(!foreignWindow->handle()); + + // But the foreign window can be recreated again, and will + // continue to be a native child of the parent window. + foreignWindow->create(); + QVERIFY(foreignWindow->handle()); + QTRY_COMPARE(nativeWindow.parentWinId(), parentWindow.winId()); +} + #include QTEST_MAIN(tst_ForeignWindow) diff --git a/tests/shared/nativewindow.h b/tests/shared/nativewindow.h index 51d211704ec..932d2d419af 100644 --- a/tests/shared/nativewindow.h +++ b/tests/shared/nativewindow.h @@ -238,7 +238,7 @@ WId NativeWindow::parentWinId() const xcb_query_tree_reply_t *tree = xcb_query_tree_reply( connection, xcb_query_tree(connection, m_handle), nullptr); const auto cleanup = qScopeGuard([&]{ free(tree); }); - return tree->parent; + return tree ? tree->parent : 0; } bool NativeWindow::isParentOf(WId childWinId)