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

[David Faure]

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



kioslave/trash/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120573/#comment47875>

    ok with the variable to avoid duplication, but now kio+solid are duplicated (and changed completely in KF5)
    
    This would be better:
    
        set (kio_trash_LIBS ...)
        if (APPLE)
            set(kio_trash_LIBS ${kio_trash_LIBS} "-framework DiskArbitration")
        endif()



kioslave/trash/kcmtrash.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47869>

    My point was that you forgot to change this one back :-)



kioslave/trash/tests/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120573/#comment47874>

    Don't duplicate the list of libs.
    
    Just add a 
    
    if (APPLE)
      target_link_libraries(testtrash "-framework DiskArbitration")
    endif()
    
    target_link_libraries is cumulative, it doesn't have to be called only once.



kioslave/trash/trashimpl.h
<https://git.reviewboard.kde.org/r/120573/#comment47870>

    spaces around '='.
    
    And no, don't worry about the "overhead". The QString() constructor does very little (it uses an internal null-string instance).
    
    No need for premature optimization at the expense of readability.



kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47871>

    !path.isEmpty() is more usual in Qt code than path.size().



kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47873>

    Should be const QString &mountPoint, you're not modifying it.
    
    Also, rename this method to idForMountPoint, it doesn't take a device string as input, but a mountpoint string. This confused me greatly when looking at the calling code :-)



kioslave/trash/trashimpl.cpp
<https://git.reviewboard.kde.org/r/120573/#comment47872>

    You can't do that. I'm surprised it doesn't crash for you, actually. encodeName returns a QByteArray. If you only store a char*, the bytearray goes out of scope and you're looking at deleted memory.
    
    Turn mp into a QByteArray, and remove the if(mp).


- David Faure


On Oct. 18, 2014, 6:08 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. 18, 2014, 6:08 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/CMakeLists.txt 3604089 
>   kioslave/trash/kcmtrash.cpp f4811fd 
>   kioslave/trash/tests/CMakeLists.txt 9161fdf 
>   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/20141019/e58bcc60/attachment.htm>