Reduce the amount of spurious property changes on ColorScope
ClosedPublic

Authored by apol on Nov 20 2017, 3:43 PM.

Details

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

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Nov 20 2017, 3:43 PM
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptNov 20 2017, 3:43 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
davidedmundson requested changes to this revision.Nov 30 2017, 8:48 AM
davidedmundson added a subscriber: davidedmundson.

Concept is good.

src/declarativeimports/core/colorscope.cpp
135

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

This revision now requires changes to proceed.Nov 30 2017, 8:48 AM
apol updated this revision to Diff 23202.Dec 1 2017, 9:17 AM

Move to tracking parents instead of doing a look-up on every color get

description needs updating with the new benefits (saving lookups every time)

src/declarativeimports/core/colorscope.cpp
192–193

check here.

mart accepted this revision.Dec 1 2017, 4:12 PM
mart added inline comments.
src/declarativeimports/core/colorscope.cpp
192–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

davidedmundson added inline comments.Dec 1 2017, 4:13 PM
src/declarativeimports/core/colorscope.cpp
192–193

yeah, my point was we may /also/ need the colorsChanged signal (via checkColorGroupChanged)

mart added a comment.Dec 1 2017, 4:15 PM

ah, you are right, yes, it should do checkcologgroupchanged instead

apol updated this revision to Diff 23233.Dec 1 2017, 8:21 PM

Fix david's comment

This revision was automatically updated to reflect the committed changes.
dfaure added a subscriber: dfaure.Feb 12 2018, 10:15 AM
dfaure added inline comments.
src/declarativeimports/core/colorscope.h
131

valgrind says this member isn't initialized, please fix.

http://www.davidfaure.fr/2018/colorscope_valgrind_log.txt