<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/119425/">https://git.reviewboard.kde.org/r/119425/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 24th, 2014, 10:55 a.m. UTC, <b>David Edmundson</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/119425/diff/2/?file=292260#file292260line45" style="color: black; font-weight: bold; text-decoration: underline;">src/declarativeimports/core/framesvgitem.cpp</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">45</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qApp</span><span class="o">-></span><span class="n">installEventFilter</span><span class="p">(</span><span class="k">this</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;">Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this.</p></pre>
</blockquote>
<p>On July 24th, 2014, 11:03 a.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;">uhm, why a filter on palette change?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
this would work for themes that use system colors but not the other ones.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
theme::themechanged should cover both cases (and if it doesn't, it should)</p></pre>
</blockquote>
<p>On July 24th, 2014, 11:13 a.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;">See SvgPrivate::checkColorHints().</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree it could make sense having it in plasma theme, but this should be part of another patch.</p></pre>
</blockquote>
<p>On July 24th, 2014, 11:25 a.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;">it does a repaintneeded, that is correct.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
but yeah, having it in theme doesn't make much sense, because technically the theme didn't change and you can't know from there if one of the svgs actually needs a repaint.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so, what all of this suggests me is that the thing that would make most sense is actually use repaintneeded, and either:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
a) just remove from the hash the id of this texture (multiple removes still possible tough)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
b) event compress the clear()<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
c) both of the above</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;">I'm getting a bit lost, sorry. Why is listening to all svg's better? RepaintNeeded will emit upon signals we don't need. Note that themeChanged doesn't clean the cache and it will rather be the textures just stopping to use the old theme, because the hash is refcounted. That's why I introduced more elements to the hash key.</p></pre>
<br />
<p>- Aleix</p>
<br />
<p>On July 24th, 2014, 11:14 a.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 Plasma.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated July 24, 2014, 11:14 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</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;">Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it.</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;">see the qDebug (to be removed before commit). </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">plasmashell 2> out<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
$ grep s_cache out | grep ": miss" | wc -l<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
342<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
$ grep s_cache out | grep ": hit" | wc -l<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
126</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis.</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/declarativeimports/core/framesvgitem.cpp <span style="color: grey">(323b06b)</span></li>
<li>src/declarativeimports/core/iconitem.cpp <span style="color: grey">(38012cc)</span></li>
<li>src/declarativeimports/core/svgitem.cpp <span style="color: grey">(eccff55)</span></li>
<li>src/declarativeimports/core/svgtexturenode.h <span style="color: grey">(21b9b2f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119425/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>