<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/125158/">https://git.reviewboard.kde.org/r/125158/</a>
</td>
</tr>
</table>
<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="https://git.reviewboard.kde.org/r/125158/diff/2/?file=403005#file403005line905" style="color: black; font-weight: bold; text-decoration: underline;">src/core/kfileitem.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">905</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mapping</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">QStandardPaths</span><span class="o">::</span><span class="n">DesktopLocation</span><span class="p">,</span> <span class="n">QLatin1Literal</span><span class="p">(</span><span class="s">"user-desktop"</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">This is much better done by a readonly array, in order to take less memory and less CPU time.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">static</span> <span style="color: #008000; font-weight: bold">const</span> <span style="color: #008000; font-weight: bold">struct</span> { QStandardPaths<span style="color: #666666">::</span>StandardLocation loc<span style="color: #666666">,</span> QString <span style="color: #008000; font-weight: bold">icon</span> } <span style="color: #008000; font-weight: bold">s_icons</span><span style="color: #BC7A00">[]</span> <span style="color: #666666">=</span> { <span style="border: 1px solid #FF0000">{</span> <span style="color: #666666">...,</span> <span style="color: #666666">...</span>} <span style="color: #666666">...</span> <span style="border: 1px solid #FF0000">}</span><span style="color: #666666">;</span>
<span style="color: #008000; font-weight: bold">static</span> <span style="color: #008000; font-weight: bold">const</span> <span style="color: #008000; font-weight: bold">int</span> <span style="color: #008000; font-weight: bold">s_iconsCount</span> <span style="color: #666666">=</span> <span style="color: #008000; font-weight: bold">sizeof</span> <span style="color: #008000; font-weight: bold">s_icons</span> <span style="color: #666666">/</span> <span style="color: #008000; font-weight: bold">sizeof</span> <span style="color: #666666">*</span><span style="color: #008000; font-weight: bold">s_icons</span><span style="color: #666666">;</span>
<span style="color: #008000; font-weight: bold">for</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">int</span> <span style="color: #008000; font-weight: bold">i</span> <span style="color: #666666">=</span> <span style="color: #008000; font-weight: bold">0</span> <span style="color: #666666">;</span> <span style="color: #008000; font-weight: bold">i</span> <span style="color: #666666"><</span> <span style="color: #008000; font-weight: bold">s_iconsCount</span><span style="color: #666666">;</span> <span style="color: #666666">++</span><span style="color: #008000; font-weight: bold">i</span><span style="color: #666666">)</span> {
<span style="color: #666666">...</span>
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(you don't even need the O(log n) lookup here, since the only use of the "map" is to iterate over it)</p></pre>
</div>
</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="https://git.reviewboard.kde.org/r/125158/diff/2/?file=403005#file403005line912" style="color: black; font-weight: bold; text-decoration: underline;">src/core/kfileitem.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">912</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mapping</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">QStandardPaths</span><span class="o">::</span><span class="n">DownloadLocation</span><span class="p">,</span> <span class="n">QLatin1Literal</span><span class="p">(</span><span class="s">"folder-download"</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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'm wondering about the case where DocumentsLocation == DownloadLocation, as it happens on many setups (IIRC).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It might be a good idea to make Documents win, rather than Download. This can be done by moving Documents to the end of the array (map.insert replaces existing values).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd put Home as the very last one, for the same reason.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(and this is also a reason not to use a QMap for "mapping" -> to be able to control order of iteration)</p></pre>
</div>
</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="https://git.reviewboard.kde.org/r/125158/diff/2/?file=403005#file403005line976" style="color: black; font-weight: bold; text-decoration: underline;">src/core/kfileitem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 KFileItem::iconName() const</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">976</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">isLocalUrl</span> <span class="o">&&</span> <span class="o">!</span><span class="n">delaySlowOperations</span> <span class="o">&&</span> <span class="n">isDir</span><span class="p">())</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Should be combined with the previous block</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">if (isLocalUrl && !delaySlowOperations && isDir()) {
// icons for standards paths
if (isDirectoryMounted(url)) {
// iconFromDirectoryFile
}
}
</pre></div>
</p></pre>
</div>
</div>
<br />
<p>- David Faure</p>
<br />
<p>On September 12th, 2015, 1:29 p.m. UTC, Harald Sitter 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.</div>
<div>By Harald Sitter.</div>
<p style="color: grey;"><i>Updated Sept. 12, 2015, 1:29 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=352498">352498</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;">this acts as an additional fallback to mimetype iconing and .directory
overrides. xdg user dirs are relocatable and can change depending on user
locale as well as user configuration and additionally are used in a range
of different desktop environments making this a very runtime sort of
decision.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BUG: 352498</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;">maked
autotested
installed
dolphin and file open dialogs now show icons</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/kfileitemtest.h <span style="color: grey">(615324f2b45fdc90a7841bdd0c8aa7f47cdf57a2)</span></li>
<li>autotests/kfileitemtest.cpp <span style="color: grey">(5f728a411401fe3009924b66970d9ae6f12c60f2)</span></li>
<li>src/core/kfileitem.cpp <span style="color: grey">(966d8626708a8f2672f1777c873f4e27e13023d6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125158/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>