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
