-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68648
-----------------------------------------------------------
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 :-)
kioslave/trash/kcmtrash.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47855>
you're right, this didn't exist in Qt4. Please revert to Q_OS_MAC then :-)
kioslave/trash/trashimpl.h
<https://git.reviewboard.kde.org/r/120573/#comment47856>
the comment is wrong, this is called on-demand all over the place. Remove
the (called from...) part.
kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47852>
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..
kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47853>
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.
kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47851>
Eek, a C cast. My eyes bleed. Anyhow you can remove completely, with my
above suggestion.
kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47854>
Why not QFileInfo(trashDir).isDir(), to use Qt rather than unportable code?
- David Faure
On Oct. 16, 2014, 10:35 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. 16, 2014, 10:35 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
>
>
