<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/126125/">https://git.reviewboard.kde.org/r/126125/</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/126125/diff/1/?file=417578#file417578line227" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/trash/kcmtrash.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; ">void TrashConfigModule::writeConfig()</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">227</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">layout</span><span class="o">-></span><span class="n">addWidget</span><span class="p">(</span> <span class="n">infoText</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;">(here and everywhere else) please remove spaces inside parenthesis. Compared to the kde4 codebase, this code was fully reformatted by astyle-kdelibs.</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/126125/diff/1/?file=417579#file417579line34" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/trash/tests/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">30</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">target_link_libraries</span><span class="p">(</span><span class="s">lockingtest</span> <span class="s">Qt5::Core</span> <span class="s">Qt5::DBus</span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">34</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">target_link_libraries</span><span class="p">(</span><span class="s">lockingtest</span> <span class="s">Qt5::Core</span> <span class="s">Qt5::DBus</span><span class="hl"> </span><span class="s"><span class="hl">Qt5::QspXDG</span></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;">you can't commit that</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/126125/diff/1/?file=417580#file417580line149" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/trash/tests/testtrash.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; ">void TestTrash::initTestCase()</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">149</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_trashDir</span> <span class="o">=</span> <span class="n">impl</span><span class="p">.</span><span class="n">trashDirectories</span><span class="p">()[</span><span class="mi">0</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;">Use .at(0) to avoid unnecessary detaching.</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/126125/diff/1/?file=417582#file417582line154" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/trash/trashimpl.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">154</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">trashDir</span> <span class="o">=</span> <span class="p">(</span><span class="n">path</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">())</span><span class="o">?</span> <span class="n">trashDirectoryPath</span><span class="p">(</span><span class="n">trashId</span><span class="p">)</span> <span class="o">:</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;">Add space before ?, and no need for () around path.isEmpty()</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/126125/diff/1/?file=417582#file417582line182" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/trash/trashimpl.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">182</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#ifndef Q_OS_MAC</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;">Any reason this uses Q_OS_MAC while the beginning of the patch uses Q_OS_OSX? That seems weird.</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/126125/diff/1/?file=417582#file417582line1167" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/trash/trashimpl.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; ">TrashImpl::TrashDirMap TrashImpl::topDirectories() 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">1161</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">trashDir</span> <span class="o">=</span> <span class="n">trashDir</span> <span class="o">+</span> <span class="n">QLatin1String</span><span class="p">(</span><span class="s">"/KDE.trash"</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;">could use += and QStringLiteral</p></pre>
</div>
</div>
<br />
<p>- David Faure</p>
<br />
<p>On November 20th, 2015, 9:09 p.m. UTC, René J.V. Bertin 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 Software on Mac OS X, KDE Frameworks and David Faure.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Nov. 20, 2015, 9:09 p.m.</i></p>
<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 is a "backport" of the <a href="https://git.reviewboard.kde.org/r/120573/" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kde-runtime modification reviewed here</a> that makes the trash ioslave use a subdirectory in the OS X trashcan.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">From the original review:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">KDE on OS X does not handle the desktop session (no "Plasma") nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up.</em></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in ~/.Trash. Files deleted from other volumes end up in /Volumes/volName/.Trashes/uid, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on .Trashes are the same as those expected by KDE.</em></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch from KDE4 was straightforward to apply as the trash implementation has only had what seem to be cosmetic changes and "Qt4 -> Qt5" adaptations.
Therefore, I have ported my code "as is", so as not to change anything that was discussed and negotiated during the rather lengthy previous review process. The only place where I've had to introduce a small additional change is in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trashForMountPoint</code>, where KF5 made <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trashDir</code> a const. Appending the KDE trash-subdir in the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trashDir</code> declaration might seem the appropriate approach, but since there is no guarantee at that point that the subdirectory actually (still) exists, the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QT_LSTAT</code> test could fail, and the the appending has to be done after the check on the actual host trash directory.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have also introduced a small change in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">testtrash.cpp</code> allowing it to run instead of failing because the 1st location returned <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trashDirectories()</code> doesn't match the one determined from QSP.</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;">OS X 10.9 with Qt 5.5.1 and KF5rameworks 5.16.0 . The code builds, and the unittests from testtrash succeed (with the Trash open in the Finder one sees the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDE.trash</code> directory show up while the test runs, and disappear afterwards).</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/ioslaves/trash/CMakeLists.txt <span style="color: grey">(05161cd)</span></li>
<li>src/ioslaves/trash/kcmtrash.cpp <span style="color: grey">(79c2ca7)</span></li>
<li>src/ioslaves/trash/tests/CMakeLists.txt <span style="color: grey">(3e138f7)</span></li>
<li>src/ioslaves/trash/tests/testtrash.cpp <span style="color: grey">(339aa19)</span></li>
<li>src/ioslaves/trash/trashimpl.h <span style="color: grey">(9886011)</span></li>
<li>src/ioslaves/trash/trashimpl.cpp <span style="color: grey">(26d9ea8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126125/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>