Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/
---

(Updated Feb. 28, 2014, 3:23 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Repository: plasma-framework


Description
---

The rationale behind this patch is on the mailing list in the thread "Minutes 
Monday" 

This doesn't boost performance or save memory much, but it paves the way for 
texture sharing, faster resizing, and plenty of other things.

Based on Frederick's comment I have reverted my changes to use QImage 
everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
Changes to plasmacore are minimal.

I'm currently porting FrameSVG which is where we should see more gains, but I 
thought I should get this reviewed/merged in parallel.

I have only seen one regression which is in the analog clock.
Some odd code in the analog clock (by me apparently!) means the width is 
dependent on the current width, which due to some changes in this patch ends up 
in a constant spiral getting to infinitely sized and explode.  


Changelog (in reverse order):
Remove manual isDirty tracking in SvgItem
Always resize the node geometry on resizes
Update to paint to fill the size of the object, not the size of texture
Fix leaking texture
Add convenient QImage image() getter in SVG
Avoid repainting if node is not changed
Render SvgItem natively rather than going through QQuickPaintedItem


Diffs
-

  src/declarativeimports/core/svgitem.h c8be7cc 
  src/declarativeimports/core/svgitem.cpp e90751a 
  src/plasma/svg.h 01d98f8 
  src/plasma/svg.cpp 9ec2aa5 

Diff: https://git.reviewboard.kde.org/r/115923/diff/


Testing
---


Thanks,

David Edmundson

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review51343
---


This review has been submitted with commit 
66bac622b4e2a268098d59b3ab708b18634a362a by David Edmundson to branch master.

- Commit Hook


On Feb. 21, 2014, 3:32 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 3:32 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review51342
---


[16:00]  so, if you are sure nothing is leaking (ii still not have 
100% how is supposed to be).. then ship it for me


- David Edmundson


On Feb. 21, 2014, 3:32 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 3:32 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

> On Feb. 21, 2014, 11:18 p.m., Sebastian Kügler wrote:
> > I've run your native_render_frame branch for a bit, some observations on my 
> > system:
> > 
> > - It mostly works
> > - taskbar doesn't find some elements in the svg, leading to lots of 
> > messages like this:
> > plasma_shell(11872)/default QSvgTinyDocument::draw: Couldn't find node 
> > normalbottomright. Skipping rendering.
> > - we seem to be hitting a few slow code pathes, which mean that I'm getting 
> > some smaller and longer freezes
> > - memory usage has gone up quite a bit, from 178MB to 328MB
> > - I'm seeing lots of threads of plasma-shell in ps
> > 
> > I like the idea, though, and I think those issues are something that can be 
> > sorted out. Looking at the code, it looks pretty clean so far, so nice work!
> 
> David Edmundson wrote:
> Aye, it turns out there's a reason Plasma::FrameSVG is really big; and 
> I'm basically recreating the most complex method inside that branch. Note 
> that branch contains a lot more stuff than that's in this review. 
> 
> I need to fold back the things I've improved/learned from this review 
> into that frame branch.
> 
> I'm going to try to IconItem next I think, it's less menacing, but 
> contains an animation which is exciting.

This code is pushed in the branch davidedmundson/svgrendering for testing.

it is _not_ the same as the native_render_frame branch. 


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50494
---


On Feb. 21, 2014, 3:32 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 3:32 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Marco Martin]

On Tuesday 25 February 2014, David Edmundson wrote:
> Is that with the branch or with this patch? They are quite diverged at
> this point?

with the branch
-- 
Marco Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

Is that with the branch or with this patch? They are quite diverged at
this point?
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

> On Feb. 21, 2014, 11:18 p.m., Sebastian Kügler wrote:
> > I've run your native_render_frame branch for a bit, some observations on my 
> > system:
> > 
> > - It mostly works
> > - taskbar doesn't find some elements in the svg, leading to lots of 
> > messages like this:
> > plasma_shell(11872)/default QSvgTinyDocument::draw: Couldn't find node 
> > normalbottomright. Skipping rendering.
> > - we seem to be hitting a few slow code pathes, which mean that I'm getting 
> > some smaller and longer freezes
> > - memory usage has gone up quite a bit, from 178MB to 328MB
> > - I'm seeing lots of threads of plasma-shell in ps
> > 
> > I like the idea, though, and I think those issues are something that can be 
> > sorted out. Looking at the code, it looks pretty clean so far, so nice work!

