> 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
