[effects/cubeslide] Fix visual glitches with Blur / BackgroundContrast effect
ClosedPublic

Authored by poboiko on Aug 30 2018, 9:29 PM.

Details

Summary

This patch fixes Bug 361516 and Bug 362360.

These bugs occurred because KWin was not very happy when windows were painted during CubeSlideEffect::paintScreen().
Another issue is that blur, although it was supposed to, did not work at all (haven't found appropriate bug on bugzilla). As well as background contrast effect.

This patch does the following thing:

  • Adopted WindowForceBlur / WindowForceBackgroundContrast logic from SlideEffect, instead of panels/stickyWindows QSets (those become useless anyway)
  • Added shouldAnimate code, which determines whether a window should be animated with the cube (i.e. ordinary windows) or should stick (i.e. panels or pinned windows, if corresponding options are checked in the settings)
  • It paints an additional non-transformed screen, on which it paints only "sticky" windows. This is done because otherwise KWin would apply blur not behind the OSD, but on the same place on moving cube face.
  • (in addition) switched to new Qt5 connect syntax.

BUG: 361516
BUG: 362360
FIXED-IN: 5.1X.X

Test Plan


(don't know if there is already a way to record a screen on wayland, so cannot post it).

Tested with all possible combinations of options both on X11 and Wayland, everything works

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
poboiko created this revision.Aug 30 2018, 9:29 PM
Restricted Application added a subscriber: kwin. · View Herald TranscriptAug 30 2018, 9:29 PM
poboiko requested review of this revision.Aug 30 2018, 9:29 PM
zzag requested changes to this revision.Aug 31 2018, 8:35 AM
zzag added a subscriber: zzag.

Wow! Thanks! I wasted hours trying to figure out what's wrong with this effect on Wayland and for some reason I hadn't noticed the isManaged check.

Minor nitpicks:

  • Please change the title to "[effects/cubeslide] ...."
  • Add BUG keywords
effects/cube/cubeslide.cpp
43

Please move it up, otherwise compiler won't be happy about reordering.

50

Please take a look at QOverload. http://doc.qt.io/qt-5/qtglobal.html#qOverload

QOverload<int, int, EffectWindow *>::of(&EffectsHandler::desktopChanged)

188–194

I think we should force roles at the beginning of the animation. And reset them at the end of the animation.

We should keep the number of memory allocations at the minimum.

410

Q_UNUSED(w)

522

Please use isActive instead.

525–532

No, that's wrong. During the cube slide animation windows are 3D transformed. Thus, that's wrong to blur background behind them.

We should force blur and background contrast only for sticky windows. Also, I think we don't need to keep track of window with forced roles. So, please delete m_forcedRoles.

543

Hm, what about renaming it to isStickyWindow?

548

It would animate notifications, desktop switch OSD, etc. What about

if (dontSlideStickyWindows && w->isOnAllDesktops()) {
    if (w->isDesktop()) {
        return true;
    }
    // other checks?
    return false;
}

Keep in mind, that's just a hypothesis.

556–571

Maybe, we should force blur and background contrast only for sticky windows.

effects/cube/cubeslide.h
75–76

Please prepend slot.

78

Unrelated whitespace change.

This revision now requires changes to proceed.Aug 31 2018, 8:35 AM

I don't use arcanist for commits, so I planned to add those keywords by hand afterwards.

effects/cube/cubeslide.cpp
188–194

Yeah, I though about it. It's just I wanted to avoid code duplication, since animation starts in several different places (i.e. when desktop changes, but also when user moves window through the border). I guess I'll make another function which does it (and maybe other common stuff)

525–532

I thought about it initially, but I found out that it actually works fine - it blurs background on the moving cube face for non-sticky windows.

Concerning m_forcedRoles - I added it so I can remove those roles afterwards. You're saying that I don't really have to do it?

543

Well, it also includes panels, but I can change it (probably, that semantics is even more obvious due to the context).

548

Oh. It actually might work, thanks! Will do the testing.

zzag added inline comments.Aug 31 2018, 11:13 AM
effects/cube/cubeslide.cpp
525–532

Yes, we don't need it. We'll force blur only for panels and sticky windows. Thus, m_forcedRoles will be redundant.

543

Ok, let's leave it as it is.

poboiko retitled this revision from [effects/cubeslide/wayland] Fix several cubeslide issues to [effects/cubeslide] Fix several cubeslide issues.Aug 31 2018, 8:39 PM
poboiko edited the summary of this revision. (Show Details)
poboiko updated this revision to Diff 40790.Aug 31 2018, 8:43 PM

OK, I've found a way to separate OSDs from other windows on Wayland (borrowed from FlipSwitchEffect, which doesn't animate OSDs, obviously) - I can check whether a window accepts focus or not. Also should have fixed other notes (do I work with WindowForceBlurRole correctly now? I'm not really sure how it works...)

poboiko marked 17 inline comments as done.Aug 31 2018, 8:44 PM

Oh, God. I'm so sorry. I was thinking about old cleanup code when I was typing comment about m_forcedRoles:

foreach (EffectWindow * w, panels)
w->setData(WindowForceBlurRole, QVariant(false));
foreach (EffectWindow * w, stickyWindows)
w->setData(WindowForceBlurRole, QVariant(false));
stickyWindows.clear();
panels.clear();

OK, so we don't really need m_forcedRoles(and panels, and stickyWindows) to track windows with forced roles. We could just reset forced roles for all windows when the animation is finished.
But, conceptually, it would be wrong. So, yeah, we need to store windows with forced roles somewhere.

Because we force blur and background contrast only for panels and sticky windows, I think we need only one QSet, e.g.

QSet<EffectWindow*> staticWindows; // panels + sticky windows

Then, when you force roles

void ...::startAnimation(...)
{
    const EffectWindowList windows = effects->stackingOrder();
    for (EffectWindow *w : windows) {
        if (shouldAnimate(w)) {
            continue;
        }
        w->setData(WindowForceBackgroundContrastRole, QVariant(true));
        w->setData(WindowForceBlurRole, QVariant(true));
        staticWindows.add(w);
    }
    ...
}

Cleanup

for (EffectWindow *w : staticWindows) {
    w->setData(WindowForceBackgroundContrastRole, QVariant());
    w->setData(WindowForceBlurRole, QVariant());
}
staticWindows.clear();

Again, sorry for the inconvenience.


do I work with WindowForceBlurRole correctly now? I'm not really sure how it works...

The "force" part in WindowForceBlurRole is kinda confusing. If you set WindowForceBlurRole for some window, background behind it won't be necessarily blurred. By setting that role, we tell the Blur effect "that's fine to do your business behind transformed windows". (same with the WindowForceBackgroundContrastRole)

So, shouldForceBlur and shouldForceBackgroundContrast are redundant. Please delete them.


Apparently, acceptsFocus() is the only way how OSD differs from ordinary window on Wayland

That looks like a workaround (isSpecialWindow checks whether window is an OSD).

@davidedmundson Can you take a look?

effects/cube/cubeslide.cpp
505

It will detach. See https://www.dvratil.cz/2015/06/qt-containers-and-c11-range-based-loops/

"Proper" way would be

const EffectWindowList windows = effects->stackingOrder();
for (EffectWindow *w : windows) {
}
516–519
528

Please force blur only for panels and sticky windows.

The blur effect can't work in 3d.

In D15175#318532, @zzag wrote:

OK, so we don't really need m_forcedRoles(and panels, and stickyWindows) to track windows with forced roles. We could just reset forced roles for all windows when the animation is finished.
But, conceptually, it would be wrong. So, yeah, we need to store windows with forced roles somewhere.

But what about the current code? It resets roles not for all windows, but only sticky ones, by simply iterating over them and checking if we shouldAnimate() them.
It doesn't look like a big overhead, since it's done only when animation ends. But we don't need a QSet with it.

The "force" part in WindowForceBlurRole is kinda confusing. If you set WindowForceBlurRole for some window, background behind it won't be necessarily blurred. By setting that role, we tell the Blur effect "that's fine to do your business behind transformed windows". (same with the WindowForceBackgroundContrastRole)

So, shouldForceBlur and shouldForceBackgroundContrast are redundant. Please delete them.

Cool, thanks for clarifying!

Again, sorry for the inconvenience.

No problem :)

Apparently, acceptsFocus() is the only way how OSD differs from ordinary window on Wayland

That looks like a workaround (isSpecialWindow checks whether window is an OSD).

It is indeed, unfortunately, that's why I've marked it with XXX. I did some debugging, here are flags for OSD on Wayland:

[Cube] Should not animate QRegion(0,0 760x148) " <2>"
 - isCurrentTab: true
 - isDialog: false
 - isDNDIcon: false
 - isDropdownMenu: false
 - isManaged: false
 - isMenu: false
 - isModel: false
 - isMovable: true
 - isMovableAcrossScreens: true
 - isNormalWindow: true
 - isNotification: false
 - isOnAllDesktops: true
 - isPopupMenu: false
 - isSkipSwitcher: false
 - isSpecialWindow: false
 - isSplash: false
 - isToolbar: false
 - isTooltip: false
 - isUserMove: false
 - isUserResize: false
 - isUtility: false
 - isVisible: true
 - keepAbove: false
 - acceptsFocus: false

and for ordinary "sticky" window (Wayland):

[Cube] Should not animate QRegion(0,0 940x1084) "Home — Dolphin"
 - isCurrentTab: true
 - isDialog: false
 - isDNDIcon: false
 - isDropdownMenu: false
 - isManaged: false
 - isMenu: false
 - isModel: false
 - isMovable: true
 - isMovableAcrossScreens: true
 - isNormalWindow: true
 - isNotification: false
 - isOnAllDesktops: true
 - isPopupMenu: false
 - isSkipSwitcher: false
 - isSpecialWindow: false
 - isSplash: false
 - isToolbar: false
 - isTooltip: false
 - isUserMove: false
 - isUserResize: false
 - isUtility: false
 - isVisible: true
 - keepAbove: false
 - acceptsFocus: true

and for OSD (X11):

[Cube] Should not animate QRegion(0,0 760x148) ""
 - isCurrentTab: true
 - isDialog: false
 - isDNDIcon: false
 - isDropdownMenu: false
 - isManaged: false
 - isMenu: false
 - isModel: false
 - isMovable: false
 - isMovableAcrossScreens: false
 - isNormalWindow: true
 - isNotification: false
 - isOnAllDesktops: true
 - isPopupMenu: false
 - isSkipSwitcher: false
 - isSpecialWindow: true
 - isSplash: false
 - isToolbar: false
 - isTooltip: false
 - isUserMove: false
 - isUserResize: false
 - isUtility: false
 - isVisible: true
 - keepAbove: true
 - acceptsFocus: true
poboiko added inline comments.Sep 1 2018, 9:27 AM
effects/cube/cubeslide.cpp
528

There is a check for !shouldAnimate, it's fine.

poboiko updated this revision to Diff 40808.Sep 1 2018, 9:55 AM

Got rid of shouldForceBlur & shouldForceBackgroundContrast; also forgot to unset background contrast role, which interfered with other effects.

poboiko marked 4 inline comments as done.Sep 1 2018, 9:56 AM
zzag added a comment.EditedSep 1 2018, 10:03 AM

We don't need KWayland includes anymore, pls delete them.

But what about the current code? It resets roles not for all windows, but only sticky ones, by simply iterating over them and checking if we shouldAnimate() them.

What if config has been changed while the effect is still active? That's unlikely to happen, but still there is a chance for that.

Other than acceptsFocus, I think everything looks fine.

Please wait for other KWin folks.

poboiko updated this revision to Diff 40838.Sep 1 2018, 10:40 PM

Added QSet for static windows & corresponding code to manage it (including slotWinowDeleted).

zzag added a comment.Sep 2 2018, 8:22 AM
This comment was removed by zzag.
zzag edited the summary of this revision. (Show Details)Sep 5 2018, 11:05 AM
zzag added a comment.EditedSep 5 2018, 1:14 PM

Hmm, I wonder why the desktop switch OSD is a special window on X11. After inspecting "the osd" with xprop and xwininfo, it looks like it's an ordinary unmanaged window, e.g.

# xprop's output
...
_NET_WM_WINDOW_TYPE(ATOM) = _KDE_NET_WM_WINDOW_TYPE_OVERRIDE, _NET_WM_WINDOW_TYPE_NORMAL
...

# xwininfo's output
...
  Override Redirect State: yes
...

On Wayland, it's not special because the OSD doesn't set appropriate type.

Regarding isUtility and acceptsFocus, please delete them. It makes sense to hide/ignore utilities and windows that don't accept focus in the Flip Switch(and Present Windows) effect, but not in this one.

Hmm, I wonder why the desktop switch OSD is a special window on X11. After inspecting "the osd" with xprop and xwininfo, it looks like it's an ordinary unmanaged window, e.g.

# xprop's output
...
_NET_WM_WINDOW_TYPE(ATOM) = _KDE_NET_WM_WINDOW_TYPE_OVERRIDE, _NET_WM_WINDOW_TYPE_NORMAL
...

# xwininfo's output
...
  Override Redirect State: yes
...

Naive investigation shows that inside libkwineffects/effectwindow.cpp, it declares EffectWindow::isSpecialWindow (using WINDOW_HELPER_DEFAULT macro) to return default value (which is set to be true) if corresponding parent()->property() is invalid.
For ordinary windows (i.e. Dolphin), the parent is KWin::Client, which is subclass of KWin::AbstractClient, which has isSpecialWindow() method (and corresponding property), which works fine.
For OSD, the parent is KWin::Unmanaged, which is subclass of KWin::Toplevel, which doesn't have this property - thus it returns default value. Apparently, that means that all unmanaged windows are special.

On Wayland, it's not special because the OSD doesn't set appropriate type.

Regarding isUtility and acceptsFocus, please delete them. It makes sense to hide/ignore utilities and windows that don't accept focus in the Flip Switch(and Present Windows) effect, but not in this one.

Well, for now acceptsFocus() is the only workaround to determine whether a window is an OSD, so in current state we need it. Either that or fixing PlasmaDialog/Wayland interaction (maybe there is some third option that I've missed, though). Unfortunately, I'm not really familiar with the latter...

As for isUtility() - well, to be honest I don't even know for sure what is considered to be an utility window, and wm-spec didn't shed much light on it.
I just thought it won't hurt if we'll make various weird/non-normal windows sticky (and for that I've borrowed the code, which seemed to be working). I totally rely on you on this matter, however.

zzag added a comment.Sep 6 2018, 8:50 AM

As for isUtility() - well, to be honest I don't even know for sure what is considered to be an utility window, and wm-spec didn't shed much light on it.

If you are familiar with GIMP, those 2 windows with layers, brushes, etc are utilities.

poboiko updated this revision to Diff 41103.Sep 6 2018, 12:36 PM

Thanks! Indeed, it doesn't make any sense to force them to be sticky. Got rid of that check.

Apparently, acceptsFocus() is the only way how OSD differs from ordinary window on Wayland

Which OSDs specifically?

There's no way to specify it with the core protocols but the Plasma specific protocol (which you'd need for positioning an OSD) has roles which includes OnScreenDisplay.

AFAIK that role is all hooked up on the kwin shellclient side but all actual OSDs need to set it correctly.

zzag added a comment.EditedSep 8 2018, 9:53 PM

Which OSDs specifically?

The one that appears when switching virtual desktops (Desktop Behavior > Virtual Desktops > Switching > "Desktop Switch On-Screen Display").

AFAIK that role is all hooked up on the kwin shellclient side but all actual OSDs need to set it correctly.

Yes, that way we would need to check only isSpecialWindow.

The problem is the desktop switch OSD(with type: PlasmaCore.Dialog.OnScreenDisplay) on Wayland is initially placed at the lower 1/3 of the screen.

zzag added a comment.EditedSep 13 2018, 4:46 PM

This diff addresses 2 different problems:

  • visual artifacts caused by the blur and the background contrast effect;
  • usage of isManaged for Wayland clients.

Can you please split this patch into two? (all you have to do is just replace w->isSpecialWindow() with !w->isManaged(), I guess)

effects/cube/cubeslide.cpp
530–532

You don't really need it.

545–555

After thinking for a while, I think this should be

if (w->isSpecialWindow()) {
    return false;
}
return !dontSlideStickyWindows;
zzag added inline comments.Sep 13 2018, 5:04 PM
effects/cube/cubeslide.cpp
100–102

Maybe, check if there any static windows.

100–102

if (!staticWindows.isEmpty()) {

...

}

poboiko updated this revision to Diff 41600.Sep 14 2018, 7:23 AM
poboiko marked 3 inline comments as done.Sep 14 2018, 7:27 AM
poboiko added inline comments.
effects/cube/cubeslide.cpp
545–555

In general, yes. When the corresponding stuff inside KWayland will get fixed.
But for now, no, because isSpecialWindow() is false for our DesktopChangeOSD (which we want to be sticky always, not just when the flag is checked).

In D15175#325337, @zzag wrote:

This diff addresses 2 different problems:

  • visual artifacts caused by the blur and the background contrast effect;
  • usage of isManaged for Wayland clients.

    Can you please split this patch into two? (all you have to do is just replace w->isSpecialWindow() with !w->isManaged(), I guess)

Yeah, sure, I can separate that one-line change. Should I post it on Phabricator as well?

zzag added a comment.Sep 14 2018, 7:33 AM

Yeah, sure, I can separate that one-line change. Should I post it on Phabricator as well?

Yes. This patch should fix the first issue and in another patch replace isManaged with isSpecialWindow. (Please don't use acceptsFocus in the first patch, i.e. in this one)

When the corresponding stuff inside KWayland will get fixed.

That's not a KWayland issue.

poboiko updated this revision to Diff 41607.Sep 14 2018, 8:04 AM
poboiko retitled this revision from [effects/cubeslide] Fix several cubeslide issues to [effects/cubeslide] Fix visual glitches with Blur / BackgroundContrast effect.
poboiko edited the summary of this revision. (Show Details)
poboiko edited the test plan for this revision. (Show Details)

Yes. This patch should fix the first issue and in another patch replace isManaged with isSpecialWindow. (Please don't use acceptsFocus in the first patch, i.e. in this one)

OK, I've done it (hope I didn't mess it up), see D15496: [effects/cubeslide] Fix "sticky" windows detection on Wayland.

That's not a KWayland issue.

Sorry, my fault. What I wanted to say is Wayland integration (I am just not really familiar which project is responsible for what on Wayland)

zzag accepted this revision.Sep 14 2018, 8:49 AM

Looks good to me. Change FIXED-IN to 5.15.0

effects/cube/cubeslide.cpp
521–526

Readability nitpick: isActive() and shouldAnimate check two different things. It would be better to have something like

if (!isActive()) {
    return;
}
if (!shouldAnimate(w)) {
    ...
}
538

We always have to animate desktop window.

if (w->isDesktop()) {
    return true;
}
This revision is now accepted and ready to land.Sep 14 2018, 8:49 AM
This revision was automatically updated to reflect the committed changes.
zzag added a comment.Sep 14 2018, 9:43 AM

@poboiko Did you address my inline comments on push?

@poboiko Did you address my inline comments on push?

Oh, damn. Sorry, I've missed those.
As for isDesktop(), I've just moved it to the second patch, since it's related to the bug 390366. I hope it's not a big deal.
I'll make readability changes you've mentioned in a separate commit then.

BTW, should it be backported to 5.15 or something?

zzag added a comment.EditedSep 14 2018, 9:50 AM

BTW, should it be backported to 5.15 or something?

No, it will be in 5.15. We could backport it to 5.14, but it would be better to test it well (because this change is pretty big).

zzag added a comment.Sep 14 2018, 9:55 AM

I'll make readability changes you've mentioned in a separate commit then.

Well, no, there is no need. That was just a nitpick. Not a serious issue.