Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem
--- 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115923/#review51342 --- [16:00] notmart 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
--- 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
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
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
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
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
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
--- 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
--- 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
Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem
--- 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
Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115923/#review50398 --- src/declarativeimports/core/svgitem.cpp https://git.reviewboard.kde.org/r/115923/#comment35458 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
Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem
--- 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 https://git.reviewboard.kde.org/r/115923/#comment35460 is this guaranteed to be always deleted? src/plasma/svg.h https://git.reviewboard.kde.org/r/115923/#comment35461 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
On Feb. 20, 2014, 7:54 p.m., Marco Martin wrote: src/declarativeimports/core/svgitem.cpp, line 138 https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line138 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 https://git.reviewboard.kde.org/r/115923/diff/1/?file=245132#file245132line109 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
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 ___ 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
On Feb. 20, 2014, 9: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 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
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? 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 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 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
--- 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115923/#review50423 --- src/declarativeimports/core/svgitem.h https://git.reviewboard.kde.org/r/115923/#comment35490 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115923/#review50424 --- src/declarativeimports/core/svgitem.cpp https://git.reviewboard.kde.org/r/115923/#comment35491 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
On Feb. 20, 2014, 9: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 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
On Feb. 20, 2014, 9:13 p.m., Martin Gräßlin wrote: src/declarativeimports/core/svgitem.cpp, line 133 https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line133 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
