<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/120460/">https://git.reviewboard.kde.org/r/120460/</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OK this wasn't too long, I reviewed it directly. It's even better, I can complain about all the non-satisfactory code, whether it was just ported or it was that way already :-)</p></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="https://git.reviewboard.kde.org/r/120460/diff/1/?file=316056#file316056line61" style="color: black; font-weight: bold; text-decoration: underline;">filenamesearch/kio_filenamesearch.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">61</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">item</span><span class="p">.</span><span class="n">name</span><span class="p">().</span><span class="n">contains</span><span class="p">(</span><span class="n">pattern</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;">item.text() would probably be better, for these special cases where text != name (name is internal, text is what the user sees in a directory listing)</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/120460/diff/1/?file=316056#file316056line66" style="color: black; font-weight: bold; text-decoration: underline;">filenamesearch/kio_filenamesearch.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">66</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QUrl</span> <span class="n">directory</span> <span class="o">=</span> <span class="n">QUrl</span><span class="o">::</span><span class="n">fromUserInput</span><span class="p">(</span><span class="n">url</span><span class="p">.</span><span class="n">queryItemValue</span><span class="p">(</span><span class="s">"url"</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;">Can this url parameter be a local path?
With its name I would expect a full URL - in which case QUrl(...) should be enough.</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/120460/diff/1/?file=316056#file316056line131" style="color: black; font-weight: bold; text-decoration: underline;">filenamesearch/kio_filenamesearch.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">131</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">path</span> <span class="o">=</span> <span class="n">fileName</span><span class="p">.</span><span class="n">path</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;">No, this should be toLocalFile()
(it makes a difference on Windows)</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/120460/diff/1/?file=316056#file316056line135" style="color: black; font-weight: bold; text-decoration: underline;">filenamesearch/kio_filenamesearch.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">135</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KIO</span><span class="o">::</span><span class="n">Job</span><span class="o">*</span> <span class="n">getJob</span> <span class="o">=</span> <span class="n">KIO</span><span class="o">::</span><span class="n">file_copy</span><span class="p">(</span><span class="n">fileName</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;">Wow, won't this be really slow? which "remote" protocols is it typically used with? Any? Well, if it's an explicit request from the user I guess that's OK...</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/120460/diff/1/?file=316056#file316056line143" style="color: black; font-weight: bold; text-decoration: underline;">filenamesearch/kio_filenamesearch.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">143</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">path</span> <span class="o">=</span> <span class="n">tempFile</span><span class="p">.</span><span class="n">fileName</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 code is keeping the filename of a file that is about to be deleted (by the QTemporaryFile destructor). So the reading will fail for sure. Sounds like this wasn't properly tested :-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For small files, KIO::get would even be better (contents in memory). But OK, if the files can be huge, I guess a tempfile is better.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One easy solution is to move the QTemporaryFile to the method scope, so it keeps existing until the end of the method. Given that the ctor doesn't do much (only assembling the tempfile template name) the performance penalty would probably be not too bad. Alternatively, move the QFile+QTextStream code to a helper function and call it in each of the two cases (local or remote) separately. Would be better for performance.</p></pre>
</div>
</div>
<br />
<p>- David Faure</p>
<br />
<p>On October 13th, 2014, 1:35 p.m. UTC, Emmanuel Pescosta 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-workspace and David Faure.</div>
<div>By Emmanuel Pescosta.</div>
<p style="color: grey;"><i>Updated Oct. 13, 2014, 1:35 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio-extras
</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;">The filenamesearch ioslave lists all files/folders which match with the given search query.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Made some small adjustments:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Ported the ioslave to Qt5/KF5
</em> Got rid of cleanup() and member variables
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Use lambda function validators instead of some weird if + assert things in searchDirectory/contentContainsPattern
</em> Immediately return when the search string is empty</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://projects.kde.org/projects/kde/applications/kde-baseapps/repository/revisions/frameworks/show/dolphin/src/search</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;">Compiles.</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>filenamesearch/kio_filenamesearch.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filenamesearch/kio_filenamesearch.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>CMakeLists.txt <span style="color: grey">(2036392)</span></li>
<li>filenamesearch/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>filenamesearch/filenamesearch.protocol <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120460/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>