Aye, it turns out there's a reason Plasma::FrameSVG is really big; and I'm 
basically recreating the most complex method inside that branch. Note that 
branch contains a lot more stuff than that's in this review. 

I need to fold back the things I've improved/learned from this review into that 
frame branch.

I'm going to try to IconItem next I think, it's less menacing, but contains an 
animation which is exciting.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50494
---


On Feb. 21, 2014, 3:32 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 3:32 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Marco Martin]

On Monday 24 February 2014, Marco Martin wrote:
> here the main problem seems that is broken the rendering of a single svg
> element.
> so for things like the expander arrows in the taskbar, the whole svg is
> rendered and the sizes of the items seems also to be reported incorrectly
> (so for instance my panel toolbox isn't usable because has size 0

that's what's happening in native_render_frame (note the dolphin entry)
http://wstaw.org/m/2014/02/24/plasma-desktopQt1685.png

does anybody encountered it as well?

-- 
Marco Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Marco Martin]

On Saturday 22 February 2014, Sebastian Kügler wrote:
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/#review50494
> ---
> 
> 
> I've run your native_render_frame branch for a bit, some observations on my
> system:
> 
> - It mostly works
> - taskbar doesn't find some elements in the svg, leading to lots of
> messages like this: plasma_shell(11872)/default QSvgTinyDocument::draw:
> Couldn't find node normalbottomright. Skipping rendering. - we seem to be

here the main problem seems that is broken the rendering of a single svg 
element.
so for things like the expander arrows in the taskbar, the whole svg is 
rendered and the sizes of the items seems also to be reported incorrectly (so 
for instance my panel toolbox isn't usable because has size 0

-- 
Marco Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Sebastian Kügler]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50494
---


I've run your native_render_frame branch for a bit, some observations on my 
system:

- It mostly works
- taskbar doesn't find some elements in the svg, leading to lots of messages 
like this:
plasma_shell(11872)/default QSvgTinyDocument::draw: Couldn't find node 
normalbottomright. Skipping rendering.
- we seem to be hitting a few slow code pathes, which mean that I'm getting 
some smaller and longer freezes
- memory usage has gone up quite a bit, from 178MB to 328MB
- I'm seeing lots of threads of plasma-shell in ps

I like the idea, though, and I think those issues are something that can be 
sorted out. Looking at the code, it looks pretty clean so far, so nice work! 

- Sebastian Kügler


On Feb. 21, 2014, 3:32 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 3:32 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/
---

(Updated Feb. 21, 2014, 3:32 p.m.)


Review request for Plasma.


Changes
---

It's a learning experience...
Moved the SVG updating outside the paint loop.

Prevents a flicker.


Repository: plasma-framework


Description
---

The rationale behind this patch is on the mailing list in the thread "Minutes 
Monday" 

This doesn't boost performance or save memory much, but it paves the way for 
texture sharing, faster resizing, and plenty of other things.

Based on Frederick's comment I have reverted my changes to use QImage 
everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
Changes to plasmacore are minimal.

I'm currently porting FrameSVG which is where we should see more gains, but I 
thought I should get this reviewed/merged in parallel.

I have only seen one regression which is in the analog clock.
Some odd code in the analog clock (by me apparently!) means the width is 
dependent on the current width, which due to some changes in this patch ends up 
in a constant spiral getting to infinitely sized and explode.  


Changelog (in reverse order):
Remove manual isDirty tracking in SvgItem
Always resize the node geometry on resizes
Update to paint to fill the size of the object, not the size of texture
Fix leaking texture
Add convenient QImage image() getter in SVG
Avoid repainting if node is not changed
Render SvgItem natively rather than going through QQuickPaintedItem


Diffs (updated)
-

  src/declarativeimports/core/svgitem.h c8be7cc 
  src/declarativeimports/core/svgitem.cpp e90751a 
  src/plasma/svg.h 01d98f8 
  src/plasma/svg.cpp 9ec2aa5 

Diff: https://git.reviewboard.kde.org/r/115923/diff/


Testing
---


Thanks,

David Edmundson

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/
---

(Updated Feb. 21, 2014, 2:15 p.m.)


Review request for Plasma.


Repository: plasma-framework


