Keep Deleted transients above old parents

Summary:
If a modal window is closed, usually, it will go behind its parent. The
reason for this is that Workspace::constrainedStackingOrder() puts only
AbstractClient transients above parents, not Deleted transients.

So, if fade/glide/scale effect animates the disappearing of a transient,
unfortunately, one can't see that animation.

BUG: 397448
FIXED-IN: 5.15.0

Test Plan:
=== Closing of a transient and parent window

Before:

https://www.youtube.com/watch?v=XiLq7EAVCp0

After:

https://www.youtube.com/watch?v=cH_Ki-sqY8M

=== Scale effect

Before:

https://www.youtube.com/watch?v=Eb2a3U7R10I

After:

https://www.youtube.com/watch?v=4AKu3fdrnYQ

=== Sheet effect

Before:

https://www.youtube.com/watch?v=xPPSnR5FUU0

After:

https://www.youtube.com/watch?v=o_hxTNT-5Hg

=== Popup menus on Wayland

Before:

https://www.youtube.com/watch?v=5DnrY8p3F5A

After:

https://www.youtube.com/watch?v=7XEo8n_CrCc

Reviewers: #kwin, davidedmundson

Reviewed By: #kwin, davidedmundson

Subscribers: abetts, davidedmundson, kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D14868

autotests/integration/stacking_order_test.cpp

@@ -23,6 +23,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "abstract_client.h"
 #include "atoms.h"
 #include "client.h"
+#include "deleted.h"
 #include "main.h"
 #include "platform.h"
 #include "shell_client.h"
@@ -51,9 +52,11 @@ private Q_SLOTS:
 
     void testTransientIsAboveParent();
     void testRaiseTransient();
+    void testDeletedTransient();
 
     void testGroupTransientIsAboveWindowGroup();
     void testRaiseGroupTransient();
+    void testDeletedGroupTransient();
     void testDontKeepAboveNonModalDialogGroupTransients();
 
 };
@@ -61,6 +64,7 @@ private Q_SLOTS:
 void StackingOrderTest::initTestCase()
 {
     qRegisterMetaType<KWin::AbstractClient *>();
+    qRegisterMetaType<KWin::Deleted *>();
     qRegisterMetaType<KWin::ShellClient *>();
 
     QSignalSpy workspaceCreatedSpy(kwinApp(), &Application::workspaceCreated);
@@ -202,6 +206,98 @@ void StackingOrderTest::testRaiseTransient()
     QCOMPARE(workspace()->stackingOrder(), (ToplevelList{anotherClient, parent, transient}));
 }
 
