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 <davidedmundson@kde.org>

src/plugins/platforms/wayland/qwaylandwindow.cpp

@@ -354,22 +354,23 @@ WId QWaylandWindow::winId() const
 
 void QWaylandWindow::setParent(const QPlatformWindow *parent)
 {
-    if (!window()->isVisible())
+    if (lastParent == parent)
-        return;
-
-    QWaylandWindow *oldparent = mSubSurfaceWindow ? mSubSurfaceWindow->parent() : nullptr;
-    if (oldparent == parent)
         return;
 
     if (mSubSurfaceWindow && parent) { // new parent, but we were a subsurface already
         delete mSubSurfaceWindow;
         QWaylandWindow *p = const_cast<QWaylandWindow *>(static_cast<const QWaylandWindow *>(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();
+        initializeWlSurface();
+        if (window()->isVisible()) {
             initWindow();
         }
     }
+    lastParent = parent;
+}
 
 QString QWaylandWindow::windowTitle() const
 {

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;

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<Surface>(surfaceResource);
+    if (!surface->m_role) {
+        surface->m_role = new SubSurfaceRole;
+    } else if (!qobject_cast<SubSurfaceRole *>(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>(surface);
+    if (!s->m_role) {
+        s->m_role = new WlShellSurfaceRole;
+    } else if (!qobject_cast<WlShellSurfaceRole *>(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);

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
+class Subsurface;
-{
-    Q_OBJECT
-public:
-    explicit Subsurface(wl_client *client, int id, int version)
-        : QtWaylandServer::wl_subsurface(client, id, version)
-    {
-    }
-};
 
 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 {

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<XdgToplevelRole *>(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<SubSurfaceRole *>(m_surface->m_role)) {
+        qWarning() << "surface already has role" << m_surface->m_role->metaObject()->className();
+        return;
+    }
     auto *p = fromResource<XdgSurface>(parent);
     m_popup = new XdgPopup(this, p, id, resource->version());
 }

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

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"