<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 October 14th, 2014, 11:13 p.m. CEST, <b>David Faure</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/6/?file=318520#file318520line170" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/trash/trashimpl.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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::init()</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">170</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">error</span><span class="p">(</span> <span class="n">err</span><span class="p">,</span> <span class="n">trashDir</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;">Shouldn't this return false like the other blocks?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However I'm not sure I understand why this could happen though. Why wouldn't we be able to create "KDE.trash" but we would be able to create "info"? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with "info"...</p></pre>
</blockquote>
<p>On October 15th, 2014, 6:25 p.m. CEST, <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;">Modified as suggested. I agree that the error shouldn't occur. Normally it <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">cannot</em> occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and "by hand". However I'm not sure how KDE_mkdir handles a situation in which a (read-only) <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">file</em> of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either.</p></pre>
</blockquote>
<p>On October 15th, 2014, 11:09 p.m. CEST, <b>David Faure</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;">mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I just don't like that this code can use either trashPath or trashPath+"/KDE.trash", and has to handle both cases everywhere, including "hacks" like "if endsWith("/KDE.trash")".
We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"remove the subdirectory we couldn't create and use the standard KDE infrastructure" is weird if you have decided that the "KDE infrastructure" on OS X <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">is</em> trashdir+"/KDE.trash".</p></pre>
</blockquote>
<p>On October 16th, 2014, 6:59 p.m. CEST, <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;">Point taken.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X?</p></pre>
</blockquote>
<p>On October 16th, 2014, 8:35 p.m. CEST, <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;">http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On the $TRASH/KDE.trash issue and in general:
OS_X stores trashed files directly in ~/.Trash, I take?
In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What happens if you delete a file named "info" or "files" or "KDE.trash"?
In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there.
Yesno?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In case: how does the OS_X trash handle hidden (.*) files?
Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)?</p></pre>
</blockquote>
<p>On October 16th, 2014, 8:47 p.m. CEST, <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;">In fact, it works exactly the way the KDE trash works, except for the restore info which (I presume) is stored in the file metadata.
So:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">deleting a file that has the same name as a file already in the trash will 1) rename that file to something like "name 1" and 2) move that file into the trash. (I've been able to see that happen when the system is very busy). In short, it's only if someone already put a KDE.trash in the trash that we'd into trouble, unless it's a directory.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Why wouldn't it put hidden files there? Again, the Finder works like Dolphin or Nautilus: hidden files are nothing special, they're just not shown in the Finder (nor in file selection dialogs).</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not sure if the trash would show as non-empty if we store everything under ~/.Trash/.KDE.trash but if it does we have the problem that the user won't understand why when s/he opens the Trash in the Finder. And if it doesn't we're more or less back where we started.
I think it's important that KDE waste becomes visible in the OS X Trash, preferably in a subdirectory.</p></pre>
</blockquote>
<p>On October 16th, 2014, 10:53 p.m. CEST, <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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why wouldn't it put hidden files there?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Counterfeit - there's only limited reason to hide a file in the trash (the kioslave simply shows them), so I could have assumed it's renamed .foo -> _.foo or so.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does OS_X consider an empty directory in ~/.Trash a non-empty trash (notably if it has no meta/restorage data)?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The "ideal" solution to me seems to ensure there's always OUR (we can hardly re-use a directory the user deleted there via the OS_X tash) ~/.Trash/KDE.trash dir (so OS_X will avoid that name) and convince the OS_X trash to ignore it while empty.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we cannot make OS_X ignore it, such directory must be created avoiding any collision w/ relgularily deleted files/dirs (ie. if there's such file or dir, <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">we</em> must becom KDE.trash_1 or similar)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The tricky part will be to (persistently) export the legal KDE.trash* path to all clients (config key? but it must be updated by any running process whenever the path changes)</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;">The situation with hidden files is a weird one, in fact. I just created a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.file</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.dir</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.dir/file</code> in ~/.Trash . As I half-expected, the Finder kept considering its Trash to be empty (and so didn't allow it to be emptied either) because it's configured to ignore "hidden" files. So far so good (or not...). But as soon as there's recognised content and you empty the Trash, those hidden entries got deleted too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So no, hidden entries don't get special treatment in the Finder's Trash, but it's also much harder (probably impossible) to get them in there through the Finder, and there's no easy way to get them to be displayed either ("show hidden files").</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I wouldn't worry about these.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">we can hardly re-use a directory the user deleted there via the OS_X tash</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, yes and no. The previous one will be gone after emptying the Trash, contents included, but I've been taking the point of view that we can recreate it when needed. It's trash, there's no particular issue with wrapping it up nicely or not, my only concern is that KDE's trash can be emptied with the rest of the trash, and without having to fire up a KDE application to do that. (If all KDE apps that offer to move items to the wastebin also had a function to empty it I wouldn't have thought to start working on this!)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The ideal solution would in fact be to figure out how to store the .trashinfo data the native way, allowing the Finder to handle restores too, at least if that's possible. I'd propose to do that in a second step (it might well require making a trip to ObjC land); let's first see how far we can go with the simpler approach I'm taking here.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On October 15th, 2014, 6:26 p.m. CEST, 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 Runtime and David Faure.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Oct. 15, 2014, 6:26 p.m.</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::fileRemoved()</code> has been modified to call a new function, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteEmptyTrashInfraStructure</code> to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteEmptyTrashInfraStructure</code> this feature actually works, as expected as far as I can tell).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Remains to be done:
- 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/kcmtrash.cpp <span style="color: grey">(f4811fd)</span></li>
<li>kioslave/trash/trashimpl.h <span style="color: grey">(bc68723)</span></li>
<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>