+struct WindowUnrefDeleter
+{
+    static inline void cleanup(Deleted *d) {
+        if (d != nullptr) {
+            d->unrefWindow();
+        }
+    }
+};
+
+void StackingOrderTest::testDeletedTransient()
+{
+    // This test verifies that deleted transients are kept above their
+    // old parents.
+
+    // Create the parent.
+    KWayland::Client::Surface *parentSurface =
+        Test::createSurface(Test::waylandCompositor());
+    QVERIFY(parentSurface);
+    KWayland::Client::ShellSurface *parentShellSurface =
+        Test::createShellSurface(parentSurface, parentSurface);
+    QVERIFY(parentShellSurface);
+    ShellClient *parent = Test::renderAndWaitForShown(parentSurface, QSize(256, 256), Qt::blue);
+    QVERIFY(parent);
+    QVERIFY(parent->isActive());
+    QVERIFY(!parent->isTransient());
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{parent}));
+
+    // Create the first transient.
+    KWayland::Client::Surface *transient1Surface =
+        Test::createSurface(Test::waylandCompositor());
+    QVERIFY(transient1Surface);
+    KWayland::Client::ShellSurface *transient1ShellSurface =
+        Test::createShellSurface(transient1Surface, transient1Surface);
+    QVERIFY(transient1ShellSurface);
+    transient1ShellSurface->setTransient(parentSurface, QPoint(0, 0));
+    ShellClient *transient1 = Test::renderAndWaitForShown(
+        transient1Surface, QSize(128, 128), Qt::red);
+    QVERIFY(transient1);
+    QTRY_VERIFY(transient1->isActive());
+    QVERIFY(transient1->isTransient());
+    QCOMPARE(transient1->transientFor(), parent);
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{parent, transient1}));
+
+    // Create the second transient.
+    KWayland::Client::Surface *transient2Surface =
+        Test::createSurface(Test::waylandCompositor());
+    QVERIFY(transient2Surface);
+    KWayland::Client::ShellSurface *transient2ShellSurface =
+        Test::createShellSurface(transient2Surface, transient2Surface);
+    QVERIFY(transient2ShellSurface);
+    transient2ShellSurface->setTransient(transient1Surface, QPoint(0, 0));
+    ShellClient *transient2 = Test::renderAndWaitForShown(
+        transient2Surface, QSize(128, 128), Qt::red);
+    QVERIFY(transient2);
+    QTRY_VERIFY(transient2->isActive());
+    QVERIFY(transient2->isTransient());
+    QCOMPARE(transient2->transientFor(), transient1);
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{parent, transient1, transient2}));
+
+    // Activate the parent, both transients have to be above it.
+    workspace()->activateClient(parent);
+    QTRY_VERIFY(parent->isActive());
+    QTRY_VERIFY(!transient1->isActive());
+    QTRY_VERIFY(!transient2->isActive());
+
+    // Close the top-most transient.
+    connect(transient2, &ShellClient::windowClosed, this,
+        [](Toplevel *toplevel, Deleted *deleted) {
+            Q_UNUSED(toplevel)
+            deleted->refWindow();
+        }
+    );
+
+    QSignalSpy windowClosedSpy(transient2, &ShellClient::windowClosed);
+    QVERIFY(windowClosedSpy.isValid());
+    delete transient2ShellSurface;
+    delete transient2Surface;
+    QVERIFY(windowClosedSpy.wait());
+
+    QScopedPointer<Deleted, WindowUnrefDeleter> deletedTransient(
+        windowClosedSpy.first().at(1).value<Deleted *>());
+    QVERIFY(deletedTransient.data());
+
+    // The deleted transient still has to be above its old parent (transient1).
+    QTRY_VERIFY(parent->isActive());
+    QTRY_VERIFY(!transient1->isActive());
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{parent, transient1, deletedTransient.data()}));
+}
+
 static xcb_window_t createGroupWindow(xcb_connection_t *conn,
                                       const QRect &geometry,
                                       xcb_window_t leaderWid = XCB_WINDOW_NONE)
@@ -501,6 +597,127 @@ void StackingOrderTest::testRaiseGroupTransient()
     QCOMPARE(workspace()->stackingOrder(), (ToplevelList{member1, leader, member2, anotherClient, transient}));
 }
 
