don't regenerate frames when setting every property
ClosedPublic

Authored by mart on Feb 2 2017, 5:55 PM.

Details

Summary

give frameSvg the concept of repaintBlocked(), that enables and
disables the regeneration of the frame data when a property is set.
the use case is when often, a lot of properties are set one after
the other (such as prefix, enabled borders, size)
collapse the formely similar, but a bit different logic of frame
regeneration is a single function for better maintanability.
QML FrameSvgItem sets repaintblocked when it starts and releases it just on oncomponentCompleted

Test Plan

plasmashell still starts, autotests still work, all frames are rendered correctly
the destruction of old frames is cutted by 50%. in the qml profiler
the creation time of a framesvgitem slightly improved, on this machine from around 26 msecs to around 21, can still be improved, but at least the code is a bit simpler

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.
mart updated this revision to Diff 10861.Feb 2 2017, 5:55 PM
mart retitled this revision from to don't regenerate frames when setting every property.
mart updated this object.
mart edited the test plan for this revision. (Show Details)
mart added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptFeb 2 2017, 5:55 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
davidedmundson added inline comments.
src/plasma/framesvg.cpp
135–136

I'm lost on why we have the pendingEnabledBorders

you have some bugs if you do:

setRepaintBlocked(false);
setEnabledBorders(Left)
enabledBorders() /// returns All not Left.

it'll get updated later but after the changed signal on the FrameSvgItem.

Do we need to know the old borders when we update? If so can we revert this logic to have a d->oldBorders that gets set at the end of the update method?

mart added inline comments.Feb 7 2017, 11:21 AM
src/plasma/framesvg.cpp
135–136

it has pendingEnabledBorders because right now the borders are saved only n the frame, that we don't know if we can keep it or we'll have to throw it away ( or just dereference because some other framesvg instance still needs it)
I don't like it that much as well, but i don't think the new value can be assigned right away.
and yes, when repaintblocked is true, it would return the old value... unless it would return pendingEnabledBorders in this case

davidedmundson added inline comments.Feb 7 2017, 11:54 AM
src/plasma/framesvg.cpp
135–136

That part makes sense now.

We still need to do something, otherwise if I have a Binding on a FrameSVGItem it's going to be broken.

I think we can just return d->pendingEnabledBorders rather than frame->enabledBorders (possibly renaming it)

the frame will never be different and it solves that problem simply.

mart updated this revision to Diff 11006.Feb 7 2017, 12:07 PM
  • just use enabledBorders to return as a property
This revision was automatically updated to reflect the committed changes.
drosca added a subscriber: drosca.Feb 15 2017, 6:54 PM

I now get tons of binding loop errors from FrameSvgItem, it also breaks delegates in networkmanager and bluetooth applets. On the screenshot you can see that the last 3 delegates (HUAWEI, UPC and Internet) are slightly moved to the left and hovering over them will correctly align them (as are the first 2 delegates on the screenshot).
I have Qt 5.8.