<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/120573/">https://git.reviewboard.kde.org/r/120573/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Oktober 13th, 2014, 6:16 nachm. UTC, <b>Milian Wolff</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/120573/diff/2/?file=318230#file318230line812" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/trash/trashimpl.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; ">bool TrashImpl::isEmpty() 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">812</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QFile</span><span class="o">::</span><span class="n">remove</span><span class="p">(</span><span class="n">infoPath</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;">why do you try to <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">delete</em> something in the check that should figure out if something is empty? that sounds very wrong to me.</p></pre>
</blockquote>
<p>On Oktober 13th, 2014, 6:28 nachm. UTC, <b>René J.V. Bertin</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;">The goal is explained in the comment: delete an empty directory infrastructure that no longer has a need to exist because the wastebin is empty, in order to empty the hosting trashcan.
Yes, it gives a side-effect to the function, but one very much related to its original function - one could say that for isEmpty to return true, said infrastructure should have been deleted.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have no objection to adding an additional function that handles this task, but it would have to be called from several locations (after isEmpty returned true), and also loop over all known trashes. Adding a few lines to isEmpty adds less overhead, and with proper comment doesn't increase maintainance cost, IMHO.</p></pre>
</blockquote>
<p>On Oktober 13th, 2014, 7:35 nachm. UTC, <b>Thomas Lübking</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;">You want to add this to "::fileRemoved()" i'd say (cause this is when the state will change), eventually on ::init() - in case that's required to catch trash emptying via safari.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Random side effects on a state queries are really bad style, so please don't do such (unless required, ie. you're hacking into other libs ;-)</p></pre>
</blockquote>
<p>On Oktober 13th, 2014, 8:01 nachm. UTC, <b>René J.V. Bertin</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;">And there I was under the impression that I <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">am</em> hacking into (an)other('s) lib O:-)</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;">Nah... that's not <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">my</em> definition of hacking :-P</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You'd do so when eg. bringing a plugin and want to sneak your way into the process by abusing a function that will be called for sure to cause sth. entirely different (because there's no other way to impact the binary behavior)</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On Oktober 13th, 2014, 5:32 nachm. 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 and KDE Runtime.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Okt. 13, 2014, 5:32 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-runtime
</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;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">~/.Trash</code>. Files deleted from other volumes end up in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/Volumes/volName/.Trashes/uid</code>, 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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.Trashes</code> are the same as those expected by KDE.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TrashImpl::init()</code> creates a standard trash in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">~/.local/share/Trash</code> (at least under OS X) but also <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TrashImpl::trashForMountPoint()</code> that is used in cases I have not yet encountered.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On OS X, my modified <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TrashImpl::init()</code> sets the standard trash directory to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">~/.Trash/KDE.trash</code> and will create the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">files</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">info</code> subdirectories as required, because they will of course be deleted when the user empties the OS X trash. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TrashImpl::isEmpty()</code> has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Remains to be done:
- figure out why <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isEmpty()</code> doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE.
- determine in what cases <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trashForMountPoint()</code> is used, and finish the modifications for it to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/.Trashes/uid/KDE.trash</code></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;">On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan</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>kioslave/trash/trashimpl.cpp <span style="color: grey">(30ee05b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120573/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>