From af1f258c11112433176be2e73a916c3a774bc47d Mon Sep 17 00:00:00 2001 From: David Redondo Date: Tue, 29 Oct 2024 17:07:18 +0100 Subject: [PATCH] client: Fix changing to subsurface now that wl_surface is not destroyed In the past isVisible() was equivalent to having a wl_surface - this does not hold anymore. The QWaylandWindow will have a wl_surface for all its life time. Changing parent while the window is hidden would run now into a protocol error. Change-Id: I2aec6217ad495daa9dbb2ee2fee1b73ba00e6937 Reviewed-by: David Edmundson --- .../platforms/wayland/qwaylandwindow.cpp | 15 +++--- .../platforms/wayland/qwaylandwindow_p.h | 1 + tests/auto/wayland/shared/coreprotocol.cpp | 27 ++++++++++ tests/auto/wayland/shared/coreprotocol.h | 51 +++++++++++++------ tests/auto/wayland/shared/xdgshell.cpp | 12 +++++ tests/auto/wayland/shared/xdgshell.h | 10 ++++ tests/auto/wayland/surface/tst_surface.cpp | 28 ++++++++++ 7 files changed, 122 insertions(+), 22 deletions(-) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index ab6cae5d0ce..cb6c07f9799 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -354,21 +354,22 @@ WId QWaylandWindow::winId() const void QWaylandWindow::setParent(const QPlatformWindow *parent) { - if (!window()->isVisible()) - return; - - QWaylandWindow *oldparent = mSubSurfaceWindow ? mSubSurfaceWindow->parent() : nullptr; - if (oldparent == parent) + if (lastParent == parent) return; if (mSubSurfaceWindow && parent) { // new parent, but we were a subsurface already delete mSubSurfaceWindow; QWaylandWindow *p = const_cast(static_cast(parent)); mSubSurfaceWindow = new QWaylandSubSurface(this, p, mDisplay->createSubSurface(this, p)); - } else { // we're changing role, need to make a new wl_surface + } else if ((!lastParent && parent) || (lastParent && !parent)) { + // we're changing role, need to make a new wl_surface reset(); - initWindow(); + initializeWlSurface(); + if (window()->isVisible()) { + initWindow(); + } } + lastParent = parent; } QString QWaylandWindow::windowTitle() const diff --git a/src/plugins/platforms/wayland/qwaylandwindow_p.h b/src/plugins/platforms/wayland/qwaylandwindow_p.h index c0a93189fea..e4ce2af700f 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow_p.h +++ b/src/plugins/platforms/wayland/qwaylandwindow_p.h @@ -366,6 +366,7 @@ private: static const wl_callback_listener callbackListener; void handleFrameCallback(struct ::wl_callback* callback); + const QPlatformWindow *lastParent = nullptr; static QWaylandWindow *mMouseGrab; static QWaylandWindow *mTopPopup; diff --git a/tests/auto/wayland/shared/coreprotocol.cpp b/tests/auto/wayland/shared/coreprotocol.cpp index 3152f26148a..aba270f13bc 100644 --- a/tests/auto/wayland/shared/coreprotocol.cpp +++ b/tests/auto/wayland/shared/coreprotocol.cpp @@ -19,6 +19,7 @@ Surface::~Surface() qDeleteAll(m_commits); if (m_wlShellSurface) m_wlShellSurface->m_surface = nullptr; + delete m_role; } void Surface::sendFrameCallbacks() @@ -181,6 +182,25 @@ QString WlCompositor::dirtyMessage() return "Dirty, surfaces left:\n\t" + messages.join("\n\t"); } +void SubCompositor::subcompositor_get_subsurface(Resource *resource, uint32_t id, + ::wl_resource *surfaceResource, + ::wl_resource *parent) +{ + QTRY_VERIFY(parent); + QTRY_VERIFY(surfaceResource); + auto surface = fromResource(surfaceResource); + if (!surface->m_role) { + surface->m_role = new SubSurfaceRole; + } else if (!qobject_cast(surface->m_role)) { + QFAIL(QByteArrayLiteral("surface already has role") + surface->m_role->metaObject()->className()); + return; + } + + auto *subsurface = new Subsurface(this, resource->client(), id, resource->version()); + m_subsurfaces.append(subsurface); + emit subsurfaceCreated(subsurface); +} + void Output::sendGeometry() { const auto resources = resourceMap().values(); @@ -628,6 +648,13 @@ WlShell::WlShell(CoreCompositor *compositor, int version) void WlShell::shell_get_shell_surface(Resource *resource, uint32_t id, wl_resource *surface) { auto *s = fromResource(surface); + if (!s->m_role) { + s->m_role = new WlShellSurfaceRole; + } else if (!qobject_cast(s->m_role)) { + QFAIL(QByteArrayLiteral("surface already has role") + s->m_role->metaObject()->className()); + return; + } + auto *wlShellSurface = new WlShellSurface(this, resource->client(), id, s); m_wlShellSurfaces << wlShellSurface; emit wlShellSurfaceCreated(wlShellSurface); diff --git a/tests/auto/wayland/shared/coreprotocol.h b/tests/auto/wayland/shared/coreprotocol.h index 4b06b5b538e..127ff0964b2 100644 --- a/tests/auto/wayland/shared/coreprotocol.h +++ b/tests/auto/wayland/shared/coreprotocol.h @@ -188,6 +188,11 @@ protected: void shell_get_shell_surface(Resource *resource, uint32_t id, ::wl_resource *surface) override; }; +class WlShellSurfaceRole : public SurfaceRole +{ + Q_OBJECT +}; + class WlShellSurface : public QObject, public QtWaylandServer::wl_shell_surface { Q_OBJECT @@ -203,15 +208,7 @@ public: Surface *m_surface = nullptr; }; -class Subsurface : public QObject, public QtWaylandServer::wl_subsurface -{ - Q_OBJECT -public: - explicit Subsurface(wl_client *client, int id, int version) - : QtWaylandServer::wl_subsurface(client, id, version) - { - } -}; +class Subsurface; class SubCompositor : public Global, public QtWaylandServer::wl_subcompositor { @@ -226,14 +223,38 @@ signals: void subsurfaceCreated(Subsurface *subsurface); protected: - void subcompositor_get_subsurface(Resource *resource, uint32_t id, ::wl_resource *surface, ::wl_resource *parent) override + void subcompositor_get_subsurface(Resource *resource, uint32_t id, + ::wl_resource *surfaceResource, + ::wl_resource *parent) override; +}; + +class SubSurfaceRole : public SurfaceRole +{ + Q_OBJECT +}; + +class Subsurface : public QObject, public QtWaylandServer::wl_subsurface +{ + Q_OBJECT +public: + explicit Subsurface(SubCompositor *subCompositor, wl_client *client, int id, int version) + : QtWaylandServer::wl_subsurface(client, id, version), m_subcompositor(subCompositor) { - QTRY_VERIFY(parent); - QTRY_VERIFY(surface); - auto *subsurface = new Subsurface(resource->client(), id, resource->version()); - m_subsurfaces.append(subsurface); // TODO: clean up? - emit subsurfaceCreated(subsurface); } + ~Subsurface() + { + m_subcompositor->m_subsurfaces.removeOne(this); + qDebug() << m_subcompositor->m_subsurfaces; + } + void subsurface_destroy(Resource *resource) override { wl_resource_destroy(resource->handle); } + void subsurface_destroy_resource(Resource *resource) override + { + Q_UNUSED(resource) + delete this; + } + +private: + SubCompositor *m_subcompositor; }; struct OutputMode { diff --git a/tests/auto/wayland/shared/xdgshell.cpp b/tests/auto/wayland/shared/xdgshell.cpp index 184522765b2..2c9d61ff510 100644 --- a/tests/auto/wayland/shared/xdgshell.cpp +++ b/tests/auto/wayland/shared/xdgshell.cpp @@ -99,6 +99,12 @@ void XdgSurface::xdg_surface_get_toplevel(Resource *resource, uint32_t id) { QVERIFY(!m_toplevel); QVERIFY(!m_popup); + if (!m_surface->m_role) { + m_surface->m_role = new XdgToplevelRole; + } else if (!qobject_cast(m_surface->m_role)) { + QFAIL(QByteArrayLiteral("surface already has role") + m_surface->m_role->metaObject()->className()); + return; + } m_toplevel = new XdgToplevel(this, id, resource->version()); emit toplevelCreated(m_toplevel); } @@ -108,6 +114,12 @@ void XdgSurface::xdg_surface_get_popup(Resource *resource, uint32_t id, wl_resou Q_UNUSED(positioner); QVERIFY(!m_toplevel); QVERIFY(!m_popup); + if (!m_surface->m_role) { + m_surface->m_role = new SubSurfaceRole; + } else if (!qobject_cast(m_surface->m_role)) { + qWarning() << "surface already has role" << m_surface->m_role->metaObject()->className(); + return; + } auto *p = fromResource(parent); m_popup = new XdgPopup(this, p, id, resource->version()); } diff --git a/tests/auto/wayland/shared/xdgshell.h b/tests/auto/wayland/shared/xdgshell.h index 3959e0668a8..7eab8028e80 100644 --- a/tests/auto/wayland/shared/xdgshell.h +++ b/tests/auto/wayland/shared/xdgshell.h @@ -85,6 +85,11 @@ protected: void xdg_surface_ack_configure(Resource *resource, uint32_t serial) override; }; +class XdgToplevelRole : public SurfaceRole +{ + Q_OBJECT +}; + class XdgToplevel : public QObject, public QtWaylandServer::xdg_toplevel { Q_OBJECT @@ -106,6 +111,11 @@ protected: void xdg_toplevel_set_min_size(Resource *resource, int32_t width, int32_t height) override; }; +class XdgPopupRole : public SurfaceRole +{ + Q_OBJECT +}; + class XdgPopup : public QObject, public QtWaylandServer::xdg_popup { Q_OBJECT diff --git a/tests/auto/wayland/surface/tst_surface.cpp b/tests/auto/wayland/surface/tst_surface.cpp index 9fe4188ef81..e5512faad06 100644 --- a/tests/auto/wayland/surface/tst_surface.cpp +++ b/tests/auto/wayland/surface/tst_surface.cpp @@ -26,6 +26,7 @@ private slots: // Subsurfaces void createSubsurface(); void createSubsurfaceForHiddenParent(); + void changeToSubsurface(); }; tst_surface::tst_surface() @@ -215,5 +216,32 @@ void tst_surface::createSubsurfaceForHiddenParent() QVERIFY(subsurface); } +void tst_surface::changeToSubsurface() +{ + QRasterWindow window1; + window1.resize(64, 64); + window1.show(); + + QRasterWindow window2; + window2.resize(64, 64); + window2.show(); + + window2.setParent(&window1); + QCOMPOSITOR_TRY_VERIFY(subSurface()); + + window2.setParent(nullptr); + QCOMPOSITOR_TRY_VERIFY(xdgToplevel(1)); + + window2.hide(); + window2.setParent(&window1); + window2.show(); + QCOMPOSITOR_TRY_VERIFY(subSurface()); + + window2.hide(); + window2.setParent(nullptr); + window2.show(); + QCOMPOSITOR_TRY_VERIFY(xdgToplevel(1)); +} + QCOMPOSITOR_TEST_MAIN(tst_surface) #include "tst_surface.moc"