Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

[Vishesh Handa]

> On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
> > src/quickaddons/managedtexturenode.h, line 52
> > <https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52>
> >
> >     even if will always remain just this member, just to me sure, it should be in a dpointer
> 
> Aleix Pol Gonzalez wrote:
>     I don't think it's a good idea to add a d-pointer there. It's a class to encapsulate the object, I don't see why we should store more things in there.
>     
>     If needs changed, then I'd suggest to create another class.
> 
> Marco Martin wrote:
>     if it's exported, i prefer a dpointer anyways
> 
> Aleix Pol Gonzalez wrote:
>     Can we please discuss this? I really don't think we want to be so cheap when it comes to memory usage. This means that each node in the scene will take a payload for no much reason.

It's not only about memory usage. The memory gets defragmented as well.

Also, maybe considering this class is so small, you may want to inline the function definitions.


- Vishesh


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


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120550/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2014, 5:54 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> -------
> 
> Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
> 
> Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter item. Uses the same strategies used in Plasma Framework for caching the textures.
> 
> 
> Diffs
> -----
> 
>   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
>   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
>   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
>   src/quickaddons/CMakeLists.txt PRE-CREATION 
>   src/quickaddons/imagetexturescache.h PRE-CREATION 
>   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
>   src/quickaddons/managedtexturenode.h PRE-CREATION 
>   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
>   tests/qiconitem_test.qml PRE-CREATION 
>   src/CMakeLists.txt eb0dfd3 
> 
> Diff: https://git.reviewboard.kde.org/r/120550/diff/
> 
> 
> Testing
> -------
> 
> I added some manual test (that was impossible to run before the patch). Also tested it in KRunner and Muon Discover, which use the component. Everything still works.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20141014/885df697/attachment.html>