<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/128465/">https://git.reviewboard.kde.org/r/128465/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On juli 16th, 2016, 9:44 p.m. UTC, <b>Mark Gaiser</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/128465/diff/1/?file=471855#file471855line342" style="color: black; font-weight: bold; text-decoration: underline;">src/kiconloader.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">342</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">mLastUnknownIconCheck</span><span class="p">.</span><span class="n">isValid</span><span class="p">()</span> <span class="o">&&</span> <span class="n">mLastUnknownIconCheck</span><span class="p">.</span><span class="n">elapsed</span><span class="p">()</span> <span class="o"><</span> <span class="n">kiconloader_ms_between_checks</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<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">343</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="nb">false</span><span class="p">;</span></pre></td>
</tr>
<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">344</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
<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">345</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mLastUnknownIconCheck</span><span class="p">.</span><span class="n">start</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;">I don't know if this is as you intent it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The call mLastUnknownIconCheck.start(); starts the timer, but does it "restart" the timer if it was already started after the 5 seconds check?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The QElapsedTimer::start and QElapsedTimer::restart functions are implemented differently (see http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qelapsedtimer_unix.cpp for reference). I havent't tested if start and restart behave the same. If the do then your code does what you intent. But then i wonder why QElapsedTimer has two different implementations for the same logic.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If start doesn't restart when called again, then your code - after the 5 seconds define - always ends up outside the if. In that case replacing mLastUnknownIconCheck.start(); with mLastUnknownIconCheck.restart(); would make it work as intended.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ahh, vague.. If you look at the qelapsedtimer_generic.cpp then you see that start just calls restart (is that being used for Linux or is qelapsedtimer_unix.cpp being used? This confuses me..). Anyhow, if the generic implementation is used then you're fine with the code as is :)</p></pre>
</blockquote>
<p>On juli 16th, 2016, 10:45 p.m. UTC, <b>David Faure</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;">src/corelib/tools/tools.pri is clear: on unix, qelapsedtimer_unix.cpp is used. In any case the intent is for this code to be cross-platform ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm using this same code in ksycoca and I heavily tested it there, I would doubt that this code never restarts the timer. But let's dig in to be sure.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The documentation for "qint64 restart()" is: "restarts the timer and returns the time elapsed since the previous start. This function is equivalent to obtaining the elapsed time with elapsed() and then starting the timer again with start(), but it does so in one single operation, avoiding the need to obtain the clock value twice."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So clearly start() can be used on an already running QElapsedTimer, it simply restarts it without returning the current elapsed time.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Seems fine to me.</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;">Sounds like it's all ok then :)
Thank you for digging into that.
+1</p></pre>
<br />
<p>- Mark</p>
<br />
<p>On juli 16th, 2016, 9:52 a.m. UTC, David Faure 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, Christoph Feck, David Rosca, Michael Pyne, and Olivier Goffart.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated jul 16, 2016, 9:52 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kiconthemes
</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;">Summary:
The code said "unknown icons should be searched for anew each time
[so that installing new icons works]". However this leads to massive
performance issues when opening the file dialog in a dir with many
files for which there is no mimetype icon.
In my case, 293 callgrind.out.* files in one dir (it's ironic that
they would create a huge performance issue...).
To make both sides happy (installing new icons should still work, but
still unknown icons shouldn't lead to a full disk search every time
icon.isNull() or icon.pixmap() is called), let's re-resolve unknown
icons again only after 5 seconds.
Benchmark results for loading an unavailable icon :
before: 43 msecs per iteration (1897 disk locations checked)
after: 0.013 msecs per iteration (pixmap found in the on-disk cache)
And the file dialog no longer crawls to a halt in the dir with 293 callgrind files.
Test Plan:
Reviewers:
Subscribers:</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;">(described in commit log)</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>autotests/kiconengine_unittest.cpp <span style="color: grey">(105e86c1e7bc6170c626aa80d34cdb6422acca9c)</span></li>
<li>src/kiconloader.cpp <span style="color: grey">(d885318c166f2a996b038218366317615886a14e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128465/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>