Keep fullscreen windows in active layer based on transients not the group
ClosedPublic

Authored by graesslin on Jan 6 2018, 10:31 AM.

Details

Summary

So far a not-active fullscreen X11 window was kept in the active layer if
the newly activated window is in the same group (that is same client
leader). For example a fullscreen X11 kwrite window is in the active layer
if another kwrite window is active. The two kwrite windows obviously
don't have anything to do with each other, but are in the same group.

This creates problems as it's not possible to raise other windows above
the active not-fullscreen kwrite window. E.g. the panel is stacked below.

The idea behind the check makes sense: if a fullscreen window opens
another window (e.g. a configuration dialog) it should not be put back
to normal layer. Thus the check is adjusted whether the new active
window is a transient to the fullscreen window. Thus the intention is
still the same, but does not cause the problems.

As the code now does not need to differentiate between X11 and Wayland
windows (group only on X11) the Client specific implementation is
removed and the method unvirtual'ed.

BUG: 388310
FIXED-IN: 5.12.0

Test Plan

Test passes

Diff Detail

Repository
R108 KWin
Branch
fullscreen-transient-not-group
Lint
No Linters Available
Unit
No Unit Test Coverage
graesslin created this revision.Jan 6 2018, 10:31 AM
Restricted Application added a project: KWin. · View Herald TranscriptJan 6 2018, 10:31 AM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
graesslin requested review of this revision.Jan 6 2018, 10:31 AM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 6 2018, 10:31 AM
romangg added a subscriber: romangg.Jan 6 2018, 1:51 PM

Are there maybe other cases of clients in X, which share the same group, are not transient, but need to have isActiveFullScreen return true on any client if one of them is the most recent activated one?

I mean you change the behavior now from testing on the group to only testing on the transient property, what seems to make sense. But what was the reason to set isActiveFullScreen to true for clients sharing the same group in the first place?

abstract_client.cpp
1155

Just a question for understanding unrelated to this patch: why should isActiveFullScreen return true, if ac->screen() != screen()? We don't know if it's really active just because it's on another screen than the one which the most recently activated client is on.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 6 2018, 1:51 PM

Are there maybe other cases of clients in X, which share the same group, are not transient, but need to have isActiveFullScreen return true on any client if one of them is the most recent activated one?

I don't think there are any cases. In fact this was anyway special KWin behavior. Have a look at https://standards.freedesktop.org/wm-spec/wm-spec-1.4.html and scroll all way down to "Stacking Order". It only talks about transients, not client leaders.

I mean you change the behavior now from testing on the group to only testing on the transient property, what seems to make sense. But what was the reason to set isActiveFullScreen to true for clients sharing the same group in the first place?

Let me try to git blame...

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 6 2018, 2:58 PM

git blame result: The group check was introduced with 476ca65295bfb3f0d90f535d9930250a13a8b323 - before it was a check for transient, but the group check was there as commented out code. The change to not use group members was commit 2d99ef918bbd797701ac63c8e3d4d7ca05f64651 based on BUG 293265.

I have a feeling we don't really know what we are doing.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 6 2018, 3:08 PM
graesslin added inline comments.Jan 6 2018, 3:10 PM
abstract_client.cpp
1155

We keep fullscreen windows in the active layer if the active window is on a different screen. Consider you have on each screen a panel. On your second screen you have vlc playing a fullscreen video. On the first screen you want to quickly check something in the browser. This ensures that vlc stays above of the panel all the time.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 6 2018, 3:10 PM

git blame result: The group check was introduced with 476ca65295bfb3f0d90f535d9930250a13a8b323 - before it was a check for transient, but the group check was there as commented out code. The change to not use group members was commit 2d99ef918bbd797701ac63c8e3d4d7ca05f64651 based on BUG 293265.

Wrong, the change to not use group members was commit 818070d3aa92df29081a07a2787e005b812ed60d (SVN 851204)

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 6 2018, 3:18 PM
romangg accepted this revision.Jan 6 2018, 6:44 PM

I've tested it with Kate and also multiple screens, and everything worked fine. On Wayland I couldn't really test it, because Kate can't be made fullscreen (with and without the patch). VLC fullscreen worked fine though, or let's say it this way: I couldn't put windows on top of it with and without the patch. :)

This revision is now accepted and ready to land.Jan 6 2018, 6:44 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 6 2018, 6:44 PM

The original implementation based the fullscreen status on the stack position of the window (ie. whenever a window would rise above the plain stack position of the FS window, it would loose the FS status, ie. top layer)
The result was iirc that random notifications would not only show up but also de-fullscreen the window and also virtual desktop switches would constantly kill the FS state.

The group/transient thing was iirc a "make this simpler" thing (assuming the group would be sufficient again, since the switch from stack => active killed the major issues and annoyances that the stack selection brought)

Notice that the new patch does not cover the case of layered transient windows (which iirc was an issue with dolphin at the time, but pls don't nail me on that)

The code in place is certainly wrong, but none of the linked commits actually removed the group check that was used in https://phabricator.kde.org/R108:476ca65295bfb3f0d90f535d9930250a13a8b323 (the other two commits predate that one)

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 6 2018, 7:37 PM
graesslin updated this revision to Diff 24870.Jan 7 2018, 8:33 AM

Check transients recursively by looking at the transients main windows

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 7 2018, 8:33 AM
This revision was automatically updated to reflect the committed changes.
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 10 2018, 4:44 PM