Description
---

The rationale behind this patch is on the mailing list in the thread "Minutes 
Monday" 

This doesn't boost performance or save memory much, but it paves the way for 
texture sharing, faster resizing, and plenty of other things.

Based on Frederick's comment I have reverted my changes to use QImage 
everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
Changes to plasmacore are minimal.

I'm currently porting FrameSVG which is where we should see more gains, but I 
thought I should get this reviewed/merged in parallel.

I have only seen one regression which is in the analog clock.
Some odd code in the analog clock (by me apparently!) means the width is 
dependent on the current width, which due to some changes in this patch ends up 
in a constant spiral getting to infinitely sized and explode.  


Changelog (in reverse order):
Remove manual isDirty tracking in SvgItem
Always resize the node geometry on resizes
Update to paint to fill the size of the object, not the size of texture
Fix leaking texture
Add convenient QImage image() getter in SVG
Avoid repainting if node is not changed
Render SvgItem natively rather than going through QQuickPaintedItem


Diffs (updated)
-

  src/declarativeimports/core/svgitem.cpp e90751a 
  src/plasma/svg.h 01d98f8 
  src/plasma/svg.cpp 9ec2aa5 
  src/declarativeimports/core/svgitem.h c8be7cc 

Diff: https://git.reviewboard.kde.org/r/115923/diff/


Testing
---


Thanks,

David Edmundson

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Martin Gräßlin]

> On Feb. 20, 2014, 9:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 133
> > 
> >
> > Q_NULLPTR

it's still in the latest diff


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---


On Feb. 21, 2014, 12:45 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 12:45 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
>   src/declarativeimports/core/svgitem.h c8be7cc 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Martin Gräßlin]

> On Feb. 20, 2014, 9:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 127
> > 
> >
> > nitpick: looking at the surrounding coding style the * should be moved 
> > from type to name
> 
> David Edmundson wrote:
> I'm really not sure which Martin is worse...
> /me grumbles and fixes it.
> 
> Martin Klapetek wrote:
> There is a coding style defined and it's there so the resulting code, 
> shared with the whole world and worked on by many people, does not look like 
> it went through set of earthquakes :P
> 
> David Edmundson wrote:
> If I ever learn how to add astyle to my pre-commit hooks, you are going 
> to be out of a job.

if you do, please share :-)


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---


On Feb. 21, 2014, 12:45 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 21, 2014, 12:45 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
>   src/declarativeimports/core/svgitem.h c8be7cc 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50424
---



src/declarativeimports/core/svgitem.cpp


I need to redraw if the theme changes, I am going to need to restore my 
flag in SvgItem to say if the texture needs repainting.
Will add in the morning.


- David Edmundson


On Feb. 20, 2014, 11:45 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 11:45 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
>   src/declarativeimports/core/svgitem.h c8be7cc 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50423
---



src/declarativeimports/core/svgitem.h


This is now removed.


- David Edmundson


On Feb. 20, 2014, 11:45 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 11:45 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
>   src/declarativeimports/core/svgitem.h c8be7cc 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/
---

(Updated Feb. 20, 2014, 11:45 p.m.)


Review request for Plasma.


Repository: plasma-framework


Description
---

The rationale behind this patch is on the mailing list in the thread "Minutes 
Monday" 

This doesn't boost performance or save memory much, but it paves the way for 
texture sharing, faster resizing, and plenty of other things.

Based on Frederick's comment I have reverted my changes to use QImage 
everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
Changes to plasmacore are minimal.

I'm currently porting FrameSVG which is where we should see more gains, but I 
thought I should get this reviewed/merged in parallel.

I have only seen one regression which is in the analog clock.
Some odd code in the analog clock (by me apparently!) means the width is 
dependent on the current width, which due to some changes in this patch ends up 
in a constant spiral getting to infinitely sized and explode.  


Changelog (in reverse order):
Remove manual isDirty tracking in SvgItem
Always resize the node geometry on resizes
Update to paint to fill the size of the object, not the size of texture
Fix leaking texture
Add convenient QImage image() getter in SVG
Avoid repainting if node is not changed
Render SvgItem natively rather than going through QQuickPaintedItem


Diffs (updated)
-

  src/declarativeimports/core/svgitem.cpp e90751a 
  src/plasma/svg.h 01d98f8 
  src/plasma/svg.cpp 9ec2aa5 
  src/declarativeimports/core/svgitem.h c8be7cc 

