D15175: [effects/cubeslide/wayland] Fix several cubeslide issues

zzag requested changes to this revision.
zzag added a comment.
This revision now requires changes to proceed.


  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

INLINE COMMENTS

> cubeslide.cpp:43
>      , progressRestriction(0.0f)
> +    , stickyPainting(false)
>  {

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

> cubeslide.cpp:50
> +            this, &CubeSlideEffect::windowDeleted);
> +    connect(effects, static_cast<void (EffectsHandler::*)(int,int,EffectWindow*)>(&EffectsHandler::desktopChanged),
> +            this, &CubeSlideEffect::slotDesktopChanged);

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

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

> cubeslide.cpp:188-194
> +    if (shouldForceBlur(w)) {
> +        w->setData(WindowForceBlurRole, QVariant(true));
> +        m_forcedRoles.blur << w;
> +    }
> +    if (shouldForceBackgroundContrast(w)) {
> +        w->setData(WindowForceBackgroundContrastRole, QVariant(true));
> +        m_forcedRoles.backgroundContrast << w;

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.

> cubeslide.cpp:410
>  
> -void CubeSlideEffect::slotDesktopChanged(int old, int current)
> +void CubeSlideEffect::slotDesktopChanged(int old, int current, EffectWindow* w)
>  {

Q_UNUSED(w)

> cubeslide.cpp:522
> +void CubeSlideEffect::windowAdded(EffectWindow* w) {
> +    if (slideRotations.empty()) {
> +        return;

Please use `isActive` instead.

> cubeslide.cpp:525-532
> +    if (shouldForceBlur(w)) {
> +        w->setData(WindowForceBlurRole, QVariant(true));
> +        m_forcedRoles.blur << w;
> +    }
> +    if (shouldForceBackgroundContrast(w)) {
> +        w->setData(WindowForceBackgroundContrastRole, QVariant(true));
> +        m_forcedRoles.backgroundContrast << w;

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.

> cubeslide.cpp:543
> +
> +bool CubeSlideEffect::shouldAnimate(const EffectWindow* w) const
> +{

Hm, what about renaming it to `isStickyWindow`?

> cubeslide.cpp:548
> +    }
> +    if (dontSlideStickyWindows && w->isOnAllDesktops() && !w->isDesktop() && !w->isDock()) {
> +        return false;

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.

> cubeslide.cpp:556-571
> +    // Adopted from SlideEffect::shouldForceBlur
> +    if (w->data(WindowForceBlurRole).toBool()) {
> +        return false;
> +    }
> +    if (w->data(WindowBlurBehindRole).isValid()) {
> +        return true;
> +    }

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

> cubeslide.h:75-76
>  private Q_SLOTS:
> -    void slotDesktopChanged(int old, int current);
> +    void windowAdded(EffectWindow* w);
> +    void windowDeleted(EffectWindow* w);
> +

Please prepend `slot`.

> cubeslide.h:78
>      void slotWindowFinishUserMovedResized(KWin::EffectWindow *w);
> -
>  private:

Unrelated whitespace change.

REPOSITORY
  R108 KWin

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

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