----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 -----------------------------------------------------------
Nice! Just some comments. kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47678> 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"... kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47679> this comment doesn't match the code in the macro, which merely concatenates two strings, it doesn't check anything nor create anything. On that note, macros expanding to code are not great... functions are much better. Or in this case, it's a two-liner used twice, I would just inline ("duplicate") the code. kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47680> same as above, why do we want this fallback? kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47681> Minor: I'd put it.value() into a const QString trashDir local variable, to avoid repeating it 4 times, and to improve readability. kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47682> deleteEmptyTrashInfraStructure is implemented on all OSes, but only called on Mac, which seems a bit inconsistent. I looked at the trash spec again, and given the special permissions required on trash dirs in other partitions (DIR/.Trash or DIR/.Trash-$uid), I would feel safer if we didn't delete trash infrastructure. So I would make the entire method OSX only. BTW you should use Q_OS_OSX instead of Q_OS_MAC. iOS for sure doesn't work this way. kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47686> Damn, if only we had known, we could have made the XDG spec closer to the OSX implementation just by adding "es" :-) kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47683> Not much reason to have an ifdef mac around a kDebug. Remove it all, or enable the kDebug everywhere. kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47684> same kioslave/trash/trashimpl.cpp <https://git.reviewboard.kde.org/r/120573/#comment47685> such a debug statement is more useful if it prints out the input to the method, i.e. "topdir". - David Faure On Oct. 14, 2014, 11:59 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. 14, 2014, 11:59 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 > >
