[x11] Emit clientRemoved after client was removed
ClosedPublic

Authored by zzag on Nov 21 2018, 10:56 AM.

Details

Summary

Currently, there is a guarantee that a client, which is about to be removed,
is no longer in the stacking order(both in constrained and unconstrained)
when Workspace::removeClient is called. However, because the client gets
removed from m_allClients after clientRemoved is emitted, it can be
re-inserted back into the stacking order.

In general, the pattern is to do some work and then notify others about
what you've done by emitting a signal. In the case of Workspace::removeClient,
we emit clientRemoved way before the client actually gets removed.

CCBUG: 392412
CCBUG: 400854

Test Plan
  • Enable the following script:
workspace.clientAdded.connect(function (client) {
    if (client.skipTaskbar || client.modal || client.transient) {
        return;
    }
    workspace.desktops = workspace.desktops + 1;
    workspace.currentDesktop = workspace.desktops;
    client.desktop = workspace.currentDesktop;
});

workspace.clientRemoved.connect(function (client) {
    if (client.skipTaskbar || client.modal || client.transient) {
        return;
    }
    workspace.desktops = workspace.desktops - 1;
});
  • Open an app, close the app.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Nov 21 2018, 10:56 AM
Restricted Application added a project: KWin. · View Herald TranscriptNov 21 2018, 10:56 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Nov 21 2018, 10:56 AM
zzag edited the test plan for this revision. (Show Details)Nov 21 2018, 10:58 AM

I'm wondering what the intention was to have it that early?

zzag added a comment.Nov 22 2018, 10:11 AM

I'm wondering what the intention was to have it that early?

Me too. That's quite unusual place to emit the signal (at least, a comment should have been added why).

I doubt there was some reasoning behind emitting clientRemoved that early. To me, it looks like it was added in a hurry.

graesslin accepted this revision.Nov 24 2018, 10:10 AM

Ok, just git blamed it. It was the scripting merge (previous version which has hardly anything to do with todays scripting) and that one had signals at a few stupid points. Also the signals were just added for scripting, so shouldn't be connected somewhere else.

This revision is now accepted and ready to land.Nov 24 2018, 10:10 AM
This revision was automatically updated to reflect the committed changes.