+void StackingOrderTest::testDeletedGroupTransient()
+{
+    // This test verifies that deleted group transients are kept above their
+    // old window groups.
+
+    const QRect geometry = QRect(0, 0, 128, 128);
+
+    QScopedPointer<xcb_connection_t, XcbConnectionDeleter> conn(
+        xcb_connect(nullptr, nullptr));
+
+    QSignalSpy windowCreatedSpy(workspace(), &Workspace::clientAdded);
+    QVERIFY(windowCreatedSpy.isValid());
+
+    // Create the group leader.
+    xcb_window_t leaderWid = createGroupWindow(conn.data(), geometry);
+    xcb_map_window(conn.data(), leaderWid);
+    xcb_flush(conn.data());
+
+    QVERIFY(windowCreatedSpy.wait());
+    Client *leader = windowCreatedSpy.first().first().value<Client *>();
+    QVERIFY(leader);
+    QVERIFY(leader->isActive());
+    QCOMPARE(leader->windowId(), leaderWid);
+    QVERIFY(!leader->isTransient());
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{leader}));
+
+    // Create another group member.
+    windowCreatedSpy.clear();
+    xcb_window_t member1Wid = createGroupWindow(conn.data(), geometry, leaderWid);
+    xcb_map_window(conn.data(), member1Wid);
+    xcb_flush(conn.data());
+
+    QVERIFY(windowCreatedSpy.wait());
+    Client *member1 = windowCreatedSpy.first().first().value<Client *>();
+    QVERIFY(member1);
+    QVERIFY(member1->isActive());
+    QCOMPARE(member1->windowId(), member1Wid);
+    QCOMPARE(member1->group(), leader->group());
+    QVERIFY(!member1->isTransient());
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{leader, member1}));
+
+    // Create yet another group member.
+    windowCreatedSpy.clear();
+    xcb_window_t member2Wid = createGroupWindow(conn.data(), geometry, leaderWid);
+    xcb_map_window(conn.data(), member2Wid);
+    xcb_flush(conn.data());
+
+    QVERIFY(windowCreatedSpy.wait());
+    Client *member2 = windowCreatedSpy.first().first().value<Client *>();
+    QVERIFY(member2);
+    QVERIFY(member2->isActive());
+    QCOMPARE(member2->windowId(), member2Wid);
+    QCOMPARE(member2->group(), leader->group());
+    QVERIFY(!member2->isTransient());
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{leader, member1, member2}));
+
+    // Create a group transient.
+    windowCreatedSpy.clear();
+    xcb_window_t transientWid = createGroupWindow(conn.data(), geometry, leaderWid);
+    xcb_icccm_set_wm_transient_for(conn.data(), transientWid, rootWindow());
+
+    // Currently, we have some weird bug workaround: if a group transient
+    // is a non-modal dialog, then it won't be kept above its window group.
+    // We need to explicitly specify window type, otherwise the window type
+    // will be deduced to _NET_WM_WINDOW_TYPE_DIALOG because we set transient
+    // for before (the EWMH spec says to do that).
+    xcb_atom_t net_wm_window_type = Xcb::Atom(
+        QByteArrayLiteral("_NET_WM_WINDOW_TYPE"), false, conn.data());
+    xcb_atom_t net_wm_window_type_normal = Xcb::Atom(
+        QByteArrayLiteral("_NET_WM_WINDOW_TYPE_NORMAL"), false, conn.data());
+    xcb_change_property(
+        conn.data(),               // c
+        XCB_PROP_MODE_REPLACE,     // mode
+        transientWid,              // window
+        net_wm_window_type,        // property
+        XCB_ATOM_ATOM,             // type
+        32,                        // format
+        1,                         // data_len
+        &net_wm_window_type_normal // data
+    );
+
+    xcb_map_window(conn.data(), transientWid);
+    xcb_flush(conn.data());
+
+    QVERIFY(windowCreatedSpy.wait());
+    Client *transient = windowCreatedSpy.first().first().value<Client *>();
+    QVERIFY(transient);
+    QVERIFY(transient->isActive());
+    QCOMPARE(transient->windowId(), transientWid);
+    QCOMPARE(transient->group(), leader->group());
+    QVERIFY(transient->isTransient());
+    QVERIFY(transient->groupTransient());
+    QVERIFY(!transient->isDialog()); // See above why
+
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{leader, member1, member2, transient}));
+
+    // Unmap the transient.
+    connect(transient, &Client::windowClosed, this,
+        [](Toplevel *toplevel, Deleted *deleted) {
+            Q_UNUSED(toplevel)
+            deleted->refWindow();
+        }
+    );
+
+    QSignalSpy windowClosedSpy(transient, &Client::windowClosed);
+    QVERIFY(windowClosedSpy.isValid());
+    xcb_unmap_window(conn.data(), transientWid);
+    xcb_flush(conn.data());
+    QVERIFY(windowClosedSpy.wait());
+
+    QScopedPointer<Deleted, WindowUnrefDeleter> deletedTransient(
+        windowClosedSpy.first().at(1).value<Deleted *>());
+    QVERIFY(deletedTransient.data());
+
+    // The transient has to be above each member of the window group.
+    QCOMPARE(workspace()->stackingOrder(), (ToplevelList{leader, member1, member2, deletedTransient.data()}));
+}
+
 void StackingOrderTest::testDontKeepAboveNonModalDialogGroupTransients()
 {
     // Bug 76026

deleted.cpp

@@ -22,8 +22,10 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 #include "workspace.h"
 #include "client.h"
+#include "group.h"
 #include "netinfo.h"
 #include "shadow.h"
+#include "shell_client.h"
 #include "decorations/decoratedclient.h"
 #include "decorations/decorationrenderer.h"
 
@@ -46,6 +48,10 @@ Deleted::Deleted()
     , m_fullscreen(false)
     , m_keepAbove(false)
     , m_keepBelow(false)
+    , m_wasActive(false)
+    , m_wasX11Client(false)
+    , m_wasWaylandClient(false)
+    , m_wasGroupTransient(false)
 {
 }
 
@@ -57,6 +63,14 @@ Deleted::~Deleted()
     if (workspace()) {
         workspace()->removeDeleted(this);
     }
+    for (Toplevel *toplevel : qAsConst(m_transientFor)) {
+        if (auto *deleted = qobject_cast<Deleted *>(toplevel)) {
+            deleted->removeTransient(this);
+        }
+    }
+    for (Deleted *transient : qAsConst(m_transients)) {
+        transient->removeTransientFor(this);
+    }
     deleteEffectWindow();
 }
 
