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
> > <https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line132>
> >
> >     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
> > <https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line45>
> >
> >     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
> > <https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line127>
> >
> >     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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140220/67b954a6/attachment-0001.html>