D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
This revision was automatically updated to reflect the committed changes. Closed by commit R119:561401636f7d: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound… (authored by crossi). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25220?vs=69635=70279 REVISION DETAIL https://phabricator.kde.org/D25220 AFFECTED FILES kcms/style/kcmstyle.cpp kcms/style/kcmstyle.h To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
ervin accepted this revision. ervin added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > crossi wrote in kcmstyle.cpp:245 > m_effectsDirty is checked line 291, to trigger or not a refresh of all > applications. > > This check before save is now useless, since if we can save, it means a > modification has occured, style or effects. Not for this patch, but I still wonder if we could kill it. I think it'd be better to have it as a local variable whose value would be computed from the state of the relevant items in the settings object. This would remove the logic of maintaining an extra state exploited only in save(). Also "effectsDirty" is a weird name I think, it doesn't seem related to any concept of effects. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D25220 To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
crossi updated this revision to Diff 69635. crossi added a comment. style + consistency REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25220?vs=69468=69635 REVISION DETAIL https://phabricator.kde.org/D25220 AFFECTED FILES kcms/style/kcmstyle.cpp kcms/style/kcmstyle.h To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
ervin added inline comments. INLINE COMMENTS > crossi wrote in kcmstyle.cpp:248 > m_model->selectedStyle() is bound to m_settings->widgetStyle() and vice > versa, so I need to save previous state to allow a rollback. > I should have used m_settings->widgetStyle() here and after for consistency. As far as rollback goes (might not be enough in your case), wouldn't checking for isSaveNeeded() on the settings and rollbacking it all be enough? We can't rollback a single item though, I guess that would be the main limitation in your case... Maybe that's something to add to KConfigSkeletonItem? Answer might be no to both, I'm just looking for opportunities to simplify code in modules. :-) REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D25220 To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
crossi added inline comments. INLINE COMMENTS > ervin wrote in kcmstyle.cpp:98 > Space before &, not after. Will fix > ervin wrote in kcmstyle.cpp:245 > AFAICT this is the only place where m_effectsDirty is read. So I think you > can remove that flag altogether, which should bring further simplifications. m_effectsDirty is checked line 291, to trigger or not a refresh of all applications. This check before save is now useless, since if we can save, it means a modification has occured, style or effects. > ervin wrote in kcmstyle.cpp:248 > Are you sure you need this m_previousStyle? I'd expect it to be exactly > m_settings->widgetStyle(). m_model->selectedStyle() is bound to m_settings->widgetStyle() and vice versa, so I need to save previous state to allow a rollback. I should have used m_settings->widgetStyle() here and after for consistency. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D25220 To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
ervin added inline comments.
INLINE COMMENTS
> kcmstyle.cpp:98
>
> -connect(m_model, ::selectedStyleChanged, this, [this] {
> -m_selectedStyleDirty = true;
> -setNeedsSave(true);
> +connect(m_model, ::selectedStyleChanged, this, [this](const
> QString& style) {
> +m_settings->setWidgetStyle(style);
Space before &, not after.
> kcmstyle.cpp:245
> {
> -if (!m_selectedStyleDirty && !m_effectsDirty) {
> -return;
AFAICT this is the only place where m_effectsDirty is read. So I think you can
remove that flag altogether, which should bring further simplifications.
> kcmstyle.cpp:248
> bool newStyleLoaded = false;
> -if (m_selectedStyleDirty) {
> +if (m_model->selectedStyle() != m_previousStyle) {
> QScopedPointer
> newStyle(QStyleFactory::create(m_model->selectedStyle()));
Are you sure you need this m_previousStyle? I'd expect it to be exactly
m_settings->widgetStyle().
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D25220
To: crossi, #plasma, ervin, bport, mart, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2,
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
crossi updated this revision to Diff 69468. crossi added a comment. missed that one REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25220?vs=69467=69468 REVISION DETAIL https://phabricator.kde.org/D25220 AFFECTED FILES kcms/style/kcmstyle.cpp kcms/style/kcmstyle.h To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
crossi updated this revision to Diff 69467. crossi added a comment. make access to selectedStyle more consistent REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25220?vs=69466=69467 REVISION DETAIL https://phabricator.kde.org/D25220 AFFECTED FILES kcms/style/kcmstyle.cpp kcms/style/kcmstyle.h To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25220: KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
crossi created this revision. crossi added reviewers: Plasma, ervin, bport, mart, broulik. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. crossi requested review of this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D25220 AFFECTED FILES kcms/style/kcmstyle.cpp kcms/style/kcmstyle.h To: crossi, #plasma, ervin, bport, mart, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
