Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[Aleix Pol Gonzalez]

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



src/plasma/svg.cpp (line 581)


It could be useful to use qGuiApp here, so that software that doesn't use 
QApplication can use it as well.


- Aleix Pol Gonzalez


On Nov. 20, 2015, 2:27 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126113/
> ---
> 
> (Updated Nov. 20, 2015, 2:27 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: 351923
> http://bugs.kde.org/show_bug.cgi?id=351923
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> since seems there are so many event filters installed on the QApplication 
> installed that may give performance issues, try to use separate, singleton 
> watchers for them to decrease the amount of eventfilters called
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/units.h fa2256e 
>   src/declarativeimports/core/units.cpp 4e2adae 
>   src/plasma/private/svg_p.h 597465e 
>   src/plasma/private/theme_p.h 5b8f71c 
>   src/plasma/private/theme_p.cpp f346343 
>   src/plasma/svg.cpp bcceaf7 
> 
> Diff: https://git.reviewboard.kde.org/r/126113/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[Marco Martin]

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

(Updated Nov. 20, 2015, 1:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit a9d3d9a81ab282298088bf0753c1c77f0ec21afe by Marco Martin 
to branch master.


Bugs: 351923
http://bugs.kde.org/show_bug.cgi?id=351923


Repository: plasma-framework


Description
---

since seems there are so many event filters installed on the QApplication 
installed that may give performance issues, try to use separate, singleton 
watchers for them to decrease the amount of eventfilters called


Diffs
-

  src/declarativeimports/core/units.h fa2256e 
  src/declarativeimports/core/units.cpp 4e2adae 
  src/plasma/private/svg_p.h 597465e 
  src/plasma/private/theme_p.h 5b8f71c 
  src/plasma/private/theme_p.cpp f346343 
  src/plasma/svg.cpp bcceaf7 

Diff: https://git.reviewboard.kde.org/r/126113/diff/


Testing
---


Thanks,

Marco Martin

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

Re: Review Request 126087: Move the i18n context from KDeclarative

[Aleix Pol Gonzalez]

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

(Updated Nov. 20, 2015, 3:03 p.m.)


Review request for KDE Frameworks, Chusslove Illich and Marco Martin.


Changes
---

Change the API into QVariant so we can have more control on the types, instead 
of leaving up to QML to do the type conversion.

Maybe it would be better to use QJSValue here, so that we didn't need the 
QJSValue -> QVariant conversion. This would require having the QML dependency 
though (and a separate object as well, probably).


Repository: ki18n


Description
---

The only way to use `i18n*()` so far in QML is by depending on all 
KDeclarative. This patch moves the necessary bits there so it can be adopted by 
an application or framework just by offering the needed bits within KI18n.
This is done by offering an object with methods that map to the `i18n*()` C++ 
counter-parts.


Diffs (updated)
-

  autotests/CMakeLists.txt 1cf0f7a 
  autotests/ki18ndeclarativetest.cpp PRE-CREATION 
  autotests/test.qml PRE-CREATION 
  docs/programmers-guide.md 13a5f9d 
  src/CMakeLists.txt 818595e 
  src/klocalizedcontext.h PRE-CREATION 
  src/klocalizedcontext.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/126087/diff/


Testing
---

Ported KDeclarative, everything still seems to work.


Thanks,

Aleix Pol Gonzalez

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

Re: Review Request 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[Aleix Pol Gonzalez]

> On Nov. 20, 2015, 1:44 p.m., Martin Gräßlin wrote:
> > The need for the change surprises me. I have been using a /opt setup for 
> > years and udev was always found. Can you show the error you hit?
> 
> René J.V. Bertin wrote:
> Have you been using a Linux distro like Ubuntu that has multi-arch, with 
> the actual libraries installed in `/[usr/]lib/x86_64-linux-gnu`? I guess it's 
> also not for nothing that most FindFoo scripts appear to use the 2-stage 
> approach where pkgconfig is queried first for hints about where to look in 
> addition to the default locations. BTW, I'm using cmake 3.3.1 . That's not 
> the host's default cmake which *may* have been patched to look in the 
> multi-arch locations, but in my experience it works better to use a cmake 
> from the target prefix (Ubuntu's Project Neon5 did the same, IIRC).
> 
> I don't have access to the Linux system right now, but the error was that 
> the library wasn't being found, IIRC it did find the headerfile. The error 
> wasn't more explicit than that, my initial reaction was to check if I had the 
> libudev-dev package installed.
> 
> René J.V. Bertin wrote:
> Actually, my memory was wrong, the error is more perverse. Or rather, 
> was, because I cannot seem to reproduce it now :-/
> 
> Somehow, the configure step completed without error, set `UDEV_LIBS` (to 
> `udev`, I think), but for whatever reason the library wasn't added to the 
> link command where it should be added. So I got a link error.
> 
> Of course I tried to rule out operator error before starting to hack the 
> FindUDev.cmake script, but I must have missed something, or something else 
> changed on my system since those few days ago. I seem to recall that I found 
> a libudev.so symlink in a generic (as opposed to a multi-arch) location, but 
> that one I can no longer find either.
> 
> So I can either discard this RR, or else it could serve as a reason 
> investigate whether some FindFoo scripts need `pkg_check_modules` to obtain 
> hints for `find_path` and `find_library`.

I'd suggest discarding it until we can properly reproduce.

Removing the build directory helps to reproduce this kind of issues, by the way.


- Aleix


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


On Nov. 20, 2015, 1:41 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126123/
> ---
> 
> (Updated Nov. 20, 2015, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks and Kubuntu.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel 
> prefix (/opt/local) it failed to find libudev which is installed "normally".
> 
> Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
> files (use pkgconfig first, and the results as hints for `find_path` and 
> `find_library`) corrected that failure.
> 
> 
> Diffs
> -
> 
>   cmake/FindUDev.cmake 9d0f21d 
> 
> Diff: https://git.reviewboard.kde.org/r/126123/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
> installed in /opt/local
> 
> 
> 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 126087: Move the i18n context from KDeclarative

[Aleix Pol Gonzalez]

> On Nov. 20, 2015, 2:33 p.m., Chusslove Illich wrote:
> > src/klocalizedcontext.cpp, line 87
> > 
> >
> > How are numbers converted to strings, before being passed here? Because 
> > when it receives actual numeric types, KI18n will take care to represent 
> > them in locale-specific way. Also it will let translators (through 
> > "translation scripting" feature) operate on numbers, e.g. to handle 
> > multi-plural cases.
> > 
> > Is it possible to get back to actual numbers, or the originating data 
> > type is irrevocably lost? If not, at least we should document this mungling 
> > somewhere.

At the moment it's QML itself doing the conversion and now that you point it 
out I see how it could be a problem... I'll try if it's possible to use 
QVariant there instead of QString directly.


- Aleix


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


On Nov. 19, 2015, 5 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Nov. 19, 2015, 5 p.m.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 1cf0f7a 
>   autotests/ki18ndeclarativetest.cpp PRE-CREATION 
>   autotests/test.qml PRE-CREATION 
>   docs/programmers-guide.md 13a5f9d 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
>   src/klocalizedcontext.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 126087: Move the i18n context from KDeclarative

[Chusslove Illich]

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



src/klocalizedcontext.h (line 28)


...of KI18n framework in QML?



src/klocalizedcontext.cpp (line 87)


How are numbers converted to strings, before being passed here? Because 
when it receives actual numeric types, KI18n will take care to represent them 
in locale-specific way. Also it will let translators (through "translation 
scripting" feature) operate on numbers, e.g. to handle multi-plural cases.

Is it possible to get back to actual numbers, or the originating data type 
is irrevocably lost? If not, at least we should document this mungling 
somewhere.



src/klocalizedcontext.cpp (line 145)


KI18n plural semantics is to take as plural deciding the first 
integer-valued argument, which needs not be the first argument. So this should 
be a loop over all arguments, until first successful conversion. (Of course, 
better if the originating numeric types can be recovered.)


- Chusslove Illich


On Нов. 19, 2015, 5 по п., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Нов. 19, 2015, 5 по п.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 1cf0f7a 
>   autotests/ki18ndeclarativetest.cpp PRE-CREATION 
>   autotests/test.qml PRE-CREATION 
>   docs/programmers-guide.md 13a5f9d 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
>   src/klocalizedcontext.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[René J . V . Bertin]

> On Nov. 20, 2015, 1:44 p.m., Martin Gräßlin wrote:
> > The need for the change surprises me. I have been using a /opt setup for 
> > years and udev was always found. Can you show the error you hit?
> 
> René J.V. Bertin wrote:
> Have you been using a Linux distro like Ubuntu that has multi-arch, with 
> the actual libraries installed in `/[usr/]lib/x86_64-linux-gnu`? I guess it's 
> also not for nothing that most FindFoo scripts appear to use the 2-stage 
> approach where pkgconfig is queried first for hints about where to look in 
> addition to the default locations. BTW, I'm using cmake 3.3.1 . That's not 
> the host's default cmake which *may* have been patched to look in the 
> multi-arch locations, but in my experience it works better to use a cmake 
> from the target prefix (Ubuntu's Project Neon5 did the same, IIRC).
> 
> I don't have access to the Linux system right now, but the error was that 
> the library wasn't being found, IIRC it did find the headerfile. The error 
> wasn't more explicit than that, my initial reaction was to check if I had the 
> libudev-dev package installed.

Actually, my memory was wrong, the error is more perverse. Or rather, was, 
because I cannot seem to reproduce it now :-/

Somehow, the configure step completed without error, set `UDEV_LIBS` (to 
`udev`, I think), but for whatever reason the library wasn't added to the link 
command where it should be added. So I got a link error.

Of course I tried to rule out operator error before starting to hack the 
FindUDev.cmake script, but I must have missed something, or something else 
changed on my system since those few days ago. I seem to recall that I found a 
libudev.so symlink in a generic (as opposed to a multi-arch) location, but that 
one I can no longer find either.

So I can either discard this RR, or else it could serve as a reason investigate 
whether some FindFoo scripts need `pkg_check_modules` to obtain hints for 
`find_path` and `find_library`.


- René J.V.


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


On Nov. 20, 2015, 1:41 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126123/
> ---
> 
> (Updated Nov. 20, 2015, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks and Kubuntu.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel 
> prefix (/opt/local) it failed to find libudev which is installed "normally".
> 
> Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
> files (use pkgconfig first, and the results as hints for `find_path` and 
> `find_library`) corrected that failure.
> 
> 
> Diffs
> -
> 
>   cmake/FindUDev.cmake 9d0f21d 
> 
> Diff: https://git.reviewboard.kde.org/r/126123/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
> installed in /opt/local
> 
> 
> 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 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[Martin Gräßlin]

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


The need for the change surprises me. I have been using a /opt setup for years 
and udev was always found. Can you show the error you hit?

- Martin Gräßlin


On Nov. 20, 2015, 1:41 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126123/
> ---
> 
> (Updated Nov. 20, 2015, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks and Kubuntu.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel 
> prefix (/opt/local) it failed to find libudev which is installed "normally".
> 
> Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
> files (use pkgconfig first, and the results as hints for `find_path` and 
> `find_library`) corrected that failure.
> 
> 
> Diffs
> -
> 
>   cmake/FindUDev.cmake 9d0f21d 
> 
> Diff: https://git.reviewboard.kde.org/r/126123/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
> installed in /opt/local
> 
> 
> 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 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[René J . V . Bertin]

> On Nov. 20, 2015, 1:44 p.m., Martin Gräßlin wrote:
> > The need for the change surprises me. I have been using a /opt setup for 
> > years and udev was always found. Can you show the error you hit?

Have you been using a Linux distro like Ubuntu that has multi-arch, with the 
actual libraries installed in `/[usr/]lib/x86_64-linux-gnu`? I guess it's also 
not for nothing that most FindFoo scripts appear to use the 2-stage approach 
where pkgconfig is queried first for hints about where to look in addition to 
the default locations. BTW, I'm using cmake 3.3.1 . That's not the host's 
default cmake which *may* have been patched to look in the multi-arch 
locations, but in my experience it works better to use a cmake from the target 
prefix (Ubuntu's Project Neon5 did the same, IIRC).

I don't have access to the Linux system right now, but the error was that the 
library wasn't being found, IIRC it did find the headerfile. The error wasn't 
more explicit than that, my initial reaction was to check if I had the 
libudev-dev package installed.


- René J.V.


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


On Nov. 20, 2015, 1:41 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126123/
> ---
> 
> (Updated Nov. 20, 2015, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks and Kubuntu.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel 
> prefix (/opt/local) it failed to find libudev which is installed "normally".
> 
> Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
> files (use pkgconfig first, and the results as hints for `find_path` and 
> `find_library`) corrected that failure.
> 
> 
> Diffs
> -
> 
>   cmake/FindUDev.cmake 9d0f21d 
> 
> Diff: https://git.reviewboard.kde.org/r/126123/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
> installed in /opt/local
> 
> 
> 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: KGlobalAccel on ~Linux?

[René J . V . Bertin]

David Faure wrote:

> On Friday 13 November 2015 22:04:35 René J. V. Bertin wrote:
>> I can imagine that the session dbus doesn't look in /opt/local by default on
>> Linux (guess I'll have to look into that)
> 
> Right. Add /opt/local/share to XDG_DATA_DIRS before starting the DBus daemon,
> otherwise the dbus service cannot be autostarted.
> 


Well, that didn't help. I waited to react until I had upgraded to Qt 5.5.1 and 
all 5.16.0 because I saw some mention of better support for Qt 5.5 .

Fact is, even with /opt/local/share appended to XDG_DATA_DIRS (now 
/usr/share:/usr/share/kde-plasma:/usr/local/share/:/usr/share/:/opt/local/share)
 
kglobalacces5 crashes calling `KDBusService service(KDBusService::Unique);` 
from 
main(). The actual crash is in QInternal::activateCallbacks(), so I'm not sure 
what to report and where ...

FWIW, with this setting for XDG_DATA_DIRS I now get what I think is appropriate 
output from kbuildsycoca5, including a message "Menu "applications-
kmenuedit.menu" not found".

On OS X where the dbus daemon is configured to look in /opt/local/share and 
where 
QSP adds that to the relevant standard locations the crash doesn't happen but I 
keep getting same error message 

"No such object path '/org/kde/kglobalaccel'"

R.

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

Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[Marco Martin]

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

(Updated Nov. 20, 2015, 11:18 a.m.)


Review request for KDE Frameworks and Plasma.


Bugs: 351923
http://bugs.kde.org/show_bug.cgi?id=351923


Repository: plasma-framework


Description
---

since seems there are so many event filters installed on the QApplication 
installed that may give performance issues, try to use separate, singleton 
watchers for them to decrease the amount of eventfilters called


Diffs (updated)
-

  src/plasma/svg.cpp bcceaf7 
  src/plasma/private/theme_p.h 5b8f71c 
  src/plasma/private/theme_p.cpp f346343 
  src/declarativeimports/core/units.h fa2256e 
  src/declarativeimports/core/units.cpp 4e2adae 
  src/plasma/private/svg_p.h 597465e 

Diff: https://git.reviewboard.kde.org/r/126113/diff/


Testing
---


Thanks,

Marco Martin

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

Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[Marco Martin]

> On Nov. 19, 2015, 5:38 p.m., David Edmundson wrote:
> > src/plasma/svg.cpp, line 47
> > 
> >
> > An SVG with colours will have a Theme object (via 
> > SvgPrivate::cacheAndColorsTheme())
> > 
> > ThemePrivate already has an (effecively shared) event filter on colours 
> > changing, can we use directly?

good idea


- Marco


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


On Nov. 20, 2015, 11:18 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126113/
> ---
> 
> (Updated Nov. 20, 2015, 11:18 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: 351923
> http://bugs.kde.org/show_bug.cgi?id=351923
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> since seems there are so many event filters installed on the QApplication 
> installed that may give performance issues, try to use separate, singleton 
> watchers for them to decrease the amount of eventfilters called
> 
> 
> Diffs
> -
> 
>   src/plasma/svg.cpp bcceaf7 
>   src/plasma/private/theme_p.h 5b8f71c 
>   src/plasma/private/theme_p.cpp f346343 
>   src/declarativeimports/core/units.h fa2256e 
>   src/declarativeimports/core/units.cpp 4e2adae 
>   src/plasma/private/svg_p.h 597465e 
> 
> Diff: https://git.reviewboard.kde.org/r/126113/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[Marco Martin]

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

(Updated Nov. 20, 2015, 12:19 p.m.)


Review request for KDE Frameworks and Plasma.


Bugs: 351923
http://bugs.kde.org/show_bug.cgi?id=351923


Repository: plasma-framework


Description
---

since seems there are so many event filters installed on the QApplication 
installed that may give performance issues, try to use separate, singleton 
watchers for them to decrease the amount of eventfilters called


Diffs (updated)
-

  src/declarativeimports/core/units.h fa2256e 
  src/declarativeimports/core/units.cpp 4e2adae 
  src/plasma/private/svg_p.h 597465e 
  src/plasma/private/theme_p.h 5b8f71c 
  src/plasma/private/theme_p.cpp f346343 
  src/plasma/svg.cpp bcceaf7 

Diff: https://git.reviewboard.kde.org/r/126113/diff/


Testing
---


Thanks,

Marco Martin

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

Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[David Edmundson]

> On Nov. 20, 2015, 12:35 p.m., David Edmundson wrote:
> > src/plasma/svg.cpp, line 590
> > 
> >
> > we may as well remove this method then?

Edit. obviously we can't. Ignore me.


- David


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


On Nov. 20, 2015, 12:19 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126113/
> ---
> 
> (Updated Nov. 20, 2015, 12:19 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: 351923
> http://bugs.kde.org/show_bug.cgi?id=351923
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> since seems there are so many event filters installed on the QApplication 
> installed that may give performance issues, try to use separate, singleton 
> watchers for them to decrease the amount of eventfilters called
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/units.h fa2256e 
>   src/declarativeimports/core/units.cpp 4e2adae 
>   src/plasma/private/svg_p.h 597465e 
>   src/plasma/private/theme_p.h 5b8f71c 
>   src/plasma/private/theme_p.cpp f346343 
>   src/plasma/svg.cpp bcceaf7 
> 
> Diff: https://git.reviewboard.kde.org/r/126113/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Review Request 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[René J . V . Bertin]

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

Review request for KDE Frameworks.


Repository: solid


Description
---

Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel prefix 
(/opt/local) it failed to find libudev which is installed "normally".

Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
files (use pkgconfig first, and the results as hints for `find_path` and 
`find_library`) corrected that failure.


Diffs
-

  cmake/FindUDev.cmake 9d0f21d 

Diff: https://git.reviewboard.kde.org/r/126123/diff/


Testing
---

On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
installed in /opt/local


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 126119: remove unused platformstatus kded

[Marco Martin]

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

(Updated Nov. 20, 2015, 10:19 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Bugs: 348840
http://bugs.kde.org/show_bug.cgi?id=348840


Repository: plasma-framework


Description
---

This kded was written at the beginning of the kf5 port cycle, it was supposed 
to do the runtime shell switching for transformable devices, but then it ended 
up completely implemented in plasmashell and not much tested anyways due to the 
lack of drivers.
this got completely unused, remove it (and eventually add it again in future if 
when there will be hardware support will be considered a viable approach, but 
probably not)


Diffs
-

  src/CMakeLists.txt 0ecb348 
  src/platformstatus/CMakeLists.txt 114d9eb 
  src/platformstatus/org.kde.platformstatus.xml d5c81bc 
  src/platformstatus/platformstatus.cpp d38635a 
  src/platformstatus/platformstatus.desktop 50e25a4 
  src/platformstatus/platformstatus.h 6c113ab 

Diff: https://git.reviewboard.kde.org/r/126119/diff/


Testing
---


Thanks,

Marco Martin

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

Re: Review Request 126113: make event filters static to decrease installed filters on qApp

[David Edmundson]

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

Ship it!



src/plasma/svg.cpp (line 590)


we may as well remove this method then?


- David Edmundson


On Nov. 20, 2015, 12:19 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126113/
> ---
> 
> (Updated Nov. 20, 2015, 12:19 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: 351923
> http://bugs.kde.org/show_bug.cgi?id=351923
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> since seems there are so many event filters installed on the QApplication 
> installed that may give performance issues, try to use separate, singleton 
> watchers for them to decrease the amount of eventfilters called
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/units.h fa2256e 
>   src/declarativeimports/core/units.cpp 4e2adae 
>   src/plasma/private/svg_p.h 597465e 
>   src/plasma/private/theme_p.h 5b8f71c 
>   src/plasma/private/theme_p.cpp f346343 
>   src/plasma/svg.cpp bcceaf7 
> 
> Diff: https://git.reviewboard.kde.org/r/126113/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 126087: Move the i18n context from KDeclarative

[Aleix Pol Gonzalez]

> On Nov. 19, 2015, 5:21 p.m., Marco Martin wrote:
> > so let's go for that and then add the import with the singleton as well, if 
> > is okay for framework maintainer are fine with the qtqml dependency.. but 
> > they should be as the "everything in kdeclarative" situation has to be 
> > solved (maybe optional dep at build time?)
> 
> Aleix Pol Gonzalez wrote:
> Are you sure you want to consider Qt5::Qml as an optional dependency? I 
> don't have a problem with it, but it seems odd as a first step.
> 
> Personally I'd like to propose it as is and if there's a reason not to 
> have it, then consider it.

For completion, at the moment the Qml dependency is only on the test. The full 
Qml dependency we'd get it if/when we do the qml runtime plugin, which I'd 
introduce in a later patch.


- Aleix


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


On Nov. 19, 2015, 5 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Nov. 19, 2015, 5 p.m.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 1cf0f7a 
>   autotests/ki18ndeclarativetest.cpp PRE-CREATION 
>   autotests/test.qml PRE-CREATION 
>   docs/programmers-guide.md 13a5f9d 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
>   src/klocalizedcontext.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

On Thursday November 19 2015 19:15:39 René J.V. Bertin wrote:

> ... Grand Central Dispatch ...

https://github.com/RJVB/KIdleTiming/commit/9514bf13b1d921e481fa6074d08fef58139dd986

Deliciously imprecise for the amateurs: almost 10% late almost systematically 
for the first 5000ms timeout of the example (against on the order of 1% for the 
alternative using a ballistic timer).

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

Re: Review Request 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[René J . V . Bertin]

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

(Updated Nov. 20, 2015, 5:13 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and Kubuntu.


Repository: solid


Description
---

Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel prefix 
(/opt/local) it failed to find libudev which is installed "normally".

Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
files (use pkgconfig first, and the results as hints for `find_path` and 
`find_library`) corrected that failure.


Diffs
-

  cmake/FindUDev.cmake 9d0f21d 

Diff: https://git.reviewboard.kde.org/r/126123/diff/


Testing
---

On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
installed in /opt/local


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 126123: [Linux/Kubuntu] find udev when installing to a parallel prefix

[René J . V . Bertin]

> On Nov. 20, 2015, 1:44 p.m., Martin Gräßlin wrote:
> > The need for the change surprises me. I have been using a /opt setup for 
> > years and udev was always found. Can you show the error you hit?
> 
> René J.V. Bertin wrote:
> Have you been using a Linux distro like Ubuntu that has multi-arch, with 
> the actual libraries installed in `/[usr/]lib/x86_64-linux-gnu`? I guess it's 
> also not for nothing that most FindFoo scripts appear to use the 2-stage 
> approach where pkgconfig is queried first for hints about where to look in 
> addition to the default locations. BTW, I'm using cmake 3.3.1 . That's not 
> the host's default cmake which *may* have been patched to look in the 
> multi-arch locations, but in my experience it works better to use a cmake 
> from the target prefix (Ubuntu's Project Neon5 did the same, IIRC).
> 
> I don't have access to the Linux system right now, but the error was that 
> the library wasn't being found, IIRC it did find the headerfile. The error 
> wasn't more explicit than that, my initial reaction was to check if I had the 
> libudev-dev package installed.
> 
> René J.V. Bertin wrote:
> Actually, my memory was wrong, the error is more perverse. Or rather, 
> was, because I cannot seem to reproduce it now :-/
> 
> Somehow, the configure step completed without error, set `UDEV_LIBS` (to 
> `udev`, I think), but for whatever reason the library wasn't added to the 
> link command where it should be added. So I got a link error.
> 
> Of course I tried to rule out operator error before starting to hack the 
> FindUDev.cmake script, but I must have missed something, or something else 
> changed on my system since those few days ago. I seem to recall that I found 
> a libudev.so symlink in a generic (as opposed to a multi-arch) location, but 
> that one I can no longer find either.
> 
> So I can either discard this RR, or else it could serve as a reason 
> investigate whether some FindFoo scripts need `pkg_check_modules` to obtain 
> hints for `find_path` and `find_library`.
> 
> Aleix Pol Gonzalez wrote:
> I'd suggest discarding it until we can properly reproduce.
> 
> Removing the build directory helps to reproduce this kind of issues, by 
> the way.

Or *not* reproduce them :)

But IIRC I did that, though usually the 1st things I try is to remove 
build/CMakeCache.txt and then build/CMake`*.


- René J.V.


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


On Nov. 20, 2015, 1:41 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126123/
> ---
> 
> (Updated Nov. 20, 2015, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks and Kubuntu.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Configuring KF5 Solid under KUbuntu 14.04 for installation in a parallel 
> prefix (/opt/local) it failed to find libudev which is installed "normally".
> 
> Modifying FindUDev.cmake to follow the same approach as other Find`*.cmake 
> files (use pkgconfig first, and the results as hints for `find_path` and 
> `find_library`) corrected that failure.
> 
> 
> Diffs
> -
> 
>   cmake/FindUDev.cmake 9d0f21d 
> 
> Diff: https://git.reviewboard.kde.org/r/126123/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04.3 with Qt 5.5.1 and all of Solid 5.16.0's KF5 dependencies 
> installed in /opt/local
> 
> 
> 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

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

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

[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

[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