@@ -117,7 +131,29 @@ void Deleted::copyToDeleted(Toplevel* c)
         m_keepAbove = client->keepAbove();
         m_keepBelow = client->keepBelow();
         m_caption = client->caption();
+
+        m_wasActive = client->isActive();
+
+        const auto *x11Client = qobject_cast<Client *>(client);
+        m_wasGroupTransient = x11Client && x11Client->groupTransient();
+
+        if (m_wasGroupTransient) {
+            const auto members = x11Client->group()->members();
+            for (Client *member : members) {
+                if (member != client) {
+                    addTransientFor(member);
+                }
+            }
+        } else {
+            AbstractClient *transientFor = client->transientFor();
+            if (transientFor != nullptr) {
+                addTransientFor(transientFor);
+            }
+        }
     }
+
+    m_wasWaylandClient = qobject_cast<ShellClient *>(c) != nullptr;
+    m_wasX11Client = !m_wasWaylandClient;
 }
 
 void Deleted::unrefWindow()
@@ -192,6 +228,22 @@ void Deleted::mainClientClosed(Toplevel *client)
         m_mainClients.removeAll(c);
 }
 
+void Deleted::transientForClosed(Toplevel *toplevel, Deleted *deleted)
+{
+    if (deleted == nullptr) {
+        m_transientFor.removeAll(toplevel);
+        return;
+    }
+
+    const int index = m_transientFor.indexOf(toplevel);
+    if (index == -1) {
+        return;
+    }
+
+    m_transientFor[index] = deleted;
+    deleted->addTransient(this);
+}
+
 xcb_window_t Deleted::frameId() const
 {
     return m_frame;
@@ -207,5 +259,26 @@ QByteArray Deleted::windowRole() const
     return m_windowRole;
 }
 
+void Deleted::addTransient(Deleted *transient)
+{
+    m_transients.append(transient);
+}
+
+void Deleted::removeTransient(Deleted *transient)
+{
+    m_transients.removeAll(transient);
+}
+
+void Deleted::addTransientFor(AbstractClient *parent)
+{
+    m_transientFor.append(parent);
+    connect(parent, &AbstractClient::windowClosed, this, &Deleted::transientForClosed);
+}
+
+void Deleted::removeTransientFor(Deleted *parent)
+{
+    m_transientFor.removeAll(parent);
+}
+
 } // namespace
 

deleted.h

@@ -104,14 +104,89 @@ public:
     QString caption() const {
         return m_caption;
     }
