Review Request 120573: [OS X] make KDE's trash use the OS X trash

Thu Oct 16 17:59:33 BST 2014

> On Oct. 14, 2014, 11:13 p.m., David Faure wrote:
> > kioslave/trash/trashimpl.cpp, line 170
> > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170>
> >
> >     Shouldn't this return false like the other blocks?
> >     
> >     And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern.
> >     
> >     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.
> >     
> >     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"...
> 
> René J.V. Bertin wrote:
>     Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* 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) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either.
> 
> David Faure wrote:
>     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).
>     
>     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.
>     
>     "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 *is* trashdir+"/KDE.trash".

Point taken.

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?

> On Oct. 14, 2014, 11:13 p.m., David Faure wrote:
> > kioslave/trash/trashimpl.cpp, line 1043
> > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043>
> >
> >     such a debug statement is more useful if it prints out the input to the method, i.e. "topdir".
> 
> René J.V. Bertin wrote:
>     Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ...
> 
> René J.V. Bertin wrote:
>     I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of.
>     
>     Unless it's a remnant from the past that's no longer being used?
> 
> David Faure wrote:
>     What would be a remnant from the past? trashForMountPoint?? It's called by TrashImpl::findTrashDirectory and by TrashImpl::scanTrashDirectories.
>     It's the way we find the trash dirs on other partitions. Surely that is used.
>     
>     You can e.g. try opening trash:/ in a kde file manager, it will scan for all trash dirs.

Well, I've done that; opening the trash in Dolphin is part of my regular testing routine for this. And NEVER have I yet seen debug output from trashForMountPoint...

- René J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68418
-----------------------------------------------------------

On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120573/
> -----------------------------------------------------------
> 
> (Updated Oct. 15, 2014, 6:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Runtime and David Faure.
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> -------
> 
> 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.
> 
> 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.
> 
> The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered.
> 
> On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` 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 `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell).
> 
> Remains to be done:
> - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash`
> 
> 
> Diffs
> -----
> 
>   kioslave/trash/kcmtrash.cpp f4811fd 
>   kioslave/trash/trashimpl.h bc68723 
>   kioslave/trash/trashimpl.cpp 30ee05b 
> 
> Diff: https://git.reviewboard.kde.org/r/120573/diff/
> 
> 
> Testing
> -------
> 
> 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
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20141016/41cac65e/attachment.htm>