> On Oct. 14, 2014, 11:13 p.m., David Faure wrote: > > kioslave/trash/trashimpl.cpp, line 170 > > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170> > > > > 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"...
Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and "by hand". However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. > On Oct. 14, 2014, 11:13 p.m., David Faure wrote: > > kioslave/trash/trashimpl.cpp, line 351 > > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line351> > > > > 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. Oh yes, the macro checks something, and creates it if it doesn't exist - or should I say that the `testDir` function called by the macro does that? Understood that duplicating the code is acceptable here. > On Oct. 14, 2014, 11:13 p.m., David Faure wrote: > > kioslave/trash/trashimpl.cpp, line 382 > > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line382> > > > > same as above, why do we want this fallback? What else would we want to do? I haven't seen provisions to move only the file and not create the `.trashinfo` entry that contains the relevant meta-data. I'm inclined to try as hard/much as possible to achieve "normal" behaviour, and have to tell the user that we couldn't move an item to the wastebin. > On Oct. 14, 2014, 11:13 p.m., David Faure wrote: > > kioslave/trash/trashimpl.cpp, line 854 > > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line854> > > > > 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. > > René J.V. Bertin wrote: > KDE on iOS, seriously? Even if so, I'd hope Qt don't use Q_OS_MAC for > that, because > 1 MAC as in Macintosh refers to a line of desktop and laptop computers, > not the iDevices > 2 iOS is in fact an embedded form of OS X Re: iOS: here's how Qt5 defines the platform tokens: ```C++ #if defined(Q_OS_DARWIN) # define Q_OS_MAC # if defined(Q_OS_DARWIN64) # define Q_OS_MAC64 # elif defined(Q_OS_DARWIN32) # define Q_OS_MAC32 # endif # include <TargetConditionals.h> # if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE # define Q_OS_IOS # elif defined(TARGET_OS_MAC) && TARGET_OS_MAC # define Q_OS_OSX # define Q_OS_MACX // compatibility synonym # endif #endif ``` > On Oct. 14, 2014, 11:13 p.m., David Faure wrote: > > kioslave/trash/trashimpl.cpp, line 1043 > > <https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043> > > > > such a debug statement is more useful if it prints out the input to the > > method, i.e. "topdir". > > René J.V. Bertin wrote: > Point(s) taken. For now I still don't know in what circumstances > trashForMountPoint is called/used. Once that figured out the new debug > statements can go altogether ... I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? - René J.V. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 ----------------------------------------------------------- On Oct. 14, 2014, 1:59 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. 14, 2014, 1:59 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 > >