+
+    /**
+     *  Returns whether the client was active.
+     *
+     * @returns @c true if the client was active at the time when it was closed,
+     *   @c false otherwise
+     **/
+    bool wasActive() const {
+        return m_wasActive;
+    }
+
+    /**
+     * Returns whether this was an X11 client.
+     *
+     * @returns @c true if it was an X11 client, @c false otherwise.
+     **/
+    bool wasX11Client() const {
+        return m_wasX11Client;
+    }
+
+    /**
+     * Returns whether this was a Wayland client.
+     *
+     * @returns @c true if it was a Wayland client, @c false otherwise.
+     **/
+    bool wasWaylandClient() const {
+        return m_wasWaylandClient;
+    }
+
+    /**
+     * Returns whether the client was a transient.
+     *
+     * @returns @c true if it was a transient, @c false otherwise.
+     **/
+    bool wasTransient() const {
+        return !m_transientFor.isEmpty();
+    }
+
+    /**
+     * Returns whether the client was a group transient.
+     *
+     * @returns @c true if it was a group transient, @c false otherwise.
+     * @note This is relevant only for X11 clients.
+     **/
+    bool wasGroupTransient() const {
+        return m_wasGroupTransient;
+    }
+
+    /**
+     * Checks whether this client was a transient for given toplevel.
+     *
+     * @param toplevel Toplevel against which we are testing.
+     * @returns @c true if it was a transient for given toplevel, @c false otherwise.
+     **/
+    bool wasTransientFor(const Toplevel *toplevel) const {
+        return m_transientFor.contains(const_cast<Toplevel *>(toplevel));
+    }
+
+    /**
+     * Returns the list of transients.
+     *
+     * Because the window is Deleted, it can have only Deleted child transients.
+     **/
+    DeletedList transients() const {
+        return m_transients;
+    }
+
 protected:
     virtual void debug(QDebug& stream) const;
 private Q_SLOTS:
     void mainClientClosed(KWin::Toplevel *client);
+    void transientForClosed(Toplevel *toplevel, Deleted *deleted);
+
 private:
     Deleted();   // use create()
     void copyToDeleted(Toplevel* c);
     virtual ~Deleted(); // deleted only using unrefWindow()
+
+    void addTransient(Deleted *transient);
+    void removeTransient(Deleted *transient);
+    void addTransientFor(AbstractClient *parent);
+    void removeTransientFor(Deleted *parent);
+
     int delete_refcount;
     double window_opacity;
     int desk;
@@ -140,6 +215,12 @@ private:
     bool m_keepAbove;
     bool m_keepBelow;
     QString m_caption;
+    bool m_wasActive;
+    bool m_wasX11Client;
+    bool m_wasWaylandClient;
+    bool m_wasGroupTransient;
+    ToplevelList m_transientFor;
+    DeletedList m_transients;
 };
 
 inline void Deleted::refWindow()

layers.cpp

@@ -525,39 +525,86 @@ ToplevelList Workspace::constrainedStackingOrder()
         stacking += layer[ lay ];
     // now keep transients above their mainwindows
     // TODO this could(?) use some optimization
