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

Sat Oct 18 18:32:08 BST 2014

> On Oct. 18, 2014, 11:28 a.m., David Faure wrote:
> > Almost there :-)
> > 
> > Re your earlier comments:
> > - trashimpl.h is completely internal to the kioslave, adding methods there is no problem
> > - trashForMountPoint is called when trashing a file that is on a different partition than HOME.
> > I can see it being called when I do this, on Linux:
> > 
> >     cd /tmp
> >     touch foo
> >     export KDE_FORK_SLAVES=1
> >     kde-cp foo trash:/
> > 
> > The debug output from kio_trash (which appears in the same terminal due to the use of KDE_FORK_SLAVES=1) says
> >  TrashImpl::findTrashDirectory: mountPoint= "/"  trashDir= ""
> > which is one line below the call to trashForMountPoint, and prints out its result. So it's called for sure :-)
> 
> René J.V. Bertin wrote:
>     Indeed it's called, but then the/my code doesn't use whatever is returned, using ~/.Trash/KDE.trash instead. Hmmm.
>     Wonder why I never saw it called when using Dolphin, though.
> 
> David Faure wrote:
>     probably because the trash dir at the toplevel of the partition doesn't exist and can't be created by the user. See the security checks in the spec & code.
>     Try creating a trash dir in the mount point with the right permissions and ownership.

No, permissions are OK and the user trash directory can be created. The reason the result was skipped was in findTrashDirectory: I had overlooked that it returns 0 if the determined directory is empty (= doesn't have the infrastructure in place).

But then we block on the fact that apparently the StorageAccess Solid query hasn't been implemented on OS X.
I could work around that of course, either by reading the output of an OS X utility command, or simply by determining a unique number from the mountpoint name (a unique number being all that's apparently required).
I'm working on something that will introduce a dependency on the DiskArbitration framework as a first approach. Suggestions welcome - or guidance how to implement the missing functions in the Solid component (because there isn't even a stub skeleton from what I can see).

> On Oct. 18, 2014, 11:28 a.m., David Faure wrote:
> > kioslave/trash/kcmtrash.cpp, line 219
> > <https://git.reviewboard.kde.org/r/120573/diff/8/?file=320048#file320048line219>
> >
> >     you're right, this didn't exist in Qt4. Please revert to Q_OS_MAC then :-)

Do you really think I waited for this request? ;)

> On Oct. 18, 2014, 11:28 a.m., David Faure wrote:
> > kioslave/trash/trashimpl.cpp, line 149
> > <https://git.reviewboard.kde.org/r/120573/diff/8/?file=320050#file320050line149>
> >
> >     Is this check still needed? We know that every entry in m_trashDirectories ends with KDE.trash, now, right?
> >     
> >     Or is this "everything but the trash in the home dir"? then better test for that explicitely.
> >     
> >     endsWith sounds fragile to me, like the code doesn't really know what it should be doing, and could get fooled by unexpected naming..

No, it shouldn't be necessary anymore. I'd put a Q_ASSERT on the endsWith return value, but we both know how things rarely ever go wrong when assert statements are built in ;)
The thing is that it's NOT a good idea to delete one of the Finder's trash directories. You can recreate it, but that doesn't mean it'll be used once the Finder has noticed its absence.
(You may be familiar with the old battle cry, `FTFF` ...)

As to using endsWith ... is there something better, like `[NSString lastPathComponent:]` ?

> On Oct. 18, 2014, 11:28 a.m., David Faure wrote:
> > kioslave/trash/trashimpl.cpp, line 159
> > <https://git.reviewboard.kde.org/r/120573/diff/8/?file=320050#file320050line159>
> >
> >     Looks like path is an optional value, not a modifyable parameter... then don't use a pointer, use an empty string as the default value.
> >     
> >     [...], const QString &path = QString() );
> >     in the header.

Yes, I don't use a pointer to modify an optional argument (`&` could work just as well for that); I didn't want to create overhead by creating an empty QString instance each time the function is called and having to determine string length rather than just checking for a null pointer ...
Is reducing overhead less important than adhering to a style principle?

> On Oct. 18, 2014, 11:28 a.m., David Faure wrote:
> > kioslave/trash/trashimpl.cpp, line 194
> > <https://git.reviewboard.kde.org/r/120573/diff/8/?file=320050#file320050line194>
> >
> >     Eek, a C cast. My eyes bleed. Anyhow you can remove completely, with my above suggestion.

At least I don't have to think twice about what that kind of cast really does, nor what kind of weird function call I'm looking at :P

> On Oct. 18, 2014, 11:28 a.m., David Faure wrote:
> > kioslave/trash/trashimpl.cpp, line 207
> > <https://git.reviewboard.kde.org/r/120573/diff/8/?file=320050#file320050line207>
> >
> >     Why not QFileInfo(trashDir).isDir(), to use Qt rather than unportable code?

Why not, indeed, now that I know it exists :)

- René J.V.

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

On Oct. 17, 2014, 12:35 a.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. 17, 2014, 12:35 a.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/20141018/f92a8387/attachment.htm>