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

Sat Nov 21 10:10:39 UTC 2015

> On Nov. 21, 2015, 3:05 a.m., Aleix Pol Gonzalez wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 129
> > <https://git.reviewboard.kde.org/r/126125/diff/1/?file=417582#file417582line129>
> >
> >     Infrastructure is 1 word

I guess that depends on how far you want to decompose, but if you say so ... consider it done :)

On Nov. 21, 2015, 3:05 a.m., René J.V. Bertin wrote:
> > I'm wondering if ifdef'ing the shit out of the code is the best solution. Would it make sense to split it in a couple of files or get an `_apple.cpp` file? Or even get a completely different trash:/ kio implementation?

I didn't realise it was that bad? 

I think the reason why I never considered this (or that it didn't come up in the KDE4 RR) is that there is still much more common code than there is specific code. So splitting up is going to introduce much more code duplication than I think is wise to allow for. I know it's code that is unlikely to evolve a lot (at least I'd guess so), but duplication is about the opposite of the reasons to write shared libraries/frameworks in the first place, no?

Many of the ifdefs are only there to call a specific function. It would indeed be possible and trivial to move the specific functions to a separate file if there is a consensus that that would be beneficial. I could also provide shims for the 2 most frequently called added functions, basically moving most of the ifdefs into those 2 functions.

I suppose the MS Windows adaptations should be handled like that too, in that case :)

- René J.V.

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

On Nov. 20, 2015, 10:09 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126125/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2015, 10:09 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> This is a "backport" of the [kde-runtime modification reviewed here](https://git.reviewboard.kde.org/r/120573/) that makes the trash ioslave use a subdirectory in the OS X trashcan.
> 
> From the original review:
> *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 patch from KDE4 was straightforward to apply as the trash implementation has only had what seem to be cosmetic changes and "Qt4 -> Qt5" adaptations.
> Therefore, I have ported my code "as is", so as not to change anything that was discussed and negotiated during the rather lengthy previous review process. The only place where I've had to introduce a small additional change is in `trashForMountPoint`, where KF5 made `trashDir` a const. Appending the KDE trash-subdir in the `trashDir` declaration might seem the appropriate approach, but since there is no guarantee at that point that the subdirectory actually (still) exists, the `QT_LSTAT` test could fail, and the the appending has to be done after the check on the actual host trash directory.
> 
> I have also introduced a small change in `testtrash.cpp` allowing it to run instead of failing because the 1st location returned `trashDirectories()` doesn't match the one determined from QSP.
> 
> 
> Diffs
> -----
> 
>   src/ioslaves/trash/CMakeLists.txt 05161cd 
>   src/ioslaves/trash/kcmtrash.cpp 79c2ca7 
>   src/ioslaves/trash/tests/CMakeLists.txt 3e138f7 
>   src/ioslaves/trash/tests/testtrash.cpp 339aa19 
>   src/ioslaves/trash/trashimpl.h 9886011 
>   src/ioslaves/trash/trashimpl.cpp 26d9ea8 
> 
> Diff: https://git.reviewboard.kde.org/r/126125/diff/
> 
> 
> Testing
> -------
> 
> OS X 10.9 with Qt 5.5.1 and KF5rameworks 5.16.0 . The code builds, and the unittests from testtrash succeed (with the Trash open in the Finder one sees the `KDE.trash` directory show up while the test runs, and disappear afterwards).
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151121/f8722109/attachment.html>