----------------------------------------------------------- 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 > >