Load and use global animation settings
ClosedPublic

Authored by sandsmark on Apr 7 2020, 1:16 PM.

Details

Reviewers
ngraham
Group Reviewers
Breeze
Summary

In addition to the specific Breeze animation settings, KDE has "global" animation settings primarily used for Qt::UIEffects like Qt::UI_AnimateMenu, Qt::UI_AnimateCombo, Qt::UI_AnimateTooltip and Qt::UI_AnimateToolBox.

This patch ensures that Breeze use and respect those settings, which both harmonizes with other styles (if QGuiApplication::desktopSettingsAware() is true).

Test Plan

Turn animations on and off in kdeglobals, and see animations turn off and on when using Breeze.

Also makes https://phabricator.kde.org/D17732 work properly with breeze.

Diff Detail

Repository
R31 Breeze
Lint
Lint Skipped
Unit
Unit Tests Skipped
sandsmark created this revision.Apr 7 2020, 1:16 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 7 2020, 1:16 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sandsmark requested review of this revision.Apr 7 2020, 1:16 PM
ndavis added a subscriber: ndavis.Apr 7 2020, 1:52 PM

So this makes Breeze respect the global animation speed setting in the Workspace Behavior -General Behavior KCM?

I have no idea how this is supposed to work. But the current fix overwrites what's in the breeze configutation, right ? Would it not lead to utter confusion ?
These settings should be set at one place and one only. Whether breeze or global I have no strong oppinion (so no strong objection to this approach), but then the other setting must go. (presently: the one in breeze)

Also, one should double check if this is really what we wants, for animation speed, or if additional factors are needed. (not all animations must have the same speed).

I have no time to test this though. Someone else should have a look (and check conflicts with the breeze option)

ngraham added a subscriber: ngraham.Apr 7 2020, 2:12 PM

I agree with Hugo that if we make Breeze respect the new global animation speed slider, we should remove the user-facing setting in Breeze's own settings to control the animation speed. Having two config knobs for ostensibly the same thing (or more accurately, one adjusts a subset of what the other one adjusts) is bound to cause confusion and problems.

ndavis added a comment.EditedApr 7 2020, 2:20 PM

I don't know enough about KDE configuration management to judge the code, but with this patch, changing animation speeds in SySe works if I restart apps after the change.

sandsmark updated this revision to Diff 79914.Apr 12 2020, 10:38 AM

Also made it store to the global configuration. This way it is backwards compatible, but the config can also be changed from both places.

I think it makes sense to have it both places, having it in the breeze settings avoids people having to go back and forth to tune their settings.

I don't know enough about KDE configuration management to judge the code, but with this patch, changing animation speeds in SySe works if I restart apps after the change.

The breeze plugin needs to do something like this: https://cgit.kde.org/plasma-integration.git/tree/src/platformtheme/khintssettings.cpp#n234 (and now I see that the slot there is missing some stuff, I'll see if I can fix that later).

sandsmark updated this revision to Diff 79916.Apr 12 2020, 11:02 AM

Now should reload the animation settings when changed anywhere.

You need to fix the git author info. If you upload a patch via the web UI instead of arc, the author info gets messed up.

It's still working for me. I also still need to restart apps for changes to the global animation settings to apply.

You need to fix the git author info. If you upload a patch via the web UI instead of arc, the author info gets messed up.

I usually just push normally after approval, but I'll see what I can do.

It's still working for me. I also still need to restart apps for changes to the global animation settings to apply.

Could you run dbus-monitor while changing it, and look for the org.kde.Breeze.Style and org.kde.KGlobalSettings messages? And are the other changes made in the UI updated automatically (which should be reloaded by the org.kde.Breeze.Style)?

As for runtime changes I'm trying to migrate more things to KConfigWatcher which I wrote to replace random ad-hoc ints everywhere as well as making sure we automatically reparse the config once and only once.

It's going to be /amazing/ but it's being rolled out as a slow migration, so there's nothing wrong with merging this as-is and migrating later.

m_animationSpeedWatcher = KConfigWatcher::create(KSharedConfig::openConfig());
connect(m_animationSpeedWatcher.data(), &KConfigWatcher::configChanged, this,
    [this](const KConfigGroup &group, const QByteArrayList &names) {
        if (group.name() == QLatin1String("KDE") && names.contains(QByteArrayLiteral("AnimationDurationFactor"))) {
            loadGlobalAnimationSettings();
        }
});

As for runtime changes I'm trying to migrate more things to KConfigWatcher which I wrote to replace random ad-hoc ints everywhere as well as making sure we automatically reparse the config once and only once.

It's going to be /amazing/ but it's being rolled out as a slow migration, so there's nothing wrong with merging this as-is and migrating later.

It looks kind of amazing, and it would get rid of that annoying magic 3 I think?

I think it makes sense to have it both places, having it in the breeze settings avoids people having to go back and forth to tune their settings.

I'm afraid I don't agree. Having two places to configure the same thing is confusing, especially because one only affects a subset of the other. If I adjust the global slider in the Workspace KCM, it doesn't affect the setting in Breeze's own settings dialog, making that setting inaccurate. So I can make the animations very slow everywhere, but the Breeze settings window inaccurately says that animations are 100ms in duration.

sandsmark updated this revision to Diff 80551.Apr 19 2020, 2:56 PM

Remove the duplication of animation control, and don't override the animation settings if people haven't adjusted it globally.

ngraham added a subscriber: cblack.Apr 20 2020, 5:49 PM

Works for me now. @hpereiradacosta, @ndavis, and @cblack, are you okay with this?

Should I move this to invent, or just push it?

ognarb awarded a token.Jul 9 2020, 4:19 PM
meven added a subscriber: meven.Jul 10 2020, 8:06 AM
meven added inline comments.
kstyle/breezestyle.cpp
189

Shouldn't it be globalConfigurationChanged instead of configurationChanged

Or globalConfigurationChanged seems unused.

ngraham accepted this revision.Aug 3 2020, 3:44 PM
This revision is now accepted and ready to land.Aug 3 2020, 3:44 PM
ngraham closed this revision.Aug 3 2020, 3:49 PM

Thanks for this change !
Can the same be done in the breeze window decoration ? It is strange to have an animation settings there and not in the style.

Hugo

Agreed. @sandsmark, would you be interested in following up with that?