Respect animation settings
Needs ReviewPublic

Authored by sandsmark on Dec 22 2018, 11:14 AM.

Details

Reviewers
ngraham
Group Reviewers
Gwenview
Summary

Turn off animations if requested by the platform, and set the animation duration to what the style has.

Test Plan

Tried forcing off animations with QApplication::setEffectEnabled() in main.cpp and with qt5ct, and adjusting the animation duration with a custom style.

Diff Detail

Repository
R260 Gwenview
Lint
Lint Skipped
Unit
Unit Tests Skipped
sandsmark created this revision.Dec 22 2018, 11:14 AM
Restricted Application added a project: Gwenview. · View Herald TranscriptDec 22 2018, 11:14 AM
sandsmark requested review of this revision.Dec 22 2018, 11:14 AM
ngraham requested changes to this revision.Dec 22 2018, 4:26 PM
ngraham added a subscriber: ngraham.

A good idea. However I found a regression: in full screen view, the top bar now never disappears; it's always visible. It needs to on;y appear when the mouse approached the top edge of the screen.

This revision now requires changes to proceed.Dec 22 2018, 4:26 PM

A good idea. However I found a regression: in full screen view, the top bar now never disappears; it's always visible. It needs to on;y appear when the mouse approached the top edge of the screen.

Bah, my bad. It worked in the first version of the patch, before I revised it to check the effectenabled-flags. Should be trivial to fix.

sandsmark updated this revision to Diff 48016.Dec 22 2018, 4:42 PM

Fixed the issue with the fullscreenbar.

Thanks, that's fixed.

However, I'm seeing that this appears to always disable all animations for me, no matter what value the animation speed is set up in the Breeze style settings:

However, I'm seeing that this appears to always disable all animations for me, no matter what value the animation speed is set up in the Breeze style settings:

Do you know what that sets? The stylehint comes from the style plugin (https://github.com/sandsmark/sandsmark-integration/blob/master/src/widgetstyle/sandsmarkstyle.cpp#L3-L12), but the UI effects come from the platform plugin.

ngraham added a subscriber: cfeck.Dec 26 2018, 12:19 AM

I'm afraid not, sorry. But @cfeck or another Frameworks or KDE Applications person may know.

I'm afraid not, sorry. But @cfeck or another Frameworks or KDE Applications person may know.

seems like the issue is that the plasma-integration plugin and breeze doesn't know about eachother. So even if you disable/enable animations in breeze, plasma-integration doesn't know about it (it reads from its own config file). I'm not sure which config should be canonical, so I don't know where bug technically is.

I know that KWin has its own animation setting, exposed in the Compositor KCM. Is that the one that sets the animation speed in the Plasma Integration plugin?

For now this should probably look at the setting in the Widget Style, but it would be nice to unify these at tome point to a single "Animation Speed" control that affects both apps and Plasma.

Intuitively this patch makes sense and is certainly "technically correct", but feedback we had in plasma with the wallpaper was that a fullscreen transition is quite a bit separate to the setting you'd use for small widget effects. 100ms for altering a few pixels of a shadow is a much slower in pixels per second than 100ms to slide something across the whole screen.

We ended up with a second (hidden) config value.


As for the discussion above, I don't see how plasma-integration is relevant. It does control QStyleHints, but you're not using any of that.
Everything you use here comes from the QStyle which is all from Breeze.

With my plasma hat on, other than sharing anecdote above, what input do you need?

As for the discussion above, I don't see how plasma-integration is relevant. It does control QStyleHints, but you're not using any of that.
Everything you use here comes from the QStyle which is all from Breeze.

Sorry, I left this patch for too long, but IIRC the problem is that some of the animation settings comes from the platform plugin (the QApplication::isEffectEnabled() stuff; AnimateMenuUiEffect, AnimateComboUiEffect, AnimateTooltipUiEffect, etc. in the platform plugin API), and some animation settings are from the style plugin. And the style config in the screenshot from ngraham doesn't affect the platform plugin config.

With my plasma hat on, other than sharing anecdote above, what input do you need?

Not sure what the best solution is here, either to let the platform plugin read the style plugin config, let the style kcm write to the platform plugin config, or something else. I'm thinking the more "correct" solution is to let the kcm (in the screenshot above) write to the platform plugin config, probably expanding it a bit, so users can control it from a central place.