Raise window group together with group transient
AbandonedPublic

Authored by zzag on Oct 15 2018, 5:26 PM.

Details

Reviewers
graesslin
Group Reviewers
KWin
Summary

Group transients and "ordinary" transients behave differently with respect
to activation:

  • If an ordinary transient is activated, its main windows will be raised with it;
  • If a group transient is activated, only it will be raised, its window group won't be raised.

That difference exists because transientFor method is meaningless for
group transients, i.e. it returns nullptr.

This change addresses the problem stated above by using an appropriate
method to get a list of main clients.

lowerClient will be addressed in another patch.

Diff Detail

Repository
R108 KWin
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4507
Build 4525: arc lint + arc unit
zzag created this revision.Oct 15 2018, 5:26 PM
Restricted Application added a project: KWin. · View Herald TranscriptOct 15 2018, 5:26 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Oct 15 2018, 5:26 PM
zzag updated this revision to Diff 43675.Oct 15 2018, 5:27 PM

Edit comments

zzag updated this revision to Diff 43742.Oct 16 2018, 4:04 PM

Rebase.

Sure, that's the technical difference, but that doesn't indicate whether it's intended or not.

It was done explicitly in 27f7e7bd63d1d3b5873043897752a1937bce5ae5

zzag added a comment.EditedOct 31 2018, 5:03 PM

For tracking: https://bugs.kde.org/show_bug.cgi?id=199910


I don't completely agree with the proposed solution (in the bug report and what we have right now).

If a transient acts as a transient for a window in a window group, then, sure, we shouldn't raise the window group.

...but, if the transient acts as a transient for the whole window group, then we should raise the window group. Otherwise, what's the point of group transients in KWin?

Client's mainClients() implementation fulfills those requirements. https://phabricator.kde.org/source/kwin/browse/master/group.cpp$739-753

Also, maybe I should have used allMainClients().

zzag updated this revision to Diff 44578.Oct 31 2018, 9:04 PM

Use allMainClients

then we should raise the window group.

Why?

zzag added a comment.Nov 1 2018, 9:41 AM

Why?

Because we do the same for "ordinary" transients.

I'm not 100% why we raise main windows when a transient is raised, but it looks like that's a common practice. Maybe it has something to do with providing context, e.g. if you raise a confirmation dialog, then you probably would like to have a look at the main window to make decisions (whether to click "OK", "Cancel", etc).

Because we do the same for "ordinary" transients.

That's not a justification for why they should it should be treated the same.
One is an explicitly set child->parent relationship by the app, the other isn't.

I'm not opposed, but I'm not convinced either. Maybe @graesslin can weigh in here.

graesslin requested changes to this revision.Nov 3 2018, 8:47 AM

I'm pretty sure this was intended behavior. The behavior probably exists much longer than I was involved. I assume it was either Lubos or Matthias adding it. Changing code like this has probably more side effects than one can expect. If such a change gets added it must be tested with applications using the window group functionality and it must be tested that applications where one would not expect it to be groups behave correctly. E.g. are multiple konsole/kwrite windows in a group? How does gimp work with the change. I just tested with kwrite and there it is that two kwrite windows have the same group leader. I'm quite certain that you do not want to raise them together.

The problem is that group does not say that the windows belong together. It's more like a place where we can cache window properties (e.g. the icons). But they are not transients, so there is no parent-child relationship. It's more like step-siblings.

This revision now requires changes to proceed.Nov 3 2018, 8:47 AM
zzag added a comment.Nov 3 2018, 10:35 AM

How does gimp work with the change.

I tested it before, didn't notice any differences.

I just tested with kwrite and there it is that two kwrite windows have the same group leader. I'm quite certain that you do not want to raise them together.

If you raise a transient, for example "Open File" dialog, the window group won't be raised.
Most apps don't use group transients.

Anyway, I should have probably added a test for such case.

zzag updated this revision to Diff 44762.Nov 3 2018, 12:07 PM

Add test

In D16228#353226, @zzag wrote:

How does gimp work with the change.

I tested it before, didn't notice any differences.

I just tested with kwrite and there it is that two kwrite windows have the same group leader. I'm quite certain that you do not want to raise them together.

If you raise a transient, for example "Open File" dialog, the window group won't be raised.
Most apps don't use group transients.