-    for (int i = stacking.size() - 1;
-            i >= 0;
-       ) {
-        AbstractClient *current = qobject_cast<AbstractClient*>(stacking[i]);
-        if (!current || !current->isTransient()) {
-            --i;
-            continue;
-        }
+    for (int i = stacking.size() - 1; i >= 0;) {
+        // Index of the main window for the current transient window.
         int i2 = -1;
-        // find topmost client this one is transient for
-        for (i2 = stacking.size() - 1; i2 >= 0; --i2) {
-            AbstractClient *c2 = qobject_cast<AbstractClient *>(stacking[i2]);
-            if (!c2) {
+
+        // If the current transient has "child" transients, we'd like to restart
+        // construction of the constrained stacking order from the position where
+        // the current transient will be moved.
+        bool hasTransients = false;
+
+        // Find topmost client this one is transient for.
+        if (auto *client = qobject_cast<AbstractClient *>(stacking[i])) {
+            if (!client->isTransient()) {
+                --i;
                 continue;
             }
-            if (c2 == current) {
-                i2 = -1; // Don't reorder, already on top of its main window.
-                break;
+            for (i2 = stacking.size() - 1; i2 >= 0; --i2) {
+                auto *c2 = qobject_cast<AbstractClient *>(stacking[i2]);
+                if (!c2) {
+                    continue;
+                }
+                if (c2 == client) {
+                    i2 = -1; // Don't reorder, already on top of its main window.
+                    break;
+                }
+                if (c2->hasTransient(client, true)
+                        && keepTransientAbove(c2, client)) {
+                    break;
+                }
             }
-            if (c2->hasTransient(current, true)
-                    && keepTransientAbove(c2, current)) {
-                break;
+
+            hasTransients = !client->transients().isEmpty();
+
+            // If the current transient doesn't have any "alive" transients, check
+            // whether it has deleted transients that have to be raised.
+            const bool searchForDeletedTransients = !hasTransients
+                && !deletedList().isEmpty();
+            if (searchForDeletedTransients) {
+                for (int j = i + 1; j < stacking.count(); ++j) {
+                    auto *deleted = qobject_cast<Deleted *>(stacking[j]);
+                    if (!deleted) {
+                        continue;
+                    }
+                    if (deleted->wasTransientFor(client)) {
+                        hasTransients = true;
+                        break;
+                    }
+                }
             }
+        } else if (auto *deleted = qobject_cast<Deleted *>(stacking[i])) {
+            if (!deleted->wasTransient()) {
+                --i;
+                continue;
+            }
+            for (i2 = stacking.size() - 1; i2 >= 0; --i2) {
+                Toplevel *c2 = stacking[i2];
+                if (c2 == deleted) {
+                    i2 = -1; // Don't reorder, already on top of its main window.
+                    break;
+                }
+                if (deleted->wasTransientFor(c2)
+                        && keepDeletedTransientAbove(c2, deleted)) {
+                    break;
+                }
+            }
+            hasTransients = !deleted->transients().isEmpty();
         }
+
         if (i2 == -1) {
             --i;
             continue;
         }
+
+        Toplevel *current = stacking[i];
+
         stacking.removeAt(i);
         --i; // move onto the next item (for next for () iteration)
         --i2; // adjust index of the mainwindow after the remove above
-        if (!current->transients().isEmpty())   // this one now can be possibly above its transients,
+        if (hasTransients) {  // this one now can be possibly above its transients,
             i = i2; // so go again higher in the stack order and possibly move those transients again
+        }
         ++i2; // insert after (on top of) the mainwindow, it's ok if it2 is now stacking.end()
         stacking.insert(i2, current);
     }
@@ -637,6 +684,40 @@ bool Workspace::keepTransientAbove(const AbstractClient* mainwindow, const Abstr
     return true;
 }
 
+bool Workspace::keepDeletedTransientAbove(const Toplevel *mainWindow, const Deleted *transient) const
+{
+    // #93832 - Don't keep splashscreens above dialogs.
+    if (transient->isSplash() && mainWindow->isDialog()) {
+        return false;
+    }
+
+    if (transient->wasX11Client()) {
+        // If a group transient was active, we should keep it above no matter
+        // what, because at the time when the transient was closed, it was above
+        // the main window.
+        if (transient->wasGroupTransient() && transient->wasActive()) {
+            return true;
+        }
+
+        // This is rather a hack for #76026. Don't keep non-modal dialogs above
+        // the mainwindow, but only if they're group transient (since only such
+        // dialogs have taskbar entry in Kicker). A proper way of doing this
+        // (both kwin and kicker) needs to be found.
+        if (transient->wasGroupTransient() && transient->isDialog()
+                && !transient->isModal()) {
+            return false;
+        }
+
+        // #63223 - Don't keep transients above docks, because the dock is kept
+        // high, and e.g. dialogs for them would be too high too.
+        if (mainWindow->isDock()) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 // Returns all windows in their stacking order on the root window.
 ToplevelList Workspace::xStackingOrder() const
 {

workspace.h

@@ -511,6 +511,7 @@ private:
     void lowerClientWithinApplication(AbstractClient* c);
     bool allowFullClientRaising(const AbstractClient* c, xcb_timestamp_t timestamp);
     bool keepTransientAbove(const AbstractClient* mainwindow, const AbstractClient* transient);
+    bool keepDeletedTransientAbove(const Toplevel *mainWindow, const Deleted *transient) const;
     void blockStackingUpdates(bool block);
     void updateToolWindows(bool also_hide);
     void fixPositionAfterCrash(xcb_window_t w, const xcb_get_geometry_reply_t *geom);