<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107358/">http://git.reviewboard.kde.org/r/107358/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for this improvement. I got bitten by the konsole/KUriFilter/FavIcon bug too, very annoying.
My attempt at fixing the situation was to make it possible to separate url resolving from getting the icon of the search provider, in KUriFilter, by making iconName() virtual.
Konsole only uses kurifilter to resolve urls, it doesn't care for the icons.
See 8394e75240a1ce57ae68b817a36be1bf642e7abf, but that's KF5 only, and of course kuriikwsfilter.cpp will need to be changed to make use of it, once kde-runtime uses KF5.</pre>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/107358/diff/1/?file=95107#file95107line488" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QString KMimeType::iconNameForUrl( const KUrl & _url, mode_t mode )</pre></td>
</tr>
</tbody>
<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">488</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="n">QHash</span><span class="o"><</span><span class="n">QUrl</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="n">m_iconNameCache</span><span class="o">=</span><span class="n">QHash</span><span class="o"><</span><span class="n">QUrl</span><span class="p">,</span><span class="n">QString</span><span class="o">></span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">the explicit call to the default constructor doesn't really bring much to the table...</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/107358/diff/1/?file=95107#file95107line495" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QString KMimeType::iconNameForUrl( const KUrl & _url, mode_t mode )</pre></td>
</tr>
</tbody>
<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">495</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">m_iconNameCache</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">url</span><span class="p">.</span><span class="n">url</span><span class="p">()))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Double-lookup. Use this instead:
const QString iconFromCache = m_iconNameCache.value(url.url());
if (!iconFromCache.isEmpty()) {
return iconFromCache;
}
Or if an empty value is valid in this cache (which I strongly suspect to be the case), pass a QString containing "NOTFOUND" as 2nd param to value() and compare with that on the line below.
Or if that's considered too ugly, do it properly, with a const_iterator and find() :)</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/107358/diff/1/?file=95107#file95107line503" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QString KMimeType::iconNameForUrl( const KUrl & _url, mode_t mode )</pre></td>
</tr>
</tbody>
<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">503</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_iconNameCache</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">url</span><span class="p">.</span><span class="n">url</span><span class="p">(),</span> <span class="n">result</span><span class="p">.</span><span class="n">value</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Of course this opens the whole issue of ... what if the favicon changes?
E.g. you're a web developer. Or you're talking to a sysadmin who tells you "fixed!"
OK, looking at the kded module, it also caches favicons, for one week, without any way to clean up the cache meanwhile. On the other hand, your QHash will get cleaned up by simply restarting the application (or the whole desktop, more likely, for the case of konqueror or konsole).
So no issue here, OK for the cache.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On November 17th, 2012, 10:56 a.m., Jaime Torres Amate wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Jaime Torres Amate.</div>
<p style="color: grey;"><i>Updated Nov. 17, 2012, 10:56 a.m.</i></p>
<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;">Does not fix the bugs (I'll not mark them as fixed), but makes them much harder to reproduce.
Just use a static QHash most of the time instead of a blocking Dbus call.</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;">In my tests, there are more than 5 thousands requests for a favicon answered by the local cache vs. less than new 50 favicons inserted.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=306338">306338</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=307973">307973</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecore/services/kmimetype.cpp <span style="color: grey">(955bf62)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107358/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>