Well, that's an application bug. KWin in general does not work around application bugs. Instead we try (whenever possible) to get the misbehaving application toolkit fixdd. An open file dialog should use transient for hint.

zzag added a comment.Nov 3 2018, 1:38 PM

An open file dialog should use transient for hint.

My comment was misleading. The open file dialog is a transient, but not group transient. :-)

In D16228#353303, @zzag wrote:

An open file dialog should use transient for hint.

My comment was misleading. The open file dialog is a transient, but not group transient. :-)

I think I don't really understand what you mean with "group transient". I know transients and groups, but what is a group transient to you?

zzag added a comment.Nov 3 2018, 4:11 PM

I think I don't really understand what you mean with "group transient". I know transients and groups, but what is a group transient to you?

Transient for its window group, i.e. a client that has root window in WM_TRANSIENT_FOR.

I follow KWin's terminology: https://phabricator.kde.org/source/kwin/browse/master/client.h$96. If that's wrong, please correct me.

Ok, this is code which is ancient and something I have never even looked into. So I did some research and found in EWMH:

_NET_WM_STATE_MODAL indicates that this is a modal dialog box. If the WM_TRANSIENT_FOR hint is set to another toplevel window, the dialog is modal for that window; if WM_TRANSIENT_FOR is not set or set to the root window the dialog is modal for its window group.

and:

Implementing enhanced support for application transient windows

If the WM_TRANSIENT_FOR property is set to None or Root window, the window should be treated as a transient for all other windows in the same group. It has been noted that this is a slight ICCCM violation, but as this behavior is pretty
standard for many toolkits and window managers, and is extremely unlikely to break anything, it seems reasonable to document it as standard.

So for all I wrote above I thought we were talking about groups.

Now I have a question: what is a real-world example for a group transient?

zzag added a comment.EditedNov 4 2018, 10:19 AM

Now I have a question: what is a real-world example for a group transient?

Relevant mailing list thread:

As an example, each desktop effect KCM dialog is a group transient (probably, because we don't set parent).

This patch tries to put "group transients" on par with transients for toplevel windows. If user raises a transient for a toplevel window, its main windows will be raised too. Why should it be different case for "group transients"?

In D16228#353813, @zzag wrote:

Now I have a question: what is a real-world example for a group transient?

Relevant mailing list thread:

No they are not. They don't have a transient parent, it's just that the EWMH evaluates that to be group transients. But that's in that case wrong. It's just a window without a parent.

This patch tries to put "group transients" on par with transients for toplevel windows. If user raises a transient for a toplevel window, its main windows will be raised too. Why should it be different case for "group transients"?

I would say: don't fix what is not broken. I'm afraid of side effects if we change such old code. I'm probably not the only one who hasn't known that transient for has such a behavior if set to root and I maintained a large window manager for almost a decade. Chances are that if we change the code it will cause regressions as applications break.

If you present me a real world use case where this feature is used and our current behavior is broken I might consider such a change. But without a real world use case I would say it's an obscure feature of ewmh which nobody uses any more

zzag added a comment.EditedNov 4 2018, 2:04 PM

I would say: don't fix what is not broken.

Well, no, it's broken. Why do we allow to have the following sandwich with a group transient but not with a transient for a toplevel window?

Conceptually, the only difference between the two is that the former can serve as a transient for more than one window. But in most cases, there would be only one main window in the group.

In D16228#353941, @zzag wrote:

I would say: don't fix what is not broken.

Well, no, it's broken. Why do we allow to have the following sandwich with a group transient but not with a transient for a toplevel window?

It's a bug in the effects kcm. It should be a transient to systemsettings. It was not the intention of the authors to make them group transients, but transients. Don't workaround bugs, fix the real ones :-)

That's my point: like in the effects kcm case I doubt the group transients are the intention of the authors. It's rather an unwanted side effect of API missusage.

zzag added a comment.EditedNov 5 2018, 3:20 PM

That's my point: like in the effects kcm case I doubt the group transients are the intention of the authors.

Probably...

But it doesn't change the fact that "group transients" are still transients. Why do we have to treat group transients differently from transients for toplevel windows?

zzag added a comment.Nov 16 2018, 4:27 PM

Hey, everyone, :-)

Could we please revisit this patch? Why shouldn't we raise group members together with group transient? If that's indeed a bad idea, I would like to abandon this patch.

zzag abandoned this revision.Jan 31 2019, 9:41 PM