D28651: Load and use global animation settings

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze, ngraham
Cc: meven, cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, 
plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze, ngraham
Cc: meven, cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, 
plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Nathaniel Graham]

ngraham closed this revision.

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze, ngraham
Cc: meven, cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, 
plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> breezestyle.cpp:189
> +QStringLiteral( "org.kde.KGlobalSettings" ),
> +QStringLiteral( "notifyChange" ), this, 
> SLOT(configurationChanged()) );
>  #if QT_VERSION < 0x050D00 // Check if Qt version < 5.13

Shouldn't it be globalConfigurationChanged instead of configurationChanged

Or globalConfigurationChanged seems unused.

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: meven, cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, 
plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[David Edmundson]

davidedmundson added a comment.


  Just push it.

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, 
#breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


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

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, 
#breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Nathaniel Graham]

ngraham added a subscriber: cblack.
ngraham added a comment.


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

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, 
#breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark updated this revision to Diff 80551.
sandsmark added a comment.


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

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28651?vs=79916=80551

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

AFFECTED FILES
  kdecoration/config/breezeconfigwidget.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h
  kstyle/config/breezestyleconfig.cpp
  kstyle/config/ui/breezestyleconfig.ui

To: sandsmark, #breeze
Cc: davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Nathaniel Graham]

ngraham added a comment.


  In D28651#646418 , @sandsmark 
wrote:
  
  > 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.

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  In D28651#647122 , @davidedmundson 
wrote:
  
  > 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?

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[David Edmundson]

davidedmundson added a comment.


  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(), ::configChanged, 
this,
[this](const KConfigGroup , const QByteArrayList ) {
if (group.name() == QLatin1String("KDE") && 
names.contains(QByteArrayLiteral("AnimationDurationFactor"))) {
loadGlobalAnimationSettings();
}
});

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  In D28651#646572 , @ndavis wrote:
  
  > 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`)?

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Noah Davis]

ndavis added a comment.


  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.

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark updated this revision to Diff 79916.
sandsmark added a comment.


  Now should reload the animation settings when changed anywhere.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28651?vs=79914=79916

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

AFFECTED FILES
  kdecoration/config/breezeconfigwidget.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  In D28651#643681 , @ndavis wrote:
  
  > 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).

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark updated this revision to Diff 79914.
sandsmark added a comment.


  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.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28651?vs=79566=79914

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

AFFECTED FILES
  kdecoration/config/breezeconfigwidget.cpp
  kstyle/breezestyle.cpp

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Noah Davis]

ndavis added a comment.


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

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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)

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Noah Davis]

ndavis added a comment.


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

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ndavis, plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark created this revision.
sandsmark added a reviewer: Breeze.
sandsmark added a project: Breeze.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
sandsmark requested review of this revision.

REVISION SUMMARY
  In addition to the specific Breeze animation settings, KDE has "global" 
animation settings primarily used for `Qt::UIEffect`s 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.

REPOSITORY
  R31 Breeze

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

AFFECTED FILES
  kstyle/breezestyle.cpp

To: sandsmark, #breeze
Cc: plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart