Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[René J . V . Bertin]

On Saturday December 12 2015 19:32:34 David Faure wrote:

Somehow I never got around to answering this. Also directing this back to the 
lists.


> I didn't mean switching at runtime. I mean using KF5 in Qt apps.
> I'm pretty sure we had this discussion already but here we go again:
> If a pure Qt app (say Scribus) has always saved her config to 
> ~/Library/Preferences (using QSP)
> and in version 1.42 starts using, say, KIO for network-aware file transfers, 
> then that should have
> no impact on the location of its config file.
> With a process-global mode for QSP, e.g. triggered by the "activator", then 
> suddenly in
> version 1.42 Scribus would get from QSP an XDG path (~/.config), and users 
> would lose
> their config. There's no reason that using one framework should change Qt's 
> behaviour globally.

I actually don't think I'd ever seen that argument before. It's true of course, 
but it takes us to a situation where we have 3 instead of 2 kinds of 
applications:
1) pure Qt applications
2) KF5 applications
3) Qt applications that use a KF5 framework or two.

Maybe I was wrong in ever thinking that there is such a thing as a KF5 
application, defined in terms of "uses a specific set of KF5 frameworks". 
This would all have been so much easier if a distinguishing class like 
KApplication had survived the 4->5 transition. After all, what's a less 
ambiguous way to define a KDE application than one that uses KApplication, 
KGuiApplication or KCoreApplication (even if those classes are nothing but an 
infinitely thin wrapper on most platforms) ...

What annoys me in this whole situation is that I'm getting zero feedback from 
MacPorts (users or developers), 2 or 3 exceptions notwithstanding. I'm pretty 
sure though that'd change if I were to introduce a QSP patch that creates a 
hardwired XDG-compliant mode.

> > > But I mean "application A can get QSP native while KF5 gets QSP in XDG 
> > > mode".
> > > So ok not per-call, but not process-global either. Something in between: 
> > > per module.

Do you remember what you meant with "module" here?

> That requires patching qstandardaths.h.

That's what I have to do anyway in order to introduce a public switch to toggle 
between native and XDG-compliant mode.

So we're getting a point were I have 2 alternatives to my current approach

1) introduce a header-based class (say KStandardPaths in kstandardpaths.h) 
which wraps all relevant QStandardPaths methods such that QSP is first toggled 
to XDG-compliant mode (or at least that the switch is called first). That class 
could also be defined by qstandardpaths.h itself (say as QXDGStandardPaths). 
That's something that might be acceptable to upstream. A local patch could then 
add a specific preprocessor token to make that headerfile also do a `#define 
QStandardPaths QXDGStandardPaths`. 
With that approach one only needs to add a -D compiler option to the software 
that wants XDG-compliant mode ... but that wouldn't affect any libraries 
(frameworks).

2) Introduce a KApplication framework with the specific mission statement of 
setting up things appropriately so that applications can be part of some sort 
of KDE universe - whatever that may mean on the different (non-Plasma) 
platforms.
There is 1 problem with this: all applications do not instantiate Q*Application 
as the 1st thing in their main() routine, so an implementation that toggles the 
QSP in the K*Application ctor wouldn't affect QSP calls made before that ctor 
is called.

Both alternatives thus have a problem in the extent of their QSP control. So 
each time I think of alternative solutions to my current implementation, I come 
back to the fact that I need to flip the toggle
a) at an appropriate time before main is called (or at least as the 1st thing 
in main)
b) OR I need to define a global bool variable that is then used in a wrapped 
QSP call or in an explicit call to the QSP mode toggle function, hopefully 
before any "unprotected" calls are made to a QSP location function.

In other words, we'd still be looking at an additional link module if I want to 
reduce the complexity (and maintenance burden) of distribution QSP/XDG patches, 
esp. if those patches are to target applications and no longer only the 
frameworks.
That makes me lean to alternative (2) and (a) above. A KApplication framework 
could contain both an initialisation routine or a private class that gets a 
global, static instance; either of those options provide an early way to call 
the QSP toggle switch with a value that can be defined when the *framework* is 
built. The K*Application ctors could then call that toggle again with a value 
that is defined when the *application* is built. Patching applications would be 
as "simple" as ensuring that the appropriate K*Application class is used 
instead of Q*Application: #include k*application.h after q*application.h, 
and link with the KApplication binary where needed.
This has the big advantage over my current approach that it

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[René J . V . Bertin]

Alex Merry wrote:

> I'm not sure what you mean by "incremental", though. All CMake variables
> overwrite their previous values when you set them, so you have to
> explicitly include the old value when you set them, and you can't really
> do this from the command line.

That's what I was getting at. If a feature, it's an annoying one that really 
doesn't play well with build systems where different components each may have 
something to add ...
Gotta live with it, I guess.

R.

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[Alex Merry]

On 2015-11-28 21:53, René J. V.  Bertin wrote:
BTW, not that it's already being used, but is 
CMAKE_EXE_LINKER_FLAGS_INIT
incremental or does one have to accumulate all elements first and use a 
single -

DCMAKE_EXE_LINKER_FLAGS_INIT=XX argument?


CMAKE_EXE_LINKER_FLAGS_INIT is a bit special. You have to specify it on 
the first CMake run you do for a project (ie: before the CMakeCache.txt 
file has been created), and whatever you put in it will be added to 
whatever default value for CMAKE_EXE_LINKER_FLAGS that CMake comes up 
with (which I believe is just the contents of the LDFLAGS env var when 
compiling with Clang or GCC). After that, it will be ignored, and you'll 
want to modify CMAKE_EXE_LINKER_FLAGS directly.


Note that what I said above only applies to setting this variable on the 
command line, not to setting it from within CMake code (when you'll just 
want to append to CMAKE_EXE_LINKER_FLAGS).


I'm not sure what you mean by "incremental", though. All CMake variables 
overwrite their previous values when you set them, so you have to 
explicitly include the old value when you set them, and you can't really 
do this from the command line.


Alex
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[René J . V . Bertin]

Alex Merry wrote:


> I'd be fine with adding something to KDECompilerSettings as long you had
> to explicitly ask for it to be enabled when running CMake. But if you're
> doing that, why not set CMAKE_EXE_LINKER_FLAGS_INIT directly on the
> CMake command line, or use the LDFLAGS env var? I assume this could all
> be done as part of the macports build process fairly easily, and that
> would seem to make most sense, given this is a macports-centric thing.

If I'm right it's not just going to be a MacPorts-centric thing, but something 
that all comparable projects (that aim to provide dependencies that are shared 
among dependents) are likely to adopt.

Indeed, MacPorts' build system is very flexible (Tcl scripts, basically); I'm 
more concerned about what currently goes on in CMake. Right now the activator 
is 
added in 2 distinct steps:
- tell CMake to find the corresponding Qt component, either in the composite 
call that finds multiple components, or using separate find expressions
- add the module as a link_target_library (PRIVATE Qt5::QspXDG).

That 2-stage nature probably makes it difficult to add the activator via the 
command line though I'm guessing that I might be able to get away with 
specifying "-framework QspXDG" on the commandline (and lose configure-time 
checking if the component exists).
BTW, not that it's already being used, but is CMAKE_EXE_LINKER_FLAGS_INIT 
incremental or does one have to accumulate all elements first and use a single -
DCMAKE_EXE_LINKER_FLAGS_INIT=XX argument?

R.

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[Alex Merry]

On 2015-11-28 19:25, René J.V. Bertin wrote:

On Saturday November 28 2015 15:29:00 David Faure wrote:
Otherwise, the next best idea is to get ECM to add your activator to 
all link lines
automatically, e.g. by adding it to CMAKE_EXE_LINKER_FLAGS or 
whatever.
This for sure beats editing every link line, or trying to guess which 
frameworks

are going to be "used by everyone".


I agree. Full heartedly even. But when I put out a query for
suggestion how to accomplish this on k-f-d the only feedback I got
could be summarised as "not interested in such a use case".
But how certain is it that all candidates that should use the
activator indeed use the ECM?


Well, it's about more than just using ECM. Which bit of ECM? This would 
probably come under something like KDECompilerSettings or some such.


When you asked about something in ECM previously, I was under the 
impression you were asking about something like the old kde4_add_app (or 
whatever it was called) macro, which ECM doesn't have an equivalent of 
(and I'm not keen on adding such a thing). I'll admit that using 
built-in CMake functionality like CMAKE_EXE_LINKER_FLAGS hadn't occurred 
to me as an alternative.


I'd be fine with adding something to KDECompilerSettings as long you had 
to explicitly ask for it to be enabled when running CMake. But if you're 
doing that, why not set CMAKE_EXE_LINKER_FLAGS_INIT directly on the 
CMake command line, or use the LDFLAGS env var? I assume this could all 
be done as part of the macports build process fairly easily, and that 
would seem to make most sense, given this is a macports-centric thing.


Alex
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[René J . V . Bertin]

On Saturday November 28 2015 15:29:00 David Faure wrote:

>I still have to see proof that a "pure Qt application" installed with MacPorts
>really cares about where QSP points to - apart from the obvious migration issue
>if this ever changes.

The question is not whether individual applications care. The principle is that 
applications installed through MacPorts (or Fink or whatever) should be able to 
behave the same way as versions of the same application installed any other 
way. Take Qt Creator. You can install it through MacPorts, but you can also 
installed it via Qt's own installers. If you've been using a version from Qt 
and migrate to the one from MacPorts (or Fink or whatever) or vice-versa, they 
should use the same configuration, "Application Support" and other directories. 
Users should be able to launch the one or the other, and find their history, 
sessions, whatever.

In other words, applications that are designed to function in a standalone 
fashion and don't need to interact with other, non-Qt5-based XDG-compliant 
applications should be able to use the native QSP locations (and in fact should 
do just that).

>Otherwise, the next best idea is to get ECM to add your activator to all link 
>lines
>automatically, e.g. by adding it to CMAKE_EXE_LINKER_FLAGS or whatever.
>This for sure beats editing every link line, or trying to guess which 
>frameworks
>are going to be "used by everyone".

I agree. Full heartedly even. But when I put out a query for suggestion how to 
accomplish this on k-f-d the only feedback I got could be summarised as "not 
interested in such a use case".
But how certain is it that all candidates that should use the activator indeed 
use the ECM?


R.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[David Faure]

On Saturday 21 November 2015 17:17:49 René J.V. Bertin wrote:
> That would mean having 2 different Qt5 installs in MacPorts; one for pure Qt 
> applications that are expected to behave "natively"

I still have to see proof that a "pure Qt application" installed with MacPorts
really cares about where QSP points to - apart from the obvious migration issue
if this ever changes.
I keep having the feeling that the simplest solution is to consider MacPorts
a different "platform" than OSX, in terms of paths. For sure the install path of
an app installed via MacPorts will be something like /opt/local while a real
OSX Qt app wouldn't go there. So can't it live with settings in ~/.config and
data files in ~/.local/share etc.?

Otherwise, the next best idea is to get ECM to add your activator to all link 
lines
automatically, e.g. by adding it to CMAKE_EXE_LINKER_FLAGS or whatever.
This for sure beats editing every link line, or trying to guess which frameworks
are going to be "used by everyone".

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

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

(Updated Nov. 21, 2015, 7:57 p.m.)


Status
--

This change has been marked as submitted.


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

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

(Updated Nov. 21, 2015, 7:23 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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Nov. 21, 2015, 7:04 p.m., David Faure wrote:
> > Search/replace typo? Looks good after fixing that.

Dang, yes.

Evidently the compiler didn't catch that :)


- René J.V.


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


On Nov. 21, 2015, 6:57 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. 21, 2015, 6:57 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126125: [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/126125/#review88679
---

Ship it!


Search/replace typo? Looks good after fixing that.


src/ioslaves/trash/trashimpl.cpp (line 121)


dDebug? did you mean qDebug?



src/ioslaves/trash/trashimpl.cpp (line 141)


seems to be contagious


- David Faure


On Nov. 21, 2015, 5:57 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. 21, 2015, 5:57 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

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

(Updated Nov. 21, 2015, 6:57 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and David Faure.


Changes
---

Resolves David's last open issue.

I looked at removing the ifdefs around `createInfrastructure()`, but that 
function cannot be a stub elsewhere but on OS X. So one would either have to 
add an argument with a platform-dependent default value, or else accept that 
the infrastructure is checked each time the function is called (even when it's 
unlikely to be discarded "behind our backs).

Since the topic came up, I've also taken the liberty to convert certain 
qDebug() calls to qWarning(), and comment most of the remaining ones. It's only 
on OS X that that output goes into a system log (I hope), and I think this 
workaround still leaves plenty to discuss on a future RR ;)


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Nov. 21, 2015, 12:35 p.m., David Faure wrote:
> > src/ioslaves/trash/tests/testtrash.cpp, line 149
> > 
> >
> > Use .at(0) to avoid unnecessary detaching.
> 
> René J.V. Bertin wrote:
> I tried that first, but that fails to compile:
> ```
> kf5-kio/work/kio-5.16.0/src/ioslaves/trash/tests/testtrash.cpp:149:42: 
> error: no member named 'at' in 'QMap'
> m_trashDir = impl.trashDirectories().at(0);
>  ~~~ ^
> 6 warnings and 1 error generated.
> ```
> 
> David Faure wrote:
> Ah, it's a map, not a list, right. Use .value(0) then, to avoid creating 
> an empty item in case 0 isn't in the map. You could do this first of course: 
> QVERIFY(trashDirs.contains(0));
> m_trashDir = trashDirs.value(0);
> 
> (note that initTestCase already has a trashDirs local var which contains 
> the result of impl.trashDirectories())

Yeah, I had noted that. I'm not sure why I didn't decide to move its 
declaration upwards ...


- René J.V.


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


On Nov. 21, 2015, 1:50 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. 21, 2015, 1:50 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Nov. 21, 2015, 2 p.m., David Faure wrote:
> > Right, I'm not sure that splitting code into a different file really works 
> > here. To avoid duplication, there would have to be a base class (interface) 
> > first, with only the platform-specific code in its two subclasses, all the 
> > common code staying where it is now. Not sure this is really applicable 
> > here though.

I was thinking that we could splice out really specific code like 
`idForMountPoint`, but I agree, the gain would really be minimal.


- René J.V.


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


On Nov. 21, 2015, 1:50 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. 21, 2015, 1:50 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[René J . V . Bertin]

On Saturday November 21 2015 12:59:40 David Faure wrote:

>I still don't see why you can't just patch Qt on macports (given that you're 
>already patching it anyway) rather than having to add something to each and 
>every link line in KF5,

That would mean having 2 different Qt5 installs in MacPorts; one for pure Qt 
applications that are expected to behave "natively", and one for KF5 apps and 
the occasional pure Qt5 app that should behave in compliance of XDG because 
it's supposed to interact with KDE4 and non-Qt applications from the 
Freedesktop universe.
That's a solution I never really considered, and which I think would never make 
it into MacPorts.

It's already bad enough in that area: after months of silence while I prepared 
the Qt4 and Qt5 ports to enable concurrent installation the official (but not 
exclusive) maintainers of those ports both woke up and decided to do things 
their way. And of course by then they felt it was too much work to go over each 
and every change I made, and apparently unthinkable even to simply test my 
implementation. So now there's the official "qt4-mac" port, I have my own with 
a different install layout (I haven't bothered submitting that one), there's 
the official "qt5" port and my submitted "qt5-kde" port which has the XDG patch 
and an install layout that like the one from the old exclusive ports and what's 
usual under Linux (i.e. stuff in ${prefix}/share, ${prefix}/include/qt5 etc. 
rather than the whole bunch hidden under ${prefix}/libexec/qt5).

I probably don't need to modify each and every link line in each and every KF5 
cmake file, despite the fact that there is no single "Tier 0" framework. 
Theoretically it ought to be enough to add the activator only to the Tier1 
frameworks, though that would probably increase the likelihood that QSP gets 
called before it's toggled. I've also noticed that CMake apparently pulls in 
certain link dependencies recursively; I've seen the activator dylib appear in 
the link list of objects where I left it out explicitly (plugins, for instance).

>but let's have that discussion separately on k-f-d.

You got it :)

R.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126125: [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/126125/#review88671
---


Right, I'm not sure that splitting code into a different file really works 
here. To avoid duplication, there would have to be a base class (interface) 
first, with only the platform-specific code in its two subclasses, all the 
common code staying where it is now. Not sure this is really applicable here 
though.

- David Faure


On Nov. 21, 2015, 12:50 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. 21, 2015, 12:50 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[David Faure]

> On Nov. 21, 2015, 11:35 a.m., David Faure wrote:
> > src/ioslaves/trash/tests/testtrash.cpp, line 149
> > 
> >
> > Use .at(0) to avoid unnecessary detaching.
> 
> René J.V. Bertin wrote:
> I tried that first, but that fails to compile:
> ```
> kf5-kio/work/kio-5.16.0/src/ioslaves/trash/tests/testtrash.cpp:149:42: 
> error: no member named 'at' in 'QMap'
> m_trashDir = impl.trashDirectories().at(0);
>  ~~~ ^
> 6 warnings and 1 error generated.
> ```

Ah, it's a map, not a list, right. Use .value(0) then, to avoid creating an 
empty item in case 0 isn't in the map. You could do this first of course: 
QVERIFY(trashDirs.contains(0));
m_trashDir = trashDirs.value(0);

(note that initTestCase already has a trashDirs local var which contains the 
result of impl.trashDirectories())


> On Nov. 21, 2015, 11:35 a.m., David Faure wrote:
> > src/ioslaves/trash/tests/CMakeLists.txt, line 34
> > 
> >
> > you can't commit that
> 
> René J.V. Bertin wrote:
> Oops, that one got through, sorry about that.
> 
> René J.V. Bertin wrote:
> BTW, you probably understood that is my (in)famous activator for the 
> ditto QSP patch that switches QSP to XDG-compliant mode.
> 
> I still have some, probably naïve, hope that someday something will be 
> integrated that will remove the need to maintain the build system patches and 
> check them at each KF5 FW version bump...

I still don't see why you can't just patch Qt on macports (given that you're 
already patching it anyway) rather than having to add something to each and 
every link line in KF5, but let's have that discussion separately on k-f-d.


- David


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


On Nov. 21, 2015, 12:50 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. 21, 2015, 12:50 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

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

[René J . V . Bertin]

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

(Updated Nov. 21, 2015, 1:50 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and David Faure.


Changes
---

Incorporated all feedback except the issues left open. I'll investigate 
splitting up and reducing the ifdeffing later.


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Nov. 21, 2015, 12:35 p.m., David Faure wrote:
> > src/ioslaves/trash/tests/CMakeLists.txt, line 34
> > 
> >
> > you can't commit that
> 
> René J.V. Bertin wrote:
> Oops, that one got through, sorry about that.

BTW, you probably understood that is my (in)famous activator for the ditto QSP 
patch that switches QSP to XDG-compliant mode.

I still have some, probably naïve, hope that someday something will be 
integrated that will remove the need to maintain the build system patches and 
check them at each KF5 FW version bump...


- René J.V.


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Nov. 21, 2015, 12:35 p.m., David Faure wrote:
> > src/ioslaves/trash/tests/CMakeLists.txt, line 34
> > 
> >
> > you can't commit that

Oops, that one got through, sorry about that.


> On Nov. 21, 2015, 12:35 p.m., David Faure wrote:
> > src/ioslaves/trash/tests/testtrash.cpp, line 149
> > 
> >
> > Use .at(0) to avoid unnecessary detaching.

I tried that first, but that fails to compile:
```
kf5-kio/work/kio-5.16.0/src/ioslaves/trash/tests/testtrash.cpp:149:42: error: 
no member named 'at' in 'QMap'
m_trashDir = impl.trashDirectories().at(0);
 ~~~ ^
6 warnings and 1 error generated.
```


> On Nov. 21, 2015, 12:35 p.m., David Faure wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 182
> > 
> >
> > Any reason this uses Q_OS_MAC while the beginning of the patch uses 
> > Q_OS_OSX? That seems weird.

Just one that got through :-/


- René J.V.


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126125: [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/126125/#review88667
---



src/ioslaves/trash/kcmtrash.cpp (line 227)


(here and everywhere else) please remove spaces inside parenthesis. 
Compared to the kde4 codebase, this code was fully reformatted by 
astyle-kdelibs.



src/ioslaves/trash/tests/CMakeLists.txt (line 34)


you can't commit that



src/ioslaves/trash/tests/testtrash.cpp (line 149)


Use .at(0) to avoid unnecessary detaching.



src/ioslaves/trash/trashimpl.cpp (line 154)


Add space before ?, and no need for () around path.isEmpty()



src/ioslaves/trash/trashimpl.cpp (line 182)


Any reason this uses Q_OS_MAC while the beginning of the patch uses 
Q_OS_OSX? That seems weird.



src/ioslaves/trash/trashimpl.cpp (line 1161)


could use += and QStringLiteral


- David Faure


On Nov. 20, 2015, 9: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, 9: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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Nov. 21, 2015, 3:05 a.m., Aleix Pol Gonzalez wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 129
> > 
> >
> > 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 
> af

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

[René J . V . Bertin]

> On Nov. 20, 2015, 10:06 p.m., René J.V. Bertin wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 947
> > 
> >
> > These were `kDebug()` statements; I guess they should become 
> > `qWarning()` now that `kDebug` is gone. I've left the qDebug() statements 
> > in because I noticed there are plenty of those in the code; shouldn't they 
> > be turned into `qCWarning()` or `qCDebug` to avoid polluting the terminal 
> > or (on OS X) the `system.log` ?
> 
> Aleix Pol Gonzalez wrote:
> Yes, they should, eventually.

Sooner rather than later as far as I'm concerned, at least those that are true 
debug statements and not warnings.

As I said, any output to a terminal that doesn't have one to be printed on will 
go to pollute the system log on OS X. For warnings that's - I actually prefer 
to have those output `unconditionally` rather than having to activate some Qt 
logging mechanism when something appears not to be running right to see if 
*maybe* there isn't a warning message that will show up then. But simple 
statements like "hey, we're going to move this file to the trash" don't belong 
in a system log.

Not in any kind of log unless the user asked for it, in fact ... privacy 
implications!


- René J.V.


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Aleix Pol Gonzalez]

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



src/ioslaves/trash/trashimpl.cpp (line 129)


Infrastructure is 1 word


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?

- Aleix Pol Gonzalez


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Aleix Pol Gonzalez]

> On Nov. 20, 2015, 10:06 p.m., René J.V. Bertin wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 947
> > 
> >
> > These were `kDebug()` statements; I guess they should become 
> > `qWarning()` now that `kDebug` is gone. I've left the qDebug() statements 
> > in because I noticed there are plenty of those in the code; shouldn't they 
> > be turned into `qCWarning()` or `qCDebug` to avoid polluting the terminal 
> > or (on OS X) the `system.log` ?

Yes, they should, eventually.


- Aleix


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

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



src/ioslaves/trash/trashimpl.cpp (line 941)


These were `kDebug()` statements; I guess they should become `qWarning()` 
now that `kDebug` is gone. I've left the qDebug() statements in because I 
noticed there are plenty of those in the code; shouldn't they be turned into 
`qCWarning()` or `qCDebug` to avoid polluting the terminal or (on OS X) the 
`system.log` ?


- René J.V. Bertin


On Nov. 20, 2015, 10:03 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:03 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

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

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel