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 turning on/off animations 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.

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.

FWIW, the patch does use the style hints (qApp->style()->styleHint()) where it makes sense.

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:

And to try to explain this a bit clearer; what you configure there in the breeze config doesn't affect e. g. QApplication::isEffectEnabled(Qt::UI_FadeTooltip), so fading is disabled, and neither does it affect UI_AnimateTooltip, so the move animation is disabled.

One solution is to extend breezeconfigwidget with options for changing GraphicEffectsLevel which plasma-integration reads, which affects those values, but that's where the Plasma people became relevant to the discussion.

sandsmark updated this revision to Diff 67194.Oct 2 2019, 2:36 PM
sandsmark edited the test plan for this revision. (Show Details)

In fact we now have a shiny new global animation slider in the General Behavior KCM. If you change the patch to make Gwenview respect that setting, I'll be happy with it.

In fact we now have a shiny new global animation slider in the General Behavior KCM. If you change the patch to make Gwenview respect that setting, I'll be happy with it.

Do you know what that adjusts? If it adjusts the normal Qt animation hints/properties like QStyle::SH_Widget_Animation_Duration this patch Should Just Work™.

sandsmark added a comment.EditedOct 11 2019, 7:29 PM

In fact we now have a shiny new global animation slider in the General Behavior KCM. If you change the patch to make Gwenview respect that setting, I'll be happy with it.

Do you know what that adjusts? If it adjusts the normal Qt animation hints/properties like QStyle::SH_Widget_Animation_Duration this patch Should Just Work™.

Apparently it just sets some value that's not used by Breeze nor plasma-integration. I think it should work (with this patch) if it is fixed to set the StyleConfigData::animationsDuration() in Breeze (I'm really confused about how StyleConfigData works, though, otherwise I'd try to create a patch).

To be 100% correct it should probably also adjust GraphicEffectsLevel in kdeglobals (if the slider is at zero it should at least remove the Qt::UI_AnimateToolBox, Qt::UI_AnimateTooltip, Qt::UI_AnimateCombo and Qt::UI_AnimateMenu values, the FadeFoo ones could probably go too). That's what plasma-integration picks up and passes to applications at least.

Something like this I think should work:

diff --git kcms/workspaceoptions/workspaceoptions.cpp kcms/workspaceoptions/workspaceoptions.cpp
index d6443e219..355e72b68 100644
--- kcms/workspaceoptions/workspaceoptions.cpp
+++ kcms/workspaceoptions/workspaceoptions.cpp
@@ -61,6 +61,12 @@ void KCMWorkspaceOptions::save()
     savePlasmarc();
     saveKdeglobals();
 
+    KConfigGroup cg(KSharedConfig::openConfig(QStringLiteral("breezerc")),
+            QStringLiteral("Style"));
+    cg.writeEntry("AnimationsDuration", qBound(0, m_animationDurationFactor * 100, 100); // I just assume 100 is max, it is default at least
+    cg.writeEntry("AnimationsEnabled", m_animationDurationFactor > 0);
+    // Could probably adjust AnimationSteps as well depending on the duration, but year
+
     setNeedsSave(false);
 }

Please feel free to send a Plasma patch!

Please feel free to send a Plasma patch!

That was the plan, but I don't have a dev environment for plasma set up, and submitting patches that don't even compile is a bit embarrassing. :-)

But fixing that should fix animations in other Qt apps with custom widgets as well.

Please feel free to send a Plasma patch!

That was the plan, but I don't have a dev environment for plasma set up, and submitting patches that don't even compile is a bit embarrassing. :-)

But fixing that should fix animations in other Qt apps with custom widgets as well.

Oh but it's so easy! :-) https://community.kde.org/Get_Involved/development#Plasma

Something like this I think should work:

It'd work, but I'd be reject it.

Breeze should be reading the global setting in kdeglobals, not the other way round.
Having KCMs write the same value in a quadrillion places doesn't scale and ends up being a mess.

But yes, making breeze / oxygen follow it is part of the plan for that patch.

Something like this I think should work:

It'd work, but I'd be reject it.

Breeze should be reading the global setting in kdeglobals, not the other way round.
Having KCMs write the same value in a quadrillion places doesn't scale and ends up being a mess.

But yes, making breeze / oxygen follow it is part of the plan for that patch.

Something like this ok?:

diff --git a/kstyle/breezestyle.cpp b/kstyle/breezestyle.cpp
  index 3eb3bca9..8299860a 100644
  --- a/kstyle/breezestyle.cpp
  +++ b/kstyle/breezestyle.cpp
  @@ -1382,6 +1382,18 @@ namespace Breeze
           // load helper configuration
           _helper->loadConfig();

  +        // Load global animation settings
  +        KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kdeglobals"));
  +        const KConfigGroup cg(config, QStringLiteral("KDE"));
  +
  +        const int animationsDuration = cg.readEntry("AnimationDurationFactor", StyleConfigData::animationsDuration() / 100.0f) * 100;
  +        if (animationsDuration > 0) {
  +            StyleConfigData::setAnimationsDuration(animationsDuration);
  +            StyleConfigData::setAnimationsEnabled(true);
  +        } else {
  +            StyleConfigData::setAnimationsEnabled(false);
  +        }
  +
           // reinitialize engines
           _animations->setupEngines();
           _windowManager->initialize();

I'm not entirely sure where in breeze the best place to put it is, but if this is ok I can open a separate PR (or whatever the phabricator name is).

Looks sane enough. Feel free to open a diff on Breeze and just tag Breeze as the reviewer. We'll take a look!

+        KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kdeglobals"));

Just KSharedConfig::openConfig()

it'll include kdeglobals wihtout a reparse

+        KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kdeglobals"));

Just KSharedConfig::openConfig()

it'll include kdeglobals wihtout a reparse

Reviewing before the review is open! That's the fastest feedback on a review I've ever gotten.

And FWIW, after https://phabricator.kde.org/D28373 landed animations are smooth where they used to bother me, so this isn't really that important for me personally anymore. But being able to turn off animations (without cluttering the gwenview settings more) is nice anyways.