[Differential] [Commented On] D4414: don't regenerate frames when setting every property

[David Rosca]

drosca added a comment.


  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.
  
  F2497425: Spectacle.TJ5519.png 

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D4414

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: drosca, davidedmundson, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4414: don't regenerate frames when setting every property

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> mart wrote in framesvg.cpp: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

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.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D4414

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4414: don't regenerate frames when setting every property

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> davidedmundson wrote in framesvg.cpp: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?

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

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D4414

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol