D15175: [effects/cubeslide] Fix several cubeslide issues

zzag added a subscriber: davidedmundson.
zzag added a comment.


  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?

INLINE COMMENTS

> cubeslide.cpp:505
> +void CubeSlideEffect::startAnimation() {
> +    for (EffectWindow* w : effects->stackingOrder()) {
> +        if (shouldAnimate(w)) {

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) {
  }

> cubeslide.cpp:516-519
> +    if (slideRotations.count() == 1)
> +        timeLine.setCurveShape(QTimeLine::EaseInOutCurve);
> +    else
> +        timeLine.setCurveShape(QTimeLine::EaseInCurve);

Please use curly braces.

https://community.kde.org/Policies/Kdelibs_Coding_Style#Braces

> cubeslide.cpp:528
> +    }
> +    if (shouldForceBlur(w)) {
> +        w->setData(WindowForceBlurRole, QVariant(true));

Please force blur only for panels and sticky windows.

The blur effect can't work in 3d.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D15175

To: poboiko, #kwin, zzag
Cc: davidedmundson, zzag, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart