[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem
From: "Vishesh Handa" <me () vhanda ! in>
Date: 2014-10-14 15:33:09
Message-ID: 20141014153309.21223.14310 () probe ! kde ! org
[Download RAW message or body]
--===============0618436127281617869==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> 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
>
>
--===============0618436127281617869==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120550/">https://git.reviewboard.kde.org/r/120550/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 13th, 2014, 1:35 p.m. UTC, <b>Marco \
Martin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/quickaddons/managedtexturenode.h</a> <span style="font-weight: \
normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">52</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">QSharedPointer</span><span class="o"><</span><span \
class="n">QSGTexture</span><span class="o">></span> <span \
class="n">m_texture</span><span class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">even \
if will always remain just this member, just to me sure, it should be in a \
dpointer</p></pre> </blockquote>
<p>On October 13th, 2014, 5:54 p.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">If needs changed, then I'd suggest to create another class.</p></pre> \
</blockquote>
<p>On October 13th, 2014, 6:46 p.m. UTC, <b>Marco Martin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">if \
it's exported, i prefer a dpointer anyways</p></pre> </blockquote>
<p>On October 13th, 2014, 11:20 p.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">It's not only about memory usage. The memory gets defragmented as well.</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Also, maybe considering this class is so small, you \
may want to inline the function definitions.</p></pre> <br />
<p>- Vishesh</p>
<br />
<p>On October 13th, 2014, 5:54 p.m. UTC, Aleix Pol Gonzalez wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Frameworks, Plasma, David Edmundson, and Marco \
Martin.</div> <div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated Oct. 13, 2014, 5:54 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdeclarative
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Moves the caching types used in Plasma Workspace into \
a QuickAddons submodule.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">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.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt <span style="color: \
grey">(2977812)</span></li>
<li>src/qmlcontrols/kquickcontrolsaddons/qiconitem.h <span style="color: \
grey">(839a33f)</span></li>
<li>src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp <span style="color: \
grey">(0eb46b1)</span></li>
<li>src/quickaddons/CMakeLists.txt <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/quickaddons/imagetexturescache.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/quickaddons/imagetexturescache.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/quickaddons/managedtexturenode.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>src/quickaddons/managedtexturenode.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>tests/qiconitem_test.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(eb0dfd3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120550/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============0618436127281617869==--
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic