D8917: Reduce the amount of spurious property changes on ColorScope
dfaure added inline comments. INLINE COMMENTS > colorscope.h:131 > +QObject *const m_parent; > +Plasma::Theme::ColorGroup m_actualGroup; > valgrind says this member isn't initialized, please fix. http://www.davidfaure.fr/2018/colorscope_valgrind_log.txt REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8917 To: apol, #plasma, mart, davidedmundson Cc: dfaure, davidedmundson, plasma-devel, #frameworks, michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
This revision was automatically updated to reflect the committed changes. Closed by commit R242:f8047e86b298: Reduce the amount of spurious property changes on ColorScope (authored by apol). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8917?vs=23233=23268 REVISION DETAIL https://phabricator.kde.org/D8917 AFFECTED FILES src/declarativeimports/core/colorscope.cpp src/declarativeimports/core/colorscope.h To: apol, #plasma, mart, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
apol updated this revision to Diff 23233. apol added a comment. Fix david's comment REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8917?vs=23202=23233 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8917 AFFECTED FILES src/declarativeimports/core/colorscope.cpp src/declarativeimports/core/colorscope.h To: apol, #plasma, mart, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
mart added a comment. ah, you are right, yes, it should do checkcologgroupchanged instead REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8917 To: apol, #plasma, mart, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
davidedmundson added inline comments. INLINE COMMENTS > mart wrote in colorscope.cpp:193 > this is when the item changes window and we're not sure we are still in the > same color set, so i think is ok to keep this signal yeah, my point was we may /also/ need the colorsChanged signal (via checkColorGroupChanged) REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8917 To: apol, #plasma, mart, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
mart accepted this revision. mart added inline comments. INLINE COMMENTS > davidedmundson wrote in colorscope.cpp:193 > check here. this is when the item changes window and we're not sure we are still in the same color set, so i think is ok to keep this signal REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8917 To: apol, #plasma, mart, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
davidedmundson added a comment.
description needs updating with the new benefits (saving lookups every time)
INLINE COMMENTS
> colorscope.cpp:193
> if (value.window) {
> emit colorGroupChanged();
> }
check here.
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D8917
To: apol, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
apol updated this revision to Diff 23202. apol added a comment. Move to tracking parents instead of doing a look-up on every color get REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8917?vs=22664=23202 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8917 AFFECTED FILES src/declarativeimports/core/colorscope.cpp src/declarativeimports/core/colorscope.h To: apol, #plasma, mart, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.
Concept is good.
INLINE COMMENTS
> colorscope.cpp:127
> }
> +m_lastGroup = m_group;
> return m_group;
It's weird to be caching in a public getter.
It opens us up for problems
If some client code (for whatever reason) did
connect(scope, inheritChanged, []() {scope->colorGroup());
then our signals in checkColorGroupChanged won't get emitted.
Can we move this member var into checkColorGroupChanged? Means we can get rid
of the mutable too
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D8917
To: apol, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8917: Reduce the amount of spurious property changes on ColorScope
apol created this revision. apol added reviewers: Plasma, mart. Restricted Application added projects: Plasma, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY At the moment whenever something changed we were emitting colorGroupChanged and then every color would recompute. This would end up being emitted over 10 times at plasma startup so far. This patch makes sure that the property will only be emitted if the color group actually changes. TEST PLAN Ran plasma, didn't notice issues. I don't see all of the changes on the property anymore REPOSITORY R242 Plasma Framework (Library) BRANCH master REVISION DETAIL https://phabricator.kde.org/D8917 AFFECTED FILES src/declarativeimports/core/colorscope.cpp src/declarativeimports/core/colorscope.h To: apol, #plasma, mart Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
