[effects] Rewrite the Glide effect
ClosedPublic

Authored by zzag on Jun 4 2018, 4:31 PM.

Details

Summary

There are several reasons why I "re-wrote" the Glide effect:

  • it doesn't work correctly because it suffers from undesired perspective distortions: The worst part is that windows are distorted so much on multiple monitor setups that it's hard to say whether that's glide animation.
  • window close animation is not quite intuitive: if the close button is located at the top and I click it, I would expect that window is rotated around the bottom edge, not the top; (IMHO)
  • it's too much distracting when working on something for quite good amount of time: e.g. when editing photos, which involves a big number of different dialogs;
  • there are issues with deletion of QTimeLine;
  • windows are not gracefully released if some other effect grabs them;
  • its code doesn't follow common coding style in KWin.

So, the "new" Glide effect is more subtle, it's possible to have
different rotation edges for window open/close animations, it doesn't
animate special windows(like audio volume feedback), the code is simpler
and readable. Yet, there are some issues with QTimeLine, which are
common to all effects in KWin anyway.

Demos


Window Open Animation


Window Close Animation


KCM

CCBUG: 394245

Test Plan
  • Enabled the Glide effect
  • Closed System Settings
  • Opened it again

Diff Detail

Repository
R108 KWin
Branch
rewrite-glide-effect
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 558
Build 570: arc lint + arc unit
zzag created this revision.Jun 4 2018, 4:31 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 4 2018, 4:31 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jun 4 2018, 4:31 PM
zzag updated this revision to Diff 35550.Jun 4 2018, 6:43 PM

Change access modifiers.

zzag updated this revision to Diff 35844.Jun 8 2018, 3:35 PM
  • Rename IsGlideWindow to IsGlideWindowRole
  • Decrease initial opacity for the window open animation
zzag updated this revision to Diff 35883.Jun 9 2018, 10:51 AM

Fix perspective distortion correction with per screen rendering.

zzag updated this revision to Diff 35884.Jun 9 2018, 12:06 PM

Don't reset IsGlideWindowRole if some other effect grabbed the window.

There is still a chance that the Glide effect could animate that window.

zzag updated this revision to Diff 36071.Jun 12 2018, 7:47 PM

Don't take visibility into account in isGlideWindow.

zzag updated this revision to Diff 36074.Jun 12 2018, 8:15 PM

Use some notion of a whitelist to decide whether to animate given window.

Don't use a blacklist to decide what windows should be animated. Instead,
use a whitelist(i.e. normal windows, dialogs, and everything with decorations).
That way, the Glide effect won't animate Audio Volume feedback. Yet, there is
still a blacklist to decide what windows should not be animated.

KRunner is a normal window so we should blacklist it. Kickoff is a normal
window so we should blacklist it. The biggest problem with Kickoff is that
it shares "plasmashell plasmashell" class with the rest of plasma windows, e.g.
the Task Manager Settings window also has "plasmashell plasmashell" class.

Other than that, I'm pretty happy right now with the Glide effect.

zzag added a comment.Jun 12 2018, 8:19 PM

It would be okay to check the blacklist after checking whether window has decorations only if Plasma doesn't reuse windows (not sure about this part).

In D13338#277548, @zzag wrote:

The biggest problem with Kickoff is that
it shares "plasmashell plasmashell" class with the rest of plasma windows, e.g.
the Task Manager Settings window also has "plasmashell plasmashell" class.

Sounds like this could be related to or the root cause of https://bugs.kde.org/show_bug.cgi?id=394934.

zzag added a comment.Jun 12 2018, 8:50 PM

Sounds like this could be related to or the root cause of https://bugs.kde.org/show_bug.cgi?id=394934.

I'm afraid this is not related to that bug report.

As I said before, the problem is plasmashell plasmashell class is shared among Plasma windows. Why did Glide effect work correctly before? E.g. it didn't animate Kickoff, but animated the Task Manager Settings windows. Well, the Glide effect tries to animate Kickoff, but then the Sliding popups effect kicks in and grabs the Kickoff window. There is a little big problem: if you turn off the Sliding popups effect, the Glide effect will animate Kickoff and all other windows that are not expected to be animated.

zzag added a comment.Jun 12 2018, 9:04 PM

One viable option I see is to not animate "slide" windows. (on wayland check w->surface()->slideOnShowHide()? on x11 somehow check _KDE_SLIDE atom?)

zzag updated this revision to Diff 36076.Jun 12 2018, 9:55 PM

Delete plasmashell and krunner from the blacklist.

zzag added a comment.EditedJun 13 2018, 12:20 PM

I'll leave things as they are with false triggering for slide windows when the Sliding popups effect is disabled.

Even with that issue being present, the "new" Glide effect is still an improvement over old glide effect:

  • animation looks the same no matter where it takes place on the screen;
  • it is more subtle (which is crucial for long-term usage);
  • it provides good defaults (IMHO);
  • close animation is more intuitive(IMHO);
  • it doesn't animate special windows (e.g. Audio Volume Feedback);
  • code is much better(IMHO). E.g. deletion of QTimeLine doesn't take its place in ~WindowInfo anymore (this is unsafe because WindowInfo didn't implement copy constructor and copy assignment operator)*.

Anyway, I think I finished my work on it and would like to hear opinion of KWin, Plasma, and VDG folks.

* There are still issues with deletion of QTimeLine, which are common to most of effects.

Can't comment on the code, but visually, I love it! I agree that it's a significant improvement over the current effect. More subtle and very classy.

zzag updated this revision to Diff 36145.Jun 14 2018, 11:39 AM

Gracefully release window if WindowClosedGrabRole role has been changed

Glide effect should unref window when it releases window that was grabbed
with WindowClosedGrabRole. Yet, that's really rare to happen (I haven't seen
any bug reports), but still possible.

zzag edited the summary of this revision. (Show Details)Jun 15 2018, 7:24 PM
zzag edited the summary of this revision. (Show Details)Jun 15 2018, 7:28 PM
zzag edited the summary of this revision. (Show Details)Jun 15 2018, 7:35 PM
zzag updated this revision to Diff 36603.Jun 24 2018, 2:57 PM

Drop the IsGlideWindowRole hack, don't need it anymore.

zzag planned changes to this revision.Jun 28 2018, 1:58 PM

Port to TimeLine.

zzag updated this revision to Diff 36929.Jun 30 2018, 7:25 AM

Port to TimeLine.

zzag updated this revision to Diff 37065.Jul 2 2018, 1:41 PM
  • support information
  • constrain opacity values
zzag updated this revision to Diff 37145.Jul 4 2018, 2:03 PM

Delete redundant check

ngraham accepted this revision as: VDG.Jul 16 2018, 6:42 PM

VDG-approved, but please get a code review too.

zzag added a comment.Jul 23 2018, 7:55 AM

Weekly ping.

Could we get Plasma or KWin review for this?

davidedmundson accepted this revision.Jul 24 2018, 2:13 PM
This revision is now accepted and ready to land.Jul 24 2018, 2:13 PM
This revision was automatically updated to reflect the committed changes.