Replaced isDock check with !hasDecoration
Changes PlannedPublic

Authored by niccolove on Feb 16 2020, 5:44 PM.

Details

Reviewers
zzag
davidedmundson
Group Reviewers
KWin
Summary

Currently, the panel leaks in when blurring the background of dialogs near it. This results in a ungly visual glitch. The panel already ignores blur of elements near it; this patch extends that behaviour to everything that has not a decoration, to make sure that the panel does not leak in. The visual bug is in fact not there anymore, see:

Before:


Before:

After:

Diff Detail

Repository
R108 KWin
Branch
fixed_blur_near_panels (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22541
Build 22559: arc lint + arc unit
niccolove created this revision.Feb 16 2020, 5:44 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 16 2020, 5:44 PM
niccolove requested review of this revision.Feb 16 2020, 5:44 PM
niccolove edited the summary of this revision. (Show Details)Feb 16 2020, 5:46 PM
niccolove edited reviewers, added: KWin, zzag; removed: kwin.
Restricted Application added a subscriber: kwin. · View Herald TranscriptFeb 16 2020, 5:47 PM

Nice. Is this an alternative to all of D27122, or only one part of it?

+1 on the visuals.

davidedmundson requested changes to this revision.Feb 16 2020, 11:34 PM
davidedmundson added a subscriber: davidedmundson.

We need to find something more reliable than noDecoration, that includes so many other things.

This revision now requires changes to proceed.Feb 16 2020, 11:34 PM

We need to find something more reliable than noDecoration, that includes so many other things.

Do you have any tip on what could be more reliable?

@zzag wrote:

Using EffectWindow::noDecoration() instead of EffectWindow::isDock() doesn't seem right. I suggest to try to tackle the problem at its root. The issue is that the blur effect doesn't sample texels strictly below the window.

I'm afraid that I don't understand what you mean with "the blur effect doesn't sample texels strictly below the window".

zzag added a comment.Feb 17 2020, 2:02 PM

I'm afraid that I don't understand what you mean with "the blur effect doesn't sample texels strictly below the window".

Bad wording, sorry. The main problem with the blur effect right now is that pixels nearby the blur region may affect the final blur result. For example, if you place two windows side by side, then the window with the highest stacking order will blur some parts of the other window. Perhaps we need to blur pixels that are strictly inside the blur region. I'm not sure what performance impact it will have and how we should deal with drop-shadows, though.

In D27439#612897, @zzag wrote:

Bad wording, sorry. The main problem with the blur effect right now is that pixels nearby the blur region may affect the final blur result. For example, if you place two windows side by side, then the window with the highest stacking order will blur some parts of the other window. Perhaps we need to blur pixels that are strictly inside the blur region. I'm not sure what performance impact it will have and how we should deal with drop-shadows, though.

Only blurring elements directly underneath was my first though as well. This has a problem, though: when you move a blurred area on top of a busy area (e.g.: transparent konsole on top of telegram) the borders of the blurred area flicker. This is because elements gets from "does not even exist" to "blurred" as soon as they get in the blur area completely changing the colors of the pixel within the blur radius instantaneously. I can say that because the feature of "only blurring when directly underneath" is already implemented for docks. To use it for any window, it's enough to change line 689 from "isDock" to "true". Doing that, you will notice the above mentioned effect. What I propose in this patch is to only apply the "only blur when directly underneath" for anything that has not a decoration, so plasmoids+krunner+context menus as well. We can safely do that as those elements generally do not move (the flickering is not present in animations) and when you try to move a window they close. Furthermore, this only happens on very transparent themes. So what you propose is to do "only blur underneath" for everything, while this patch is "only blur underneath" exclusively for noDecoration - if you think that applying it for everything is fine, only applying it to noDecoration should be fine as well. I tried this patch in various conditions and the only problem that I could find is when you a) have a very transparent theme b) move windows underneath plasmoids/krunner using alt+drag, and even in that case the flickering is not that strong; in my opinion, the overall benefit for everyone is much bigger.
I hope this better explains the reasoning behind this patch. I'm not an expert at all with kwin so please tell me if I'm making invalid assumptions. That said, what I observed changing the code was consistent with what I wrote above.

I tried to make a video to show the flickering of turning it on for everything, but my PC apparently can't record my screen. Still, it might make the concept a bit more clear:

niccolove requested review of this revision.Mar 2 2020, 2:47 PM
zzag added a comment.EditedMar 12 2020, 4:28 PM

The problem is that windows without decorations are not special in any way so I don't think that the blur effect should work differently for them. As David pointed out, we need some other more "reliable" way to switch between blur modes. On the other hand, I would like to point out that we would still have the same problem. Ideally, if two windows are placed side by side, no pixels from one window should bleed to the other one. Perhaps we should tackle the problem at its roots rather than try to work around it. If we can't fix it, I personally would be okay to live with what we have now (having custom blur path for only dock windows).

Ideally, if two windows are placed side by side, no pixels from one window should bleed to the other one

I disagree here, because moving the window by just 1px (so that they overlap) would change the blur area a lot. If that happens every time you move a transparent area over a sharp border, you will get flickering (click on the rectangle, it's a gif):

problem at its roots

So I don't think there's a problem at the root. The fact is that moving windows should not blur ONLY underneath, because it would cause the visual glitches shown in the gif above when they are moved. My reasoning was "then, let's only apply the special blur to windows that can't be moved by the user" and in my mind, those were the windows without decorations. And that's why I though noDecoration windows were special: they can't be moved by the user, so they can get the special blur. That said: if you think that all moving windows should instead only blur underneath, then that's a very easy patch and I can do it, it would solve my problem. But it would cause the visual glitches shown in the gif above.

If you instead want me to find a better target than "no decoration" for the special blur, then I can find some better rules such as "only plasma windows" or "only plasma windows with no decorations" or "only windows that are touching but not overlapping a panel". Please feel free to tell me which way to go forward - I know you already said "tackle the problem at its roots", but I don't agree it's a problem since it would be instead wrong to make moving windows only blur underneath.

zzag added a comment.EditedMar 16 2020, 8:39 AM

Doing this hack for panels and plasma dialogs would be fine.

Thanks :-)
Since kwin does not consider plasmoids as dialogs but rather as normal windows, should I target those using w->windowClass() == QString("plasmashell plasmashell") && !w->hasDecoration()? It seems that I'm including just what I want to this way (panels, plasmoids and notifications). I could not find a more specific flag reading the documentation.

zzag added a comment.Mar 17 2020, 8:29 AM

It would be nice to have a generic way to detect plasma dialogs, so we don't need to hard code window classes. Please notice that checking whether the given wnidow belongs to the plasmashell is not enough since there are third party dock applications, e.g. latte-dock.

Frankly, I don't know what the best way to detect plasma dialogs is. Maybe @davidedmundson has an idea.

Frankly, I don't know what the best way to detect plasma dialogs is. Maybe @davidedmundson has an idea.

Apparently we don't want to do that. Notifications are plasma dialogs, and Veggero says they should be excluded.

If we want to find the panel and popups relating to the panel, our best bet is:

w->isDock || w->transientFor()->isDock()

That means exposing transientFor in EffectWindow, but that could be useful in general anyway.
That should work on Wayland too

niccolove updated this revision to Diff 80585.Apr 19 2020, 8:25 PM
niccolove edited the summary of this revision. (Show Details)

Expose transientfor

niccolove planned changes to this revision.Apr 19 2020, 8:26 PM

I added what I have done so far. Still doesn't work yet.