Diff: https://git.reviewboard.kde.org/r/115923/diff/


Testing
---


Thanks,

David Edmundson

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 45
> > 
> >
> > are you sure that you can delete the texture here? Is the object being 
> > destroyed from the rendering thread?
> 
> David Edmundson wrote:
> It's a good question, but I think it's OK. 
> 
> I would have thought they'd have some very strong guards against deleting 
> a QQuickItem whilst they're still trying to use it. That would cause people 
> other major problems. 
> 
> The texture inherits from QObject so I could call deleteLater() instead? 
> Alternately I can subclass the node and delete the texture in the destructor 
> of the node, which would be quite tidy.
> 
> Hopefully in a week or so I'll be able to give you a far more concrete 
> answer when I actually understand how this area works.
> 
> Martin Gräßlin wrote:
> > The texture inherits from QObject so I could call deleteLater() instead?
> 
> I'm not sure whether that would help. The gl destruction code needs to be 
> called from the rendering thread, which is not necessarily the same thread as 
> which created the texture.
> 
> I just checked how I solved this problem in the WindowThumbnailItem and 
> it looks like I created my own QSGSimpleTextureNode which cleans up. Of 
> course that assumes that the node gets cleaned up in the right way.
> 
> /me is a little bit unhappy with the Qt documentation around it. 
> Especially who has ownership over what.

In this case it's always true, as we create in updatePaintNode which is always 
in the render thread.
I'll subclass anyway, will save me having deletes in two places.


> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 127
> > 
> >
> > nitpick: looking at the surrounding coding style the * should be moved 
> > from type to name
> 
> David Edmundson wrote:
> I'm really not sure which Martin is worse...
> /me grumbles and fixes it.
> 
> Martin Klapetek wrote:
> There is a coding style defined and it's there so the resulting code, 
> shared with the whole world and worked on by many people, does not look like 
> it went through set of earthquakes :P

If I ever learn how to add astyle to my pre-commit hooks, you are going to be 
out of a job.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---


On Feb. 20, 2014, 7:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 7:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Martin Klapetek]

> On Feb. 20, 2014, 9:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 127
> > 
> >
> > nitpick: looking at the surrounding coding style the * should be moved 
> > from type to name
> 
> David Edmundson wrote:
> I'm really not sure which Martin is worse...
> /me grumbles and fixes it.

There is a coding style defined and it's there so the resulting code, shared 
with the whole world and worked on by many people, does not look like it went 
through set of earthquakes :P


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---


On Feb. 20, 2014, 8:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 8:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Martin Gräßlin]

> On Feb. 20, 2014, 9:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 45
> > 
> >
> > are you sure that you can delete the texture here? Is the object being 
> > destroyed from the rendering thread?
> 
> David Edmundson wrote:
> It's a good question, but I think it's OK. 
> 
> I would have thought they'd have some very strong guards against deleting 
> a QQuickItem whilst they're still trying to use it. That would cause people 
> other major problems. 
> 
> The texture inherits from QObject so I could call deleteLater() instead? 
> Alternately I can subclass the node and delete the texture in the destructor 
> of the node, which would be quite tidy.
> 
> Hopefully in a week or so I'll be able to give you a far more concrete 
> answer when I actually understand how this area works.

> The texture inherits from QObject so I could call deleteLater() instead?

I'm not sure whether that would help. The gl destruction code needs to be 
called from the rendering thread, which is not necessarily the same thread as 
which created the texture.

I just checked how I solved this problem in the WindowThumbnailItem and it 
looks like I created my own QSGSimpleTextureNode which cleans up. Of course 
that assumes that the node gets cleaned up in the right way.

/me is a little bit unhappy with the Qt documentation around it. Especially who 
has ownership over what.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---


On Feb. 20, 2014, 8:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 8:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 132
> > 
> >
> > I never really understood who takes ownership over the nodes: does one 
> > have to delete the oldNode?

It very much seems so. QQuickText does it for example if d->text is empty. Same 
for other QQuick classes.


> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 45
> > 
> >
> > are you sure that you can delete the texture here? Is the object being 
> > destroyed from the rendering thread?

It's a good question, but I think it's OK. 

