Review Request 120970: Remove combo "Graphical Effects"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/ --- Review request for Plasma. Repository: plasma-desktop Description --- Remove combo "Graphical Effects" It set a config value which was exposed in KGlobalSettings::graphicEffectsLevel that is now deprecated. Diffs - kcms/style/finetuning.ui 3abb692 kcms/style/kcmstyle.cpp 6585ee6 Diff: https://git.reviewboard.kde.org/r/120970/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69790 --- +1 - Martin Gräßlin On Nov. 4, 2014, 11:55 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 11:55 a.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- Who's responsible for setting the dedicated replacement QStyle::styleHint(SH_Widget_Animate)? Is it now up to the style then, ie. animations turned on/off in Oxygen/Breeze? - Kai Uwe Broulik On Nov. 4, 2014, 10:55 vorm., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 10:55 vorm.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Nov. 4, 2014, 10:59 a.m., Kai Uwe Broulik wrote: > > Who's responsible for setting the dedicated replacement > > QStyle::styleHint(SH_Widget_Animate)? > > Is it now up to the style then, ie. animations turned on/off in > > Oxygen/Breeze? Turns out that was a very good question. frameworkintegration/src/kstyle/kstyle.cpp case SH_Widget_Animate: { KConfigGroup g(KSharedConfig::openConfig(), "KDE-Global GUI Settings"); return g.readEntry("GraphicEffectsLevel", true); } So we were using this config entry, but writing it as an integer and reading it as a boolean. So it was animated for everything, except: "Low display resolution and Low CPU" - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- On Nov. 4, 2014, 10:55 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 10:55 a.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Nov. 4, 2014, 11:59 a.m., Kai Uwe Broulik wrote: > > Who's responsible for setting the dedicated replacement > > QStyle::styleHint(SH_Widget_Animate)? > > Is it now up to the style then, ie. animations turned on/off in > > Oxygen/Breeze? > > David Edmundson wrote: > Turns out that was a very good question. > > frameworkintegration/src/kstyle/kstyle.cpp > case SH_Widget_Animate: { > KConfigGroup g(KSharedConfig::openConfig(), "KDE-Global GUI > Settings"); > return g.readEntry("GraphicEffectsLevel", true); > } > > So we were using this config entry, but writing it as an integer and > reading it as a boolean. So it was animated for everything, except: "Low > display resolution and Low CPU" Personal opinion: this shouldn't be on any config option at all. If at all the animation setting should get derived from the hardware. If a style wants to expose an option that's fine, but it should be in the style specific config.. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- On Nov. 4, 2014, 11:55 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 11:55 a.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Nov. 4, 2014, 10:59 vorm., Kai Uwe Broulik wrote: > > Who's responsible for setting the dedicated replacement > > QStyle::styleHint(SH_Widget_Animate)? > > Is it now up to the style then, ie. animations turned on/off in > > Oxygen/Breeze? > > David Edmundson wrote: > Turns out that was a very good question. > > frameworkintegration/src/kstyle/kstyle.cpp > case SH_Widget_Animate: { > KConfigGroup g(KSharedConfig::openConfig(), "KDE-Global GUI > Settings"); > return g.readEntry("GraphicEffectsLevel", true); > } > > So we were using this config entry, but writing it as an integer and > reading it as a boolean. So it was animated for everything, except: "Low > display resolution and Low CPU" > > Martin Gräßlin wrote: > Personal opinion: this shouldn't be on any config option at all. If at > all the animation setting should get derived from the hardware. If a style > wants to expose an option that's fine, but it should be in the style specific > config.. Deduced from the hardware (and "is remote session") is certainly nice but difficult, no? Also, what about Plasma's animations? We do use units.{long,short}Duration everywhere but can this be disabled somewhere or what settings does that follow? - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- On Nov. 4, 2014, 10:55 vorm., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 10:55 vorm.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Nov. 4, 2014, 11:59 a.m., Kai Uwe Broulik wrote: > > Who's responsible for setting the dedicated replacement > > QStyle::styleHint(SH_Widget_Animate)? > > Is it now up to the style then, ie. animations turned on/off in > > Oxygen/Breeze? > > David Edmundson wrote: > Turns out that was a very good question. > > frameworkintegration/src/kstyle/kstyle.cpp > case SH_Widget_Animate: { > KConfigGroup g(KSharedConfig::openConfig(), "KDE-Global GUI > Settings"); > return g.readEntry("GraphicEffectsLevel", true); > } > > So we were using this config entry, but writing it as an integer and > reading it as a boolean. So it was animated for everything, except: "Low > display resolution and Low CPU" > > Martin Gräßlin wrote: > Personal opinion: this shouldn't be on any config option at all. If at > all the animation setting should get derived from the hardware. If a style > wants to expose an option that's fine, but it should be in the style specific > config.. > > Kai Uwe Broulik wrote: > Deduced from the hardware (and "is remote session") is certainly nice but > difficult, no? > Also, what about Plasma's animations? We do use > units.{long,short}Duration everywhere but can this be disabled somewhere or > what settings does that follow? > We do use units.{long,short}Duration everywhere but can this be disabled > somewhere or what settings does that follow? Look&Feel package, iirc. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- On Nov. 4, 2014, 11:55 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 11:55 a.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Lis. 4, 2014, 11:59 dop., Kai Uwe Broulik wrote: > > Who's responsible for setting the dedicated replacement > > QStyle::styleHint(SH_Widget_Animate)? > > Is it now up to the style then, ie. animations turned on/off in > > Oxygen/Breeze? > > David Edmundson wrote: > Turns out that was a very good question. > > frameworkintegration/src/kstyle/kstyle.cpp > case SH_Widget_Animate: { > KConfigGroup g(KSharedConfig::openConfig(), "KDE-Global GUI > Settings"); > return g.readEntry("GraphicEffectsLevel", true); > } > > So we were using this config entry, but writing it as an integer and > reading it as a boolean. So it was animated for everything, except: "Low > display resolution and Low CPU" > > Martin Gräßlin wrote: > Personal opinion: this shouldn't be on any config option at all. If at > all the animation setting should get derived from the hardware. If a style > wants to expose an option that's fine, but it should be in the style specific > config.. > > Kai Uwe Broulik wrote: > Deduced from the hardware (and "is remote session") is certainly nice but > difficult, no? > Also, what about Plasma's animations? We do use > units.{long,short}Duration everywhere but can this be disabled somewhere or > what settings does that follow? > > Martin Klapetek wrote: > > We do use units.{long,short}Duration everywhere but can this be > disabled somewhere or what settings does that follow? > > Look&Feel package, iirc. Not only that, you can also enable/disable the individual effects using QApplication::setEffectEnabled(Qt::UIEffect effect, bool enable = true), see http://qt-project.org/doc/qt-5/qt.html#UIEffect-enum - Lukáš --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- On Lis. 4, 2014, 11:55 dop., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Lis. 4, 2014, 11:55 dop.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Nov. 4, 2014, 11:59 a.m., Kai Uwe Broulik wrote: > > Who's responsible for setting the dedicated replacement > > QStyle::styleHint(SH_Widget_Animate)? > > Is it now up to the style then, ie. animations turned on/off in > > Oxygen/Breeze? > > David Edmundson wrote: > Turns out that was a very good question. > > frameworkintegration/src/kstyle/kstyle.cpp > case SH_Widget_Animate: { > KConfigGroup g(KSharedConfig::openConfig(), "KDE-Global GUI > Settings"); > return g.readEntry("GraphicEffectsLevel", true); > } > > So we were using this config entry, but writing it as an integer and > reading it as a boolean. So it was animated for everything, except: "Low > display resolution and Low CPU" > > Martin Gräßlin wrote: > Personal opinion: this shouldn't be on any config option at all. If at > all the animation setting should get derived from the hardware. If a style > wants to expose an option that's fine, but it should be in the style specific > config.. > > Kai Uwe Broulik wrote: > Deduced from the hardware (and "is remote session") is certainly nice but > difficult, no? > Also, what about Plasma's animations? We do use > units.{long,short}Duration everywhere but can this be disabled somewhere or > what settings does that follow? > > Martin Klapetek wrote: > > We do use units.{long,short}Duration everywhere but can this be > disabled somewhere or what settings does that follow? > > Look&Feel package, iirc. > > Lukáš Tinkl wrote: > Not only that, you can also enable/disable the individual effects using > QApplication::setEffectEnabled(Qt::UIEffect effect, bool enable = true), see > http://qt-project.org/doc/qt-5/qt.html#UIEffect-enum > Also, what about Plasma's animations? completely orthogonal. For a widget style the CPU/RAM combination is important. For Plasma the GPU is important. E.g. if we run on llvmpipe we should disable all animations even if it's a 16 core CPU with 32 GB of RAM. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review69791 --- On Nov. 4, 2014, 11:55 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 11:55 a.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review70582 --- Ship it! I think we're all cool with removing this particular combobox (thanks Martin!), it's useless and its implementation is broken. We can always discuss bringing some kind of eye-candy-o-meter back, this patch can go in already. - Sebastian Kügler On Nov. 4, 2014, 10:55 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Nov. 4, 2014, 10:55 a.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/ --- (Updated Dec. 10, 2014, 2:49 p.m.) Status -- This change has been marked as submitted. Review request for Plasma. Repository: plasma-desktop Description --- Remove combo "Graphical Effects" It set a config value which was exposed in KGlobalSettings::graphicEffectsLevel that is now deprecated. Diffs - kcms/style/finetuning.ui 3abb692 kcms/style/kcmstyle.cpp 6585ee6 Diff: https://git.reviewboard.kde.org/r/120970/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review71713 --- I think this change has to be reverted, the setting in question is being used in frameworkintegration to set the level of graphical details to Qt, thru the QPA plugin: m_hints[QPlatformTheme::UiEffects] = cg.readEntry("GraphicEffectsLevel", 0) != 0 ? QPlatformTheme::GeneralUiEffect : 0; - Lukáš Tinkl On Pro. 10, 2014, 3:49 odp., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Pro. 10, 2014, 3:49 odp.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 120970: Remove combo "Graphical Effects"
> On Dec. 10, 2014, 3:19 p.m., Lukáš Tinkl wrote: > > I think this change has to be reverted, the setting in question is being > > used in frameworkintegration to set the level of graphical details to Qt, > > thru the QPA plugin: > > > > m_hints[QPlatformTheme::UiEffects] = cg.readEntry("GraphicEffectsLevel", 0) > > != 0 ? QPlatformTheme::GeneralUiEffect : 0; That's using it purely as a boolean. I'm not having a 5 entry combo box for that. Personally I say it can be a hidden config option; we don't /need/ to expose every option just because it can be set. If you really want it, I could combine it with Breeze's internal "Enable Animation" config option. From a usability POV it seems better than having two very similar sounding options which are scattered. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120970/#review71713 --- On Dec. 10, 2014, 2:49 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120970/ > --- > > (Updated Dec. 10, 2014, 2:49 p.m.) > > > Review request for Plasma. > > > Repository: plasma-desktop > > > Description > --- > > Remove combo "Graphical Effects" > > It set a config value which was exposed in > KGlobalSettings::graphicEffectsLevel that is now deprecated. > > > Diffs > - > > kcms/style/finetuning.ui 3abb692 > kcms/style/kcmstyle.cpp 6585ee6 > > Diff: https://git.reviewboard.kde.org/r/120970/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel