Keep Deleted transients above old parents
ClosedPublic

Authored by zzag on Aug 15 2018, 9:02 PM.

Details

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:

After:

Scale effect

Before:

After:

Sheet effect

Before:

After:

Popup menus on Wayland

Before:

After:

Diff Detail

Repository
R108 KWin
Branch
deleted-transients-stacking-order
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2126
Build 2144: arc lint + arc unit
zzag created this revision.Aug 15 2018, 9:02 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 15 2018, 9:02 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 15 2018, 9:02 PM
zzag updated this revision to Diff 39834.Aug 16 2018, 7:31 AM

Clean up in ~Deleted

zzag added a comment.Aug 16 2018, 5:30 PM

Not sure how to implement keepTransientAbove for deleted windows. Especially, how to deal with group transients on X11?

zzag added a comment.Aug 16 2018, 5:31 PM
This comment was removed by zzag.

Also, could someone please say whether that's a right way to solve the problem?

Not read the full code, but in general makes sense, the unconstrained stacking order contains deleted in the same place as the original, the constrained version should too.

Maybe you can simplify it with some new virtuals on the toplevel for isTransient/transientFor rather than having two code paths.

zzag added a comment.EditedAug 17 2018, 8:36 AM

Maybe you can simplify it with some new virtuals on the toplevel for isTransient/transientFor rather than having two code paths.

With was prefix, I wanted to emphasize that Deleted is not actually a transient. It was a transient for a parent window and we keep the deleted transient above the parent only because of effects. When the parent gets closed, the was part is questionable. But, in most cases, Toplevel returned from wasTransientFor() will be either a Client or ShellClient, so the was part is actually "valid", I guess.

Yeah, maybe it will simplify code, not sure (I'm still learning how KWin works)

zzag abandoned this revision.Aug 19 2018, 9:24 AM

I hit the wall with trying to figure out what to do with group transients.

As it is said in geometry.cpp, transientFor will return nullptr if a window is a transient for every window in its window group. So, the current implementation will keep them as they are right now, which is not quite good.

One possible solution is to copy members of a window group to which the transient belongs to. Also, because that's pretty X11 specific thing we maybe need to add some methods to distinguish ShellClients from Clients, Deleted::wasClient() is too generic. I don't like this solution.

Anyway, I'm done with it.

zzag added a comment.Aug 19 2018, 3:31 PM
In D14868#311383, @zzag wrote:

One possible solution is to copy members of a window group to which the transient belongs to. Also, because that's pretty X11 specific thing we maybe need to add some methods to distinguish ShellClients from Clients, Deleted::wasClient() is too generic. I don't like this solution.

As last resort, I've tried to implement this solution. See https://github.com/zzag/kwin/commit/a9fb51b871040f9fbe5f7f0a26e75f84b1f42d9f

Basically, relationship between transients and transientFor is M-2-M.

The search window(KMail) still goes below because of transient->isDialog() && !transient->isModal().

So, that solution is not good too.

zzag reclaimed this revision.Aug 19 2018, 3:36 PM

Maybe, this solution is not that bad.

zzag updated this revision to Diff 40248.Aug 22 2018, 7:38 PM

Clean up method names

zzag updated this revision to Diff 41829.Sep 17 2018, 1:15 PM

Support group transients

zzag added a comment.Sep 17 2018, 1:16 PM

That's pretty ugly solution, but somehow it works. :/

abetts added a subscriber: abetts.Sep 17 2018, 2:22 PM

+1 I love it! This is the kind of attention to detail and polish these animations need. Thanks for working on this zzag!

zzag abandoned this revision.Sep 22 2018, 10:25 AM
zzag reclaimed this revision.Oct 15 2018, 12:47 PM
zzag updated this revision to Diff 43727.Oct 16 2018, 11:41 AM

verbose var name

zzag planned changes to this revision.Oct 16 2018, 11:45 AM

Add explanatory comments.

zzag updated this revision to Diff 43730.Oct 16 2018, 12:33 PM

Comments

zzag retitled this revision from RFC: Keep Deleted transients above old parents to Keep Deleted transients above old parents.Oct 16 2018, 12:33 PM
zzag edited the summary of this revision. (Show Details)Oct 16 2018, 12:39 PM
zzag edited the test plan for this revision. (Show Details)
zzag edited the test plan for this revision. (Show Details)Oct 16 2018, 12:45 PM
zzag edited the test plan for this revision. (Show Details)
zzag updated this revision to Diff 43732.Oct 16 2018, 12:56 PM
zzag edited the test plan for this revision. (Show Details)

edit comments

davidedmundson accepted this revision.Oct 16 2018, 2:43 PM
davidedmundson added inline comments.
deleted.cpp
141

I'd expect to see that in Javascript, but it's weird in C++

This revision is now accepted and ready to land.Oct 16 2018, 2:43 PM
zzag updated this revision to Diff 43740.Oct 16 2018, 3:30 PM
  • Don't use !!;
  • Search for deleted transients in AbstractClient code path.
zzag edited the summary of this revision. (Show Details)Oct 16 2018, 3:31 PM
This revision was automatically updated to reflect the committed changes.