I would have thought they'd have some very strong guards against deleting a 
QQuickItem whilst they're still trying to use it. That would cause people other 
major problems. 

The texture inherits from QObject so I could call deleteLater() instead? 
Alternately I can subclass the node and delete the texture in the destructor of 
the node, which would be quite tidy.

Hopefully in a week or so I'll be able to give you a far more concrete answer 
when I actually understand how this area works.


> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 127
> > 
> >
> > nitpick: looking at the surrounding coding style the * should be moved 
> > from type to name

I'm really not sure which Martin is worse...
/me grumbles and fixes it.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---


On Feb. 20, 2014, 7:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 7:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

> On Feb. 20, 2014, 7:54 p.m., Marco Martin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 138
> > 
> >
> > is this guaranteed to be always deleted?

I believe so.

There's a flag on QGSNode QSGNode::OwnedByParent
this is set by default.

The QQuickItem documentation example and tests appear to be the same.


> On Feb. 20, 2014, 7:54 p.m., Marco Martin wrote:
> > src/plasma/svg.h, line 109
> > 
> >
> > this doesn't seem to do a much useful thing?

It's a convenient wrapper round svg->pixmap().toImage();

Given I'm going to use it from more places, it seems like it'll keep things 
neater. It will also allows me to add things I need for the rest of porting 
without breaking the current API everyone else is using.

For my FrameSVG stuff, I need to pass a QSize here.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50401
---


On Feb. 20, 2014, 7:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 7:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
---



src/declarativeimports/core/svgitem.cpp


looks like QPainter include could be removed



src/declarativeimports/core/svgitem.cpp


are you sure that you can delete the texture here? Is the object being 
destroyed from the rendering thread?



src/declarativeimports/core/svgitem.cpp


nitpick: looking at the surrounding coding style the * should be moved from 
type to name



src/declarativeimports/core/svgitem.cpp


nitpick: Q_UNUSED doesn't need a ;



src/declarativeimports/core/svgitem.cpp


I never really understood who takes ownership over the nodes: does one have 
to delete the oldNode?



src/declarativeimports/core/svgitem.cpp


Q_NULLPTR



src/plasma/svg.cpp


nitpick: empty lines


- Martin Gräßlin


On Feb. 20, 2014, 8:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 8:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50401
---


pretty much good to go i think


src/declarativeimports/core/svgitem.cpp


is this guaranteed to be always deleted?



src/plasma/svg.h


this doesn't seem to do a much useful thing?


- Marco Martin


On Feb. 20, 2014, 7:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 7:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50398
---



src/declarativeimports/core/svgitem.cpp


"and size is approximately the same"


- David Edmundson


On Feb. 20, 2014, 7:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> ---
> 
> (Updated Feb. 20, 2014, 7:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes 
> Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for 
> texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage 
> everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I 
> thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is 
> dependent on the current width, which due to some changes in this patch ends 
> up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/
---

Review request for Plasma.


Repository: plasma-framework


Description
---

The rationale behind this patch is on the mailing list in the thread "Minutes 
Monday" 

This doesn't boost performance or save memory much, but it paves the way for 
texture sharing, faster resizing, and plenty of other things.

Based on Frederick's comment I have reverted my changes to use QImage 
everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
Changes to plasmacore are minimal.

I'm currently porting FrameSVG which is where we should see more gains, but I 
thought I should get this reviewed/merged in parallel.

I have only seen one regression which is in the analog clock.
Some odd code in the analog clock (by me apparently!) means the width is 
dependent on the current width, which due to some changes in this patch ends up 
in a constant spiral getting to infinitely sized and explode.  


Changelog (in reverse order):
Remove manual isDirty tracking in SvgItem
Always resize the node geometry on resizes
Update to paint to fill the size of the object, not the size of texture
Fix leaking texture
Add convenient QImage image() getter in SVG
Avoid repainting if node is not changed
Render SvgItem natively rather than going through QQuickPaintedItem


Diffs
-

  src/declarativeimports/core/svgitem.h c8be7cc 
  src/declarativeimports/core/svgitem.cpp e90751a 
  src/plasma/svg.h 01d98f8 
  src/plasma/svg.cpp 9ec2aa5 

Diff: https://git.reviewboard.kde.org/r/115923/diff/


Testing
---


Thanks,

David Edmundson

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel