D14302: Don't block forever in ensureKdeinitRunning

[Lamarque Souza]

lvsouza added a comment.


  I think I know what is happening. This line
  
  QDeadlineTimer timer(qMax(timeout, -1));// QDT only takes -1 as "forever"
  
  passes the result of qMax() to QDeadlineTimer's constructor. That constructor 
receives a quint64. Since qMax() is a template:
  
  inline const T (const T , const T ) { return (a < b) ? b : a; }
  
  it will use the type of the assigned variable (quint64 in this case) as T and 
casting -1 to INT64_MAX. Changing the line to:
  
  QDeadlineTimer timer(qMax(timeout, qint64(-1)));
  
  should solve the problem. If it does not then this should work:
  
  qint64 maxTimeout = qMax(timeout, -1);
  QDeadlineTimer timer(maxTimeout);

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: lvsouza, kde-frameworks-devel, michaelh, ngraham, bruns

D6687: Adding support to ipv*.route-metric

[Lamarque Souza]

lvsouza accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D6687

To: pvillani, jgrulich, lvsouza
Cc: #frameworks

D6687: Adding support to ipv*.route-metric

[Lamarque Souza]

lvsouza requested changes to this revision.
lvsouza added a comment.
This revision now requires changes to proceed.


  The summary says this patch adds route metric support to IPv4 too, but no 
IPv4 file is touched by this patch. Have you missed the IPv4 changes?

INLINE COMMENTS

> ipv6setting.cpp:412
>  
> +if(routeMetric() > 0){
> +setting.insert(QLatin1String(NMQT_SETTING_IP6_CONFIG_ROUTE_METRIC), 
> routeMetric());

You could have added a comment stating that according to NetworkManager's 
documentation zero is not a possible value for IPv6's route metric (the value 
is automatically changed to 1024 when trying to set it to zero).

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D6687

To: pvillani, jgrulich, lvsouza
Cc: #frameworks

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

> On May 4, 2017, 12:40 p.m., Albert Astals Cid wrote:
> > Lamarque, you broke the build.
> 
> Lamarque Souza wrote:
> Fixed. Thanks for the quick report about the broken build and sorry for 
> not adding all files to the commit.
> 
> Ben Cooksley wrote:
> This patch broke the MSVC build and will be reverted shortly. 
> Please see 
> https://build-sandbox.kde.org/job/Frameworks%20solid%20kf5-qt5%20WindowsQt5.7/2/consoleText
>  for the build log.

Well, the simpler solution is removing all those #if, #include and #error 
clauses (lines 31 oto 39 of autotests/solidudisks2test.cpp), they are not 
required. I do not have a MSVC machine with frameworks installed to test that 
now. I will test that after next frameworks release.


- Lamarque


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


On May 4, 2017, 12:32 p.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated May 4, 2017, 12:32 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 128958: Add application/x-openvpn-profile mime type

[Lamarque Souza]

> On May 1, 2017, 2:05 p.m., Lamarque Souza wrote:
> > ping
> 
> David Faure wrote:
> ping? did you see my comment? ;)

Yes, I did see your comment. The "ping" was to this review's author to follow 
your instructions :-) I am one of the Plasma NM developers and there is review 
for Plasma NM that depends on this one, so I am interested in having 
application/x-openvpn-profile mime type available so I can approve the other 
review.


- Lamarque


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


On Sept. 20, 2016, 11:59 a.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128958/
> ---
> 
> (Updated Sept. 20, 2016, 11:59 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> This adds a mime type for .ovpn files used for configuring OpenVPN clients.
> 
> 
> Diffs
> -
> 
>   src/mimetypes/kde5.xml 81ec9a7 
> 
> Diff: https://git.reviewboard.kde.org/r/128958/diff/
> 
> 
> Testing
> ---
> 
> I can now click a file and have plasma-nm's connection editor import it.
> 
> Not sure about the magic, "client" is all I could find which isn't very 
> descriptive, so perhaps better to just look for *.ovpn (I have also seen 
> *.openvpn but *.ovpn seems more common)
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

> On May 4, 2017, 12:40 p.m., Albert Astals Cid wrote:
> > Lamarque, you broke the build.

Fixed. Thanks for the quick report about the broken build and sorry for not 
adding all files to the commit.


- Lamarque


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


On May 4, 2017, 12:32 p.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated May 4, 2017, 12:32 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 128958: Add application/x-openvpn-profile mime type

[Lamarque Souza]

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



ping

- Lamarque Souza


On Sept. 20, 2016, 11:59 a.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128958/
> ---
> 
> (Updated Sept. 20, 2016, 11:59 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> This adds a mime type for .ovpn files used for configuring OpenVPN clients.
> 
> 
> Diffs
> -
> 
>   src/mimetypes/kde5.xml 81ec9a7 
> 
> Diff: https://git.reviewboard.kde.org/r/128958/diff/
> 
> 
> Testing
> ---
> 
> I can now click a file and have plasma-nm's connection editor import it.
> 
> Not sure about the magic, "client" is all I could find which isn't very 
> descriptive, so perhaps better to just look for *.ovpn (I have also seen 
> *.openvpn but *.ovpn seems more common)
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

> On April 28, 2017, 1:28 p.m., Lamarque Souza wrote:
> > Ship It!
> 
> KJ Tsanaktsidis wrote:
> Great - thanks for your help and for bearing with my rusty C++! What 
> happens now? I'm waiting on this patch to land for another patch I submitted 
> a while ago: https://git.reviewboard.kde.org/r/130084/

Well, if you do not have a developer account I can commit the patch for you. In 
order to do that I need your real name and email address to add them to the 
commit's header.


- Lamarque


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


On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 23, 2017, 9:56 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 23, 2017, 9:56 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

> On April 22, 2017, 10:27 a.m., Lamarque Souza wrote:
> > autotests/CMakeLists.txt, line 66
> > <https://git.reviewboard.kde.org/r/130090/diff/5/?file=494817#file494817line66>
> >
> > CMake's developers recommend using else() instead of 
> > else(). The  part used to be required with cmake 
> > 2.6.x, that is not true with cmake 3.x that we use nowadays.
> 
> KJ Tsanaktsidis wrote:
> I'm not sure I understand here - elseif() needs to have the expression to 
> match to enter the elseif() block? In any case I've gone ahead and deleted 
> this because I worked out a cleaner way to do this using cmake generator 
> expressions.

Oh sorry, I did a mistake. I thought it wase a else(). The expression is 
required for elseif(), the old code was correct. You can revert to the old 
code, which require less lines of code. Sorry again.


- Lamarque


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


On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 23, 2017, 9:56 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

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


Fix it, then Ship it!





autotests/CMakeLists.txt (line 66)
<https://git.reviewboard.kde.org/r/130090/#comment68585>

CMake's developers recommend using else() instead of else(). 
The  part used to be required with cmake 2.6.x, that is not true 
with cmake 3.x that we use nowadays.



autotests/CMakeLists.txt (line 68)
<https://git.reviewboard.kde.org/r/130090/#comment68590>

Here too.



autotests/fakeUdisks2.h (line 37)
<https://git.reviewboard.kde.org/r/130090/#comment68586>

Code style: the & also goes next to the method's name instead of the return 
type.



autotests/fakeUdisks2.cpp (line 29)
<https://git.reviewboard.kde.org/r/130090/#comment68587>

Here too.



src/solid/devices/backends/udisks2/CMakeLists.txt (line 32)
<https://git.reviewboard.kde.org/r/130090/#comment68588>

elseif() instead of elseif()



src/solid/devices/backends/udisks2/CMakeLists.txt (line 35)
<https://git.reviewboard.kde.org/r/130090/#comment68589>

Here too.


- Lamarque Souza


On April 22, 2017, 12:38 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 22, 2017, 12:38 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

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




autotests/fakeUdisks2.h (line 35)
<https://git.reviewboard.kde.org/r/130090/#comment68567>

It is common practice to pass QString parameters QString as const &:

FakeUdisks2BlockDevice(QObject *parent, const  device, quint64 
device_number);


- Lamarque Souza


On April 20, 2017, 8:49 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 20, 2017, 8:49 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

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




autotests/fakeUdisks2.h (line 2)
<https://git.reviewboard.kde.org/r/130090/#comment68561>

You should use this file's author name(s). If you are the author then it is 
your name and email address.



autotests/fakeUdisks2.h (line 35)
<https://git.reviewboard.kde.org/r/130090/#comment68562>

Use const QString  and the * goes to the variable, not the type in 
our code style, like this:

FakeUdisks2BlockDevice(QObject *parent, const QString , quint64 
device_number);



autotests/fakeUdisks2.cpp (line 2)
<https://git.reviewboard.kde.org/r/130090/#comment68563>

Your name?



autotests/solidudisks2test.cpp (line 78)
<https://git.reviewboard.kde.org/r/130090/#comment68565>

I think registering to systemBus only works if you are root or has the 
right permissions in dbus' configuration. Have you tested this unit test when 
running as normal user?



autotests/solidudisks2test.cpp (line 96)
<https://git.reviewboard.kde.org/r/130090/#comment68564>

Code style: remove the extra whitespace at the end of the line.


- Lamarque Souza


On April 19, 2017, 11:05 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 19, 2017, 11:05 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Lamarque Souza]

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




src/solid/devices/backends/udisks2/udisksblock.cpp (line 31)
<https://git.reviewboard.kde.org/r/130090/#comment68558>

Usually we use uppper case for macros. That would also save some lines in 
this patch.

nitpik: add spaces around >>, like you did for & in the line below.


- Lamarque Souza


On April 17, 2017, 8:42 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> ---
> 
> (Updated April 17, 2017, 8:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in  by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers,  includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -
> 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 
> 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 
> 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> ---
> 
> I've written a little snippet to iterate through block devices, print their 
> major/minor number, and their device properties. It was previously 
> incorrectly labeling all my disks with major 0 and minor == device_number 
> (since it was using the first 20 bits for the minor). It now correctly 
> identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 123534: Add public dependency on ModemManager

[Lamarque Souza]

> On Feb. 25, 2017, 11:53 p.m., Albert Astals Cid wrote:
> > This patch is whitespace only, i guess because it was commited. If noone 
> > says anything against i'll will close it in two weeks.

Yeah, it is already commited. You can discard it.


- Lamarque


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


On April 27, 2015, 2:44 p.m., Jan Grulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123534/
> ---
> 
> (Updated April 27, 2015, 2:44 p.m.)
> 
> 
> Review request for KDE Frameworks and Lamarque Souza.
> 
> 
> Repository: modemmanager-qt
> 
> 
> Description
> ---
> 
> This patch adds public dependency on ModemManager and also uses 
> FindModemManager.cmake from extra-cmake-modules instead own copy.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 2af6f56 
>   KF5ModemManagerQtConfig.cmake.in d60610a 
>   autotests/CMakeLists.txt 03e1939 
>   cmake/FindModemManager.cmake d522e93 
>   src/CMakeLists.txt 29a7a63 
>   src/fakemodem/CMakeLists.txt b6d4a90 
> 
> Diff: https://git.reviewboard.kde.org/r/123534/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

Re: Review Request 129750: Q_ENUMS -> Q_ENUM and Q_FLAGS -> Q_FLAG

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On Jan. 2, 2017, 11:31 a.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129750/
> ---
> 
> (Updated Jan. 2, 2017, 11:31 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> in src/solid/power/power.h i changed States to InhibitionTypes since States 
> doesn't exist
> 
> https://woboq.com/blog/q_enum.html
> 
> 
> Diffs
> -
> 
>   src/solid/devices/frontend/battery.h e7a9d6d 
>   src/solid/devices/frontend/deviceinterface.h dd212cc 
>   src/solid/devices/frontend/genericinterface.h cf043b1 
>   src/solid/devices/frontend/networkshare.h 3cfde67 
>   src/solid/devices/frontend/opticaldisc.h 9412b73 
>   src/solid/devices/frontend/opticaldrive.h 6f70494 
>   src/solid/devices/frontend/processor.h 2153b2c 
>   src/solid/devices/frontend/storagedrive.h b2f70bc 
>   src/solid/devices/frontend/storagevolume.h 59903ad 
>   src/solid/power/inhibition.h 4616f39 
>   src/solid/power/inhibitionjob.h 0c0c233 
>   src/solid/power/job.h 789bc8c 
>   src/solid/power/power.h 35f9032 
> 
> Diff: https://git.reviewboard.kde.org/r/129750/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129607: Display Application version in About dialog header

[Lamarque Souza]

> On Dec. 3, 2016, 4:48 p.m., Martin Klapetek wrote:
> > How about "Using" for the title of the tab and then removing the "Using" 
> > label from inside the tab?
> > 
> > Also, +1 to putting the version front.
> 
> Thomas Pfeiffer wrote:
> Hm, "Using" sounds a bit weird without context, but "Libraries" or maybe 
> "Libraries used" makes sense to me. The only thing that would not really fit 
> from a purely technical perspective is the windowing system used (though that 
> does not fit in "Version" either because it does not say anything about its 
> version), but I don't think that is a problem.
> 
> Martin Klapetek wrote:
> "Running with" / "Running on" are some other ideas. In any case, I think 
> the "Using" label should now go as its purpose would be now covered by the 
> tab title.

What about "Platform" for the title of the tab?


- Lamarque


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


On Dec. 3, 2016, 1:39 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129607/
> ---
> 
> (Updated Dec. 3, 2016, 1:39 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and Sebastian Kügler.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Last year, a change in kaboutapplicationdialog (review 124133) moved the 
> application version in a separate tab. However for applications, it is really 
> annoying (several Kdenlive users complained and I fully agree) to not have 
> this information clearly displayed as soon as the About dialog is displayed. 
> I understand the interest of having a separate version tab displaying the 
> various underlying libraries used, but I propose to display again the 
> application version in the About dialog header (see screenshot).
> 
> 
> Diffs
> -
> 
>   src/kaboutapplicationdialog.cpp 156410e 
> 
> Diff: https://git.reviewboard.kde.org/r/129607/diff/
> 
> 
> Testing
> ---
> 
> Reverts a recent change, tested and works.
> 
> 
> File Attachments
> 
> 
> About dialog with version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png
> Updated patch
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/ee13cb47-2162-4be7-8255-3dce7f470c2b__about2.png
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

Re: Review Request 128843: Use standard org.freedesktop.DBus.Properties interfaces for monitoring PropertiesChanged signal for NM 1.4.0+

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On Sept. 6, 2016, 11:56 a.m., Jan Grulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128843/
> ---
> 
> (Updated Sept. 6, 2016, 11:56 a.m.)
> 
> 
> Review request for KDE Frameworks, Network Management and Lamarque Souza.
> 
> 
> Repository: networkmanager-qt
> 
> 
> Description
> ---
> 
> Since NM 1.2.x they switched to using o.f.DBus.Properties for sending 
> PropertiesChanged signal. They still support their previous solution which 
> uses their o.f.NetworkManager.Foo interfaces, but with release of NM 1.4.0 
> this is a bit broken (altough already fixed in upstream). With the fix for 
> that they marked PropertiesChanged signal for their o.f.NM.Foo interfaces as 
> deprecated and we should be slowly moving towards the standardized 
> o.f.DBus.Properties interface instead.
> 
> See https://bugzilla.gnome.org/show_bug.cgi?id=770629
> 
> 
> Diffs
> -
> 
>   src/accesspoint.cpp d1abe31 
>   src/accesspoint_p.h bf111c8 
>   src/activeconnection.cpp fc8a1ac 
>   src/activeconnection_p.h d021437 
>   src/adsldevice.cpp f07b5e6 
>   src/bluetoothdevice.cpp 3ccf276 
>   src/bonddevice.cpp d9798b0 
>   src/bridgedevice.cpp f345ad5 
>   src/connection.cpp 5b80a93 
>   src/connection_p.h 5f43d93 
>   src/device.cpp f7bc7e6 
>   src/device_p.h da85faf 
>   src/dhcp4config.cpp efcd98d 
>   src/dhcp4config_p.h ce5563c 
>   src/dhcp6config.cpp 4f0a3b6 
>   src/dhcp6config_p.h 093f234 
>   src/genericdevice.cpp a6919d5 
>   src/gredevice.cpp 1f81ea4 
>   src/infinibanddevice.cpp 15b3575 
>   src/macvlandevice.cpp db2c594 
>   src/manager.cpp 01723d7 
>   src/manager_p.h d0a9d8c 
>   src/modemdevice.cpp 89394f0 
>   src/olpcmeshdevice.cpp 286a080 
>   src/settings.cpp f75b86a 
>   src/settings_p.h 466d0a7 
>   src/teamdevice.cpp 543d26d 
>   src/tundevice.cpp 68eb103 
>   src/vethdevice.cpp bb7c9fd 
>   src/vlandevice.cpp 415dec9 
>   src/vpnconnection.cpp d2d17e4 
>   src/vpnconnection_p.h 25ba589 
>   src/wimaxdevice.cpp daec0a6 
>   src/wireddevice.cpp af147b1 
>   src/wirelessdevice.cpp dad918d 
> 
> Diff: https://git.reviewboard.kde.org/r/128843/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

Re: Review Request 128843: Use standard org.freedesktop.DBus.Properties interfaces for monitoring PropertiesChanged signal for NM 1.4.0+

[Lamarque Souza]

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




src/adsldevice.cpp (line 46)
<https://git.reviewboard.kde.org/r/128843/#comment66594>

There is no dbusPropertiesChanged slot in AdslDevicePrivate. This line will 
issue a warning when loading any application that makes use of NetworkManagerQt.



src/bluetoothdevice.cpp (line 47)
<https://git.reviewboard.kde.org/r/128843/#comment66595>

Same here.



src/bonddevice.cpp (line 54)
<https://git.reviewboard.kde.org/r/128843/#comment66596>

Same here.



src/bridgedevice.cpp (line 52)
<https://git.reviewboard.kde.org/r/128843/#comment66597>

Same here.



src/device.cpp (line 574)
<https://git.reviewboard.kde.org/r/128843/#comment66598>

You did not connect any signal to this slot.



src/genericdevice.cpp (line 47)
<https://git.reviewboard.kde.org/r/128843/#comment66599>

There is no dbusPropertiesChanged slot in the d-pointer.



src/gredevice.cpp (line 56)
<https://git.reviewboard.kde.org/r/128843/#comment66600>

There is no dbusPropertiesChanged slot in the d-pointer.



src/infinibanddevice.cpp (line 49)
<https://git.reviewboard.kde.org/r/128843/#comment66601>

Same here.



src/macvlandevice.cpp (line 48)
<https://git.reviewboard.kde.org/r/128843/#comment66602>

There is no dbusPropertiesChanged slot in the d-pointer.



src/modemdevice.cpp (line 59)
<https://git.reviewboard.kde.org/r/128843/#comment66603>

Maybe you should use Qt::UniqueConnection type here since the constructor 
below also connects this same signal to the same slot. Just to make sure we do 
not endup processing the same signal twice.



src/olpcmeshdevice.cpp (line 47)
<https://git.reviewboard.kde.org/r/128843/#comment66604>

There is no dbusPropertiesChanged slot in the d-pointer.



src/teamdevice.cpp (line 52)
<https://git.reviewboard.kde.org/r/128843/#comment66605>

There is no dbusPropertiesChanged slot in the d-pointer.



src/tundevice.cpp (line 51)
<https://git.reviewboard.kde.org/r/128843/#comment66606>

There is no dbusPropertiesChanged slot in the d-pointer.



src/vethdevice.cpp (line 48)
<https://git.reviewboard.kde.org/r/128843/#comment66607>

There is no dbusPropertiesChanged slot in the d-pointer.



src/vlandevice.cpp (line 52)
<https://git.reviewboard.kde.org/r/128843/#comment66608>

There is no dbusPropertiesChanged slot in the d-pointer.



src/wimaxdevice.cpp (line 59)
<https://git.reviewboard.kde.org/r/128843/#comment66609>

There is no dbusPropertiesChanged slot in the d-pointer.



src/wireddevice.cpp (line 55)
<https://git.reviewboard.kde.org/r/128843/#comment66610>

There is no dbusPropertiesChanged slot in the d-pointer.



src/wirelessdevice.cpp (line 71)
<https://git.reviewboard.kde.org/r/128843/#comment66611>

There is no dbusPropertiesChanged slot in the d-pointer. It's odd not 
having a propertiesChanged slot for wireless. Is this correct?


- Lamarque Souza


On Sept. 6, 2016, 11:56 a.m., Jan Grulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128843/
> ---
> 
> (Updated Sept. 6, 2016, 11:56 a.m.)
> 
> 
> Review request for KDE Frameworks, Network Management and Lamarque Souza.
> 
> 
> Repository: networkmanager-qt
> 
> 
> Description
> ---
> 
> Since NM 1.2.x they switched to using o.f.DBus.Properties for sending 
> PropertiesChanged signal. They still support their previous solution which 
> uses their o.f.NetworkManager.Foo interfaces, but with release of NM 1.4.0 
> this is a bit broken (altough already fixed in upstream). With the fix for 
> that they marked PropertiesChanged signal for their o.f.NM.Foo interfaces as 
> deprecated and we should be slowly moving towards the standardized 
> o.f.DBus.Properties interface instead.
> 
> See https://bugzilla.gnome.org/show_bug.cgi?id=770629
> 
> 
> Diffs
> -
> 
>   src/accesspoint.cpp d1abe31 
>   src/accesspoint_p.h bf111c8 
>   src/activeconnection.cpp fc8a1ac 
>   src/activeconnection_p.h d021437 
>   src/adsldevice.cpp f07b5e6 
>   src/bluetoothdevice.cpp 3ccf276 
>   src/bonddevice.cpp d9798b0 
>   src/bridgedevice.cpp f345ad5 
>   src/connection.cpp 5b80a93 
>   src/connection_p.h 5f43d93 
>   src/device.cpp f7bc7e6 
>   src/device_p.h da85faf 
>   src/dhcp4config.cpp efcd98d 
>   src/dhcp4config_p.h ce5563c 
>   src/dhcp6config.cpp 4f0a3b6 
>   src/dhcp6config_p.h 093f234 
>   src/genericdevice.cpp a6919

Re: Review Request 128083: Find CPUs by subsystem rather than driver

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On June 3, 2016, 12:15 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128083/
> ---
> 
> (Updated June 3, 2016, 12:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Solid.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> The driver "processor" is part of the ACPI module, which isn't used by
> all systems and architectures.
> 
> Detecting devices by CPU subsystem seems to work, and whilst
> theoretically this change could pick up a non CPU device in the same 
> subsystem they
> would be filtered out by the udevmanager which has an extra check
> that CPUs are valid and not just empty sockets.
> 
> 
> Diffs
> -
> 
>   src/solid/devices/backends/udev/udevdevice.cpp 
> 9fb5e092679cd3c6860b2055cf3dedb915addfda 
>   src/solid/devices/backends/udev/udevmanager.cpp 
> 3f3a671798e84e6d577df7c3b9b80150ac4d01fc 
> 
> Diff: https://git.reviewboard.kde.org/r/128083/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Review Request 128084: Handle reading /proc/cpuinfo for Arm processors

[Lamarque Souza]

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




src/solid/devices/backends/udev/cpuinfo.cpp (line 70)
<https://git.reviewboard.kde.org/r/128084/#comment65041>

This extracts the current cpu speed, not the maximum. Is that what you 
want? Yes then I think you should rename extractCpuSpeed() to 
extractCurrentCpuSpeed().


- Lamarque Souza


On June 3, 2016, 12:15 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128084/
> ---
> 
> (Updated June 3, 2016, 12:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Solid.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> The /proc/cpuinfo format is wildly different between architectures, our
> current method for scraping data only works on x86.
> 
> This patch moves all parsing logic into cpuinfo.cpp and adds a fallback
> search that works for arm and arm64 architectures.
> 
> 
> https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/proc.c#L55
> 
> https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/cpuinfo.c#L104
> 
> 
> Diffs
> -
> 
>   src/solid/devices/backends/udev/cpuinfo.h 
> e8d5dda729535e767b4640f41a01f4d0e27e6542 
>   src/solid/devices/backends/udev/cpuinfo.cpp 
> ecaad58ceee34d46343b67015c03d9a3de87ee72 
>   src/solid/devices/backends/udev/udevdevice.cpp 
> 9fb5e092679cd3c6860b2055cf3dedb915addfda 
>   src/solid/devices/backends/udev/udevprocessor.cpp 
> 80d14f5b40b4478a32d7c113747fe9da50e54b6f 
> 
> Diff: https://git.reviewboard.kde.org/r/128084/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Review Request 128085: Fix check that CPU is a valid CPU

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On June 3, 2016, 12:15 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128085/
> ---
> 
> (Updated June 3, 2016, 12:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Solid.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> We have a check that an devices/system/cpu/cpuN entry contains a
> processor and not just an empty socket, however it relies on somewhat
> outdated /sys files.
> 
> cpuN/cpufreq isn't guaranteed to exist as for some systems (my AMD
> processor at least).
> 
> The docs in the linux kernel imply topology/core_id should always exist,
> and should still work as a valid check that we have a populated CPU
> socket.
> 
> 
> Diffs
> -
> 
>   src/solid/devices/backends/udev/udevmanager.cpp 
> 3f3a671798e84e6d577df7c3b9b80150ac4d01fc 
> 
> Diff: https://git.reviewboard.kde.org/r/128085/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Broken ABI in networkmanager-qt

[Lamarque Souza]

On Tue, May 17, 2016 at 8:36 AM, Harald Sitter  wrote:

> On Tue, May 17, 2016 at 11:06 AM, Jan Grulich  wrote:
> > Hi,
> >
> > we decided to drop WiMAX support in nm-qt when it's compiled against NM
> 1.2.0,
> > but this seems to break binary compatibility when nm-qt was previously
> build
> > against older NM version. I didn't realize this before that this could
> happen
> > and now I'm not sure how fix that.
> >
> > We could either:
> > 1) Revert the change removing WiMAX support, but that would break ABI
> > compatibility one more time.
> >
> > 2) Keep it as it is and let packagers know about this problem and ask
> them to
> > rebuild everything using nm-qt (plasma-nm, plasma-workspace) in case they
> > already have NM 1.2.0.
> >
> > What do you think is a better option?
>
> I am not sure how 1) would be an issue, or how it would break ABI
> again for that matter.
> If you add the relevant ABI back you are doing an ABI addition on top
> of the effectively new ABI, and adding things generally is a binary
> compatible change to both the original ABI (which would be back again)
> as well as the new semi-broken ABI as it would simply expand to again
> contain the removed ABIs. That said 1) is the better option. If one
> has built on top of nm-1.2, binary compatibility would be restored as
> the previous ABIs are back. If one hasn't built on top of nm-1.2,
> nothing would change and it would be as though the binary incompatible
> change never happened.
>
>
I agree with Harald here. manager.h is the only affected public header. You
just need to re-add the five wimax related methods with an empty
implementation and it should work. Fortunately, none of those methods are
virtual, so adding them does not break ABI.


> 2) would imply that we disregard the frameworks ABI stability promise
> which probably wouldn't be very nice. This option would also at the
> very least require a so-version bump to communicate the breakage
> properly.
>
> HS
>

No ABI will be broken by re-adding those methods. Again, if any of those
methods were virtual then re-adding them would indeed break ABI.


Lamarque V. Souza

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

Re: Review Request 127209: Fix some issues found by Coverity

[Lamarque Souza]

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




src/lib/kaboutdata.cpp (line 993)
<https://git.reviewboard.kde.org/r/127209/#comment63332>

I think you must change the second "%s" to "%1" here, otherwise the value 
returned by qAppName() will be added to the string created by 
QCoreApplication::translate().



src/lib/kaboutdata.cpp (line 1006)
<https://git.reviewboard.kde.org/r/127209/#comment6>

    Same here.


- Lamarque Souza


On Feb. 28, 2016, 7:46 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127209/
> ---
> 
> (Updated Feb. 28, 2016, 7:46 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Mostly initializing values and using printf() correctly.
> 
> 
> Diffs
> -
> 
>   src/lib/io/kdirwatch.cpp f2fd3b8 
>   src/lib/io/kprocess_p.h 70fe91f 
>   src/lib/kaboutdata.cpp a220977 
>   src/lib/randomness/krandom.cpp bdbdec6 
>   src/lib/util/kformatprivate.cpp 3d98007 
> 
> Diff: https://git.reviewboard.kde.org/r/127209/diff/
> 
> 
> Testing
> ---
> 
> 
> 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 126222: Fix rating widget flickering when selecting rating with mouse

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On Dec. 3, 2015, 10:19 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126222/
> ---
> 
> (Updated Dec. 3, 2015, 10:19 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Lamarque Souza.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Refactor the rating calculation:
> 1. Disable feature introduced in bug 171343 when halfSteps is enabled.
> 2. Merge the two pieces of code, and try to make it clearer and simpler.
> 3. make hover 0 star also show visual effect.
> 
> 
> Diffs
> -
> 
>   src/kratingpainter.cpp 1a4378a 
>   src/kratingwidget.cpp d0d2903 
> 
> Diff: https://git.reviewboard.kde.org/r/126222/diff/
> 
> 
> Testing
> ---
> 
> Current rating is 0, hover on the 1st star, 1 star lights up, click on 1st 
> star, rating becomes 2.
> 
> Current rating is 2. hover on the 1st star, half star lights up, click on 1st 
> star, rating becomes 1.
> 
> Current rating is 1/2. move mouse to the left most edge, star's color becomes 
> lighter. Click and rating becomes 0.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126222: Fix rating widget flickering when selecting rating with mouse

[Lamarque Souza]

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



src/kratingwidget.cpp (line 181)
<https://git.reviewboard.kde.org/r/126222/#comment60992>

Code style: move the bracket at the end of this line to the next line.


- Lamarque Souza


On Dec. 3, 2015, 10:19 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126222/
> ---
> 
> (Updated Dec. 3, 2015, 10:19 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Lamarque Souza.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Refactor the rating calculation:
> 1. Disable feature introduced in bug 171343 when halfSteps is enabled.
> 2. Merge the two pieces of code, and try to make it clearer and simpler.
> 3. make hover 0 star also show visual effect.
> 
> 
> Diffs
> -
> 
>   src/kratingpainter.cpp 1a4378a 
>   src/kratingwidget.cpp d0d2903 
> 
> Diff: https://git.reviewboard.kde.org/r/126222/diff/
> 
> 
> Testing
> ---
> 
> Current rating is 0, hover on the 1st star, 1 star lights up, click on 1st 
> star, rating becomes 2.
> 
> Current rating is 2. hover on the 1st star, half star lights up, click on 1st 
> star, rating becomes 1.
> 
> Current rating is 1/2. move mouse to the left most edge, star's color becomes 
> lighter. Click and rating becomes 0.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126222: Fix rating widget flickering when selecting rating with mouse

[Lamarque Souza]

> On Dec. 2, 2015, 4:45 p.m., Lamarque Souza wrote:
> > That code was added to fix this bug 
> > https://bugs.kde.org/show_bug.cgi?id=171343#c6
> 
> Xuetian Weng wrote:
> Emm.. interesting feature. But code inside mouseMoveEvent looks fishy, 
> and this feature shouldn't be used when halfStepsEnabled is true. I'll try to 
> keep this feature then.

There is no change in the diff between revisions 1 and 2. Could you update the 
diff again?


- Lamarque


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


On Dec. 2, 2015, 11:37 p.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126222/
> ---
> 
> (Updated Dec. 2, 2015, 11:37 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Lamarque Souza.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Refactor the rating calculation:
> 1. Disable feature introduced in bug 171343 when halfSteps is enabled.
> 2. Merge the two pieces of code, and try to make it clearer and simpler.
> 3. make hover 0 star also show visual effect.
> 
> 
> Diffs
> -
> 
>   src/kratingwidget.cpp d0d2903 
> 
> Diff: https://git.reviewboard.kde.org/r/126222/diff/
> 
> 
> Testing
> ---
> 
> Current rating is 0, hover on the 1st star, 1 star lights up, click on 1st 
> star, rating becomes 2.
> 
> Current rating is 2. hover on the 1st star, half star lights up, click on 1st 
> star, rating becomes 1.
> 
> Current rating is 1/2. move mouse to the left most edge, star's color becomes 
> lighter. Click and rating becomes 0.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126222: Fix rating widget flickering when selecting rating with mouse

[Lamarque Souza]

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


That code was added to fix this bug 
https://bugs.kde.org/show_bug.cgi?id=171343#c6

- Lamarque Souza


On Dec. 2, 2015, 4:34 p.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126222/
> ---
> 
> (Updated Dec. 2, 2015, 4:34 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> mousePressed and mouseMoved seems to contains some hack code. I don't know 
> what these hacks are originally for, but it seems that they are not valid 
> anymore and causes problem.
> 
> Currently, whenever a mouse move happens, the rating value cycles between 0, 
> 1, 2. This change drops all hack code.
> 
> 
> Diffs
> -
> 
>   src/kratingwidget.cpp d0d2903 
> 
> Diff: https://git.reviewboard.kde.org/r/126222/diff/
> 
> 
> Testing
> ---
> 
> No flickering anymore when selecting rating.
> Tested with following case, all cases are able to select all possible rating 
> value.
> setMaxRating(2) setHalfStepsEnabled(false)
> setMaxRating(1) setHalfStepsEnabled(false)
> setMaxRating(1) setHalfStepsEnabled(true)
> setMaxRating(5) setHalfStepsEnabled(true)
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126161: OS X housekeeping

[Lamarque Souza]

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



src/kdeinit/kdeinit5_proxy.cpp (line 1)
<https://git.reviewboard.kde.org/r/126161/#comment60862>

Please add a copyright header for each new file [1]. If I am not mistaken 
all frameworks 5 files should use LGLP+ license.

[1] https://techbase.kde.org/Policies/Licensing_Policy


- Lamarque Souza


On Nov. 24, 2015, 11:03 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> ---
> 
> (Updated Nov. 24, 2015, 11:03 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> This patch addresses several issues with the OS X adaptations:
> 
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true 
> programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 
> 14th 2009, which prevents a kdeinit crash that is caused by calling exec 
> after `fork()` in an application that has used non-POSIX APIs and/or calling 
> those APIs in the exec'ed application. This patch (originally by MacPorts 
> developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a 
> proxy application to do the actual exec.
> 
> 
> Diffs
> -
> 
>   src/kdeinit/CMakeLists.txt f94db71 
>   src/kdeinit/kdeinit5_proxy.cpp PRE-CREATION 
>   src/kdeinit/kinit.cpp a18008a 
>   src/klauncher/CMakeLists.txt 746edfa 
>   src/klauncher/klauncher.h e155f72 
>   src/klauncher/klauncher.cpp 8b3d343 
>   src/klauncher/klauncher_main.cpp f69aaa5 
>   src/start_kdeinit/CMakeLists.txt 46d6cb3 
> 
> Diff: https://git.reviewboard.kde.org/r/126161/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 . With this patch, 
> starting `kded5` will launch kdeinit5 and klauncher5 as expected, but 
> `kdeinit5 --kded` does not yet launch `kded5`. This is probably acceptable 
> for typical KF5 use on OS X (kded5 can be launched as a login item or as a 
> LaunchAgent) but I will have another look at why the kded isn't started.
> 
> I am not yet able to perform further testing; practice will for instance have 
> to show whether point 2 above needs revision (apps that need to be installed 
> as app bundles).
> 
> Similarly it will have to be seen whether point 3 above has any drawbacks. 
> Applications running as agents do not show up in the Dock or App Switcher. 
> Thus, klauncher will not be able to "turn itself into" an application that 
> does have a full GUI presence with my current modification. I don't know if 
> that's supposed to be possible though.
> NB: I have been building the KDE4 klauncher in a way that makes it impossible 
> to construct a GUI at all, so I'm not expecting issues in KF5 as long as 
> klauncher's role hasn't evolved too much.
> 
> 
> 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 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Lamarque Souza]

> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.h, line 24
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417216#file417216line24>
> >
> > Nitpick: this should go after #include 
> 
> René J.V. Bertin wrote:
> Any guidelines that dictate this?

https://wiki.qt.io/Coding_Conventions there will be more guidelines in 
https://techbase.kde.org/Policies/Frameworks_Coding_Style, until there please 
use Qt's code conventions.


> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 178
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line178>
> >
> > Use qCWarning instead.
> 
> René J.V. Bertin wrote:
> I presume that should be using KIDLETIME for the category?
> 
> Isn't it possible to get the `KIDLETIME()` symbol through libKF5KIdleTime 
> rather than having to pull in `../../logging.cpp`?

Read https://community.kde.org/Frameworks/Porting_To_qCDebug for more 
information on how to define KIDLETIME.


> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 213
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line213>
> >
> > Shouldn't you check the return value of this method?
> 
> René J.V. Bertin wrote:
> As far as I can see all it can be used for is to print a warning, right?

I guess so, since the "additional" prefix implies that it is not required.


> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 215
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line215>
> >
> > This should be changed to "return true", right?
> 
> René J.V. Bertin wrote:
> Yeah, that seems logical, but I don't see any documentation on what 
> setupPoller() should return. Then again the upstream code doesn't appear to 
> use the return value anyway so the question is a bit moot ...

The point is that you changed the semantics of this method. The original code 
returns true on all successful setUpPoller() calls. Now it returns false on the 
first successfull setUpPoller() call and returns true only on the second call 
and on. Besides, this is a framework, do not assume that there will always be 
just one "upstream" for a framework.


> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 422
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line422>
> >
> > Remove trailing space and shouldn't the commented code in this method 
> > be removed?
> 
> René J.V. Bertin wrote:
> I put the commented code as a reference for alternative approaches that 
> could be investigated once more if ever `updateSystemActivity` is removed. If 
> there's a policy against such comments I can also store the snippets in a 
> separate file, but that means adding one. What's preferable?

Well, it is a general consensus that adding commented code is not a good 
programming practice. I am not aware of any written recomendation in Frameworks 
or Qt code conventions about that but I have never seen a patch with commented 
code to pass any code review either.


> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 78
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line78>
> >
> > There is no point in doing this if the next line will delete 
> > nativeGrabber.
> 
> René J.V. Bertin wrote:
> I've learned to be fool-proof with this kind of thing (I don't trust 
> `delete` to zero memory before releasing it; there's no 
> `~CocoaEventFilter()`) but what does make it redundant here is setting 
> `m_nativeGrabber = 0` just after deleting `nativeGrabber`.

Delete never zeros memory before releasing it for performance reasons. The 
overhead of zeroing m_monitorId here is minimum though. Anyway nobody will be 
able to access m_monitorId after the next line or the program will crash.


> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 54
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line54>
> >
> > Usually when a new operation returns 0 it is because system is on short 
> > on RAM memory (or memory is too fragmented). I would add assert here 
> > instead of silently ignoring the failure to allocate memory.
> 
> René J.V. Bertin wrote:
> And I would argue that it is up to the calling software to decide how to 
> react to a failure to set up idle detection: in this

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

[Lamarque Souza]

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



src/plugins/osx/macpoller.h (line 24)
<https://git.reviewboard.kde.org/r/126078/#comment60663>

Nitpick: this should go after #include 



src/plugins/osx/macpoller.h (line 75)
<https://git.reviewboard.kde.org/r/126078/#comment60678>

One line per variable.



src/plugins/osx/macpoller.h (line 76)
<https://git.reviewboard.kde.org/r/126078/#comment60679>

Here too.



src/plugins/osx/macpoller.cpp (line 2)
<https://git.reviewboard.kde.org/r/126078/#comment60664>

This should be appended to the list, not prepended. I know, the next line 
is wrong too.



src/plugins/osx/macpoller.cpp (line 29)
<https://git.reviewboard.kde.org/r/126078/#comment60665>

Remove trailing space character.



src/plugins/osx/macpoller.cpp (line 49)
<https://git.reviewboard.kde.org/r/126078/#comment60677>

Initialize m_nativeGrabber here too.



src/plugins/osx/macpoller.cpp (line 100)
<https://git.reviewboard.kde.org/r/126078/#comment60681>

Use qCWarning instead.



src/plugins/osx/macpoller.cpp (line 135)
<https://git.reviewboard.kde.org/r/126078/#comment60666>

Shouldn't you check the return value of this method?



src/plugins/osx/macpoller.cpp (line 137)
<https://git.reviewboard.kde.org/r/126078/#comment60667>

This should be changed to "return true", right?



src/plugins/osx/macpoller.cpp (line 212)
<https://git.reviewboard.kde.org/r/126078/#comment60669>

We usually join the last } with the next "else {", like this:

} else {



src/plugins/osx/macpoller.cpp (line 221)
<https://git.reviewboard.kde.org/r/126078/#comment60668>

Remove commented code.



src/plugins/osx/macpoller.cpp (line 242)
<https://git.reviewboard.kde.org/r/126078/#comment60670>

We usually do not use extra space after ( and before )



src/plugins/osx/macpoller.cpp (line 274)
<https://git.reviewboard.kde.org/r/126078/#comment60671>

Remove commented code.



src/plugins/osx/macpoller.cpp (line 325)
<https://git.reviewboard.kde.org/r/126078/#comment60672>

I do not get the "Unsetting m_catch is enough" comment. What is it supposed 
to mean since unsetting m_catch is the original code.



src/plugins/osx/macpoller.cpp (line 343)
<https://git.reviewboard.kde.org/r/126078/#comment60673>

Remove trailing space and shouldn't the commented code in this method be 
removed?



src/plugins/osx/macpoller_helper.mm (line 2)
<https://git.reviewboard.kde.org/r/126078/#comment60674>

Append new entries in copyright section, do not prepend.



src/plugins/osx/macpoller_helper.mm (line 43)
<https://git.reviewboard.kde.org/r/126078/#comment60680>

Use qCDebug instead.



src/plugins/osx/macpoller_helper.mm (line 54)
<https://git.reviewboard.kde.org/r/126078/#comment60676>

Usually when a new operation returns 0 it is because system is on short on 
RAM memory (or memory is too fragmented). I would add assert here instead of 
silently ignoring the failure to allocate memory.



src/plugins/osx/macpoller_helper.mm (line 78)
<https://git.reviewboard.kde.org/r/126078/#comment60682>

There is no point in doing this if the next line will delete nativeGrabber.



src/plugins/osx/macpoller_helper.mm (line 82)
<https://git.reviewboard.kde.org/r/126078/#comment60675>

This line can go to inside the if clause above.


- Lamarque Souza


On Nov. 17, 2015, 9:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 9:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Test

Re: Review Request 124714: Add support to qmake's options to api.kde.org

[Lamarque Souza]

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

(Updated Nov. 8, 2015, 9:40 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Alex Merry.


Changes
---

Submitted with commit b3693ce2960c96b6bfec5d557ef75df5e18ef05d by Lamarque V. 
Souza to branch master.


Repository: kapidox


Description
---

NetworkManagerQt requires some options to be added to .pro file to compile:

QT += NetworkManagerQt dbus network
CONFIG += link_pkgconfig
PKGCONFIG += libnm # from NetworkManager package
DEFINES += QT_NO_KEYWORDS # libnm declares a variable named signals

kapidox already supports the first option (QT), but not the others. This patch 
adds support to adding the other three options.


Diffs
-

  src/kapidox/data/templates/fwinfo.html a20c1d1 

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


Testing
---

I added the lines below to networkmanager-qt/metainfo.html and kapidox created 
the correct output:

qmakepro:
 - CONFIG += link_pkgconfig
 - DEFINES += QT_NO_KEYWORDS
 - PKGCONFIG += libnm


Thanks,

Lamarque Souza

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

Re: Review Request 124714: Add support to qmake's options to api.kde.org

[Lamarque Souza]

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

(Updated Nov. 8, 2015, 1:21 a.m.)


Review request for KDE Frameworks and Alex Merry.


Changes
---

This version allows adding all lines from qmakepro section to the output html 
file.


Repository: kapidox


Description
---

NetworkManagerQt requires some options to be added to .pro file to compile:

QT += NetworkManagerQt dbus network
CONFIG += link_pkgconfig
PKGCONFIG += libnm # from NetworkManager package
DEFINES += QT_NO_KEYWORDS # libnm declares a variable named signals

kapidox already supports the first option (QT), but not the others. This patch 
adds support to adding the other three options.


Diffs (updated)
-

  src/kapidox/data/templates/fwinfo.html a20c1d1 

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


Testing (updated)
---

I added the lines below to networkmanager-qt/metainfo.html and kapidox created 
the correct output:

qmakepro:
 - CONFIG += link_pkgconfig
 - DEFINES += QT_NO_KEYWORDS
 - PKGCONFIG += libnm


Thanks,

Lamarque Souza

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

Re: Review Request 124714: Add support to qmake's options to api.kde.org

[Lamarque Souza]

> On Nov. 3, 2015, 7:42 p.m., Alex Merry wrote:
> > Sorry I've let this sit for so long - Real Life(TM) got in the way.
> > 
> > Rather than adding variables for each of these, I'd rather have some way to 
> > add free text to this field. Also, have you checked that an extra line 
> > break doesn't get added when the fields are not set?
> > 
> > Finally, have you explored ways to make these *not* be required? I don't 
> > know QMake's build system very well, but it seems to me that at least some 
> > of these might be able to go in the .pri file (and it might be possible to 
> > protect client projects from the need to set QT_NO_KEYWORDS).

I have changed the patch to allow adding (almost) free text to the field. The 
dash at each line start is still needed to mark linebreaks in the output html 
file.

I am not able to compile networkmanager-qt using qmake from Qt 5.4.2 anymore, 
so I cannot test your suggestion to move some of the configurations to .pri 
file. Anyway, the current patch seems to work and does not add unnecessary 
fields like the first version.


- Lamarque


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


On Nov. 8, 2015, 1:21 a.m., Lamarque Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124714/
> ---
> 
> (Updated Nov. 8, 2015, 1:21 a.m.)
> 
> 
> Review request for KDE Frameworks and Alex Merry.
> 
> 
> Repository: kapidox
> 
> 
> Description
> ---
> 
> NetworkManagerQt requires some options to be added to .pro file to compile:
> 
> QT += NetworkManagerQt dbus network
> CONFIG += link_pkgconfig
> PKGCONFIG += libnm # from NetworkManager package
> DEFINES += QT_NO_KEYWORDS # libnm declares a variable named signals
> 
> kapidox already supports the first option (QT), but not the others. This 
> patch adds support to adding the other three options.
> 
> 
> Diffs
> -
> 
>   src/kapidox/data/templates/fwinfo.html a20c1d1 
> 
> Diff: https://git.reviewboard.kde.org/r/124714/diff/
> 
> 
> Testing
> ---
> 
> I added the lines below to networkmanager-qt/metainfo.html and kapidox 
> created the correct output:
> 
> qmakepro:
>  - CONFIG += link_pkgconfig
>  - DEFINES += QT_NO_KEYWORDS
>  - PKGCONFIG += libnm
> 
> 
> Thanks,
> 
> Lamarque Souza
> 
>

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

Re: Documentation on adding new Solid Backends

[Lamarque Souza]

Hi,

The documentation is located in
http://techbase.kde.org/Development/Tutorials/Solid_Tutorials. I am afraid
it is scarse and in some cases outdated. You can also take a look at
available backends in git://anongit.kde.org/solid/src/solid/devices/backends
as examples, specially udev and udisks2 backends. Basically your backend
must implement one or more interfaces from solid/src/solid/devices/ifaces.
For the network part of devd I do not think Solid has something that can
fit in now. Solid used to provide interface to manage network
(de)activation until Plasma 4.9, but not anymore. Our network applet
(Plasma NM) talks to NetworkManager throught NetworkManager-Qt framework
instead now. I am afraid you will need to implement a new applet (plasmoid)
to manage network (de)activation in FreeBSD if NetworkManager is not
available there.

Lamarque V. Souza

http://planetkde.org/pt-br

On Fri, Sep 4, 2015 at 5:43 PM, Tobias Berner  wrote:

> Hi there
>
> Im trying to figure out how to implement a devd-Backend for Solid on
> FreeBSD,
> But I'm unable to find any information on the topic of solid-backends.
>
> Any pointers would be greatly appreciated.
>
>
> mfg Tobias
> ___
> Kde-frameworks-devel mailing list
> Kde-frameworks-devel@kde.org
> https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124963: New widget: KNewPasswordWidget

[Lamarque Souza]

 On Aug. 28, 2015, 11:30 a.m., Lamarque Souza wrote:
  src/knewpasswordwidget.cpp, line 75
  https://git.reviewboard.kde.org/r/124963/diff/1/?file=399453#file399453line75
 
  i18n instead of tr
 
 Sune Vuorela wrote:
 Aren't we in a tier1 framework where i18n and friends aren't available?

Yeah, you're right. Please revert the i18n change and sorry for not noticing 
that before.


- Lamarque


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


On Aug. 28, 2015, 3:52 p.m., Elvis Angelaccio wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124963/
 ---
 
 (Updated Aug. 28, 2015, 3:52 p.m.)
 
 
 Review request for KDE Frameworks and Christoph Feck.
 
 
 Repository: kwidgetsaddons
 
 
 Description
 ---
 
 This widget is a stripped-down version of KNewPasswordDialog, without any 
 dialog-specific stuff.
 
 ### Why a new widget?
 
 This widget is meant to be easily embedded in any custom password dialog, 
 including KNewPasswordDialog. It's the least common denominator of features 
 that any password dialog should offer to the user.
 
 ### Features
 
 * Password visibility action (same as RR 124698). The password verification 
 field is hidden if the user shows the password.
 * Background warning colour. The password verification field gets a coloured 
 background whenever the password and its verification don't match. (feature 
 borrowed from Keepass)
 * Password status signal. This allows the upper level dialogs to update their 
 stuff (enable/disable OK button, show warnings for low password strength, 
 etc.)
 * Password strength bar can be hidden.
 * Unit test.
 
 ### Use cases
 At least the following:
 
 * Ark (new archive dialog)
 * KNewPasswordDialog refactoring (my next RR if this one gets accepted)
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt ac708ef33e3be89db85d43911f96e536c52f741d 
   autotests/knewpasswordwidgettest.h PRE-CREATION 
   autotests/knewpasswordwidgettest.cpp PRE-CREATION 
   src/CMakeLists.txt e03e9bbd6d73811873b0a465f86da269f4295138 
   src/knewpasswordwidget.h PRE-CREATION 
   src/knewpasswordwidget.cpp PRE-CREATION 
   src/knewpasswordwidget.ui PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124963/diff/
 
 
 Testing
 ---
 
 
 File Attachments
 
 
 knewpasswordwidget1.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/5796469c-28b7-4151-b9cb-6a327631db75__knewpasswordwidget1.png
 knewpasswordwidget2.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/b3bfc9ac-ab8e-404c-8091-b5ad9e3e054f__knewpasswordwidget2.png
 knewpasswordwidget3.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/dfaeea4e-65da-4961-b266-986a21f54ca7__knewpasswordwidget3.png
 
 
 Thanks,
 
 Elvis Angelaccio
 


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

Re: Review Request 124963: New widget: KNewPasswordWidget

[Lamarque Souza]

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



src/knewpasswordwidget.h (line 179)
https://git.reviewboard.kde.org/r/124963/#comment58492

s/strenght/strength/



src/knewpasswordwidget.h (line 185)
https://git.reviewboard.kde.org/r/124963/#comment58493

s/has/returns/



src/knewpasswordwidget.cpp (line 35)
https://git.reviewboard.kde.org/r/124963/#comment58494

Also initialize passwordStatus and toggleEchoModeAction variables here.



src/knewpasswordwidget.cpp (line 63)
https://git.reviewboard.kde.org/r/124963/#comment58495

i18n instead of tr. You can also use ritch text mode here:

http://doc.qt.io/qt-5.4/richtext-html-subset.html



src/knewpasswordwidget.cpp (line 75)
https://git.reviewboard.kde.org/r/124963/#comment58496

i18n instead of tr



src/knewpasswordwidget.cpp (line 92)
https://git.reviewboard.kde.org/r/124963/#comment58497

Is there any documentation explaining this calculation?



src/knewpasswordwidget.cpp (line 124)
https://git.reviewboard.kde.org/r/124963/#comment58498

Code style: join this line with the next one.



src/knewpasswordwidget.cpp (line 154)
https://git.reviewboard.kde.org/r/124963/#comment58499

Please write a short explanation of what is happening here. By what I could 
understand you are calculating the number of characters with different 
categories in the password, am I correct?



src/knewpasswordwidget.cpp (line 202)
https://git.reviewboard.kde.org/r/124963/#comment58500

Before emitting this signal check if parameter status is different from 
passwordStatus. Do not emit the signal if they are equal.



src/knewpasswordwidget.cpp (line 275)
https://git.reviewboard.kde.org/r/124963/#comment58503

If maxLength is less than minimumPasswordLength the user will not be able 
to type a valid password. Should not you check for that here and use 
minimumPasswordLength instead of maxLength in that particular case?


- Lamarque Souza


On Aug. 28, 2015, 9:24 a.m., Elvis Angelaccio wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124963/
 ---
 
 (Updated Aug. 28, 2015, 9:24 a.m.)
 
 
 Review request for KDE Frameworks and Christoph Feck.
 
 
 Repository: kwidgetsaddons
 
 
 Description
 ---
 
 This widget is a stripped-down version of KNewPasswordDialog, without any 
 dialog-specific stuff.
 
 ### Why a new widget?
 
 This widget is meant to be easily embedded in any custom password dialog, 
 including KNewPasswordDialog. It's the least common denominator of features 
 that any password dialog should offer to the user.
 
 ### Features
 
 * Password visibility action (same as RR 124698). The password verification 
 field is hidden if the user shows the password.
 * Background warning colour. The password verification field gets a coloured 
 background whenever the password and its verification don't match. (feature 
 borrowed from Keepass)
 * Password status signal. This allows the upper level dialogs to update their 
 stuff (enable/disable OK button, show warnings for low password strength, 
 etc.)
 * Password strength bar can be hidden.
 * Unit test.
 
 ### Use cases
 At least the following:
 
 * Ark (new archive dialog)
 * KNewPasswordDialog refactoring (my next RR if this one gets accepted)
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt ac708ef33e3be89db85d43911f96e536c52f741d 
   autotests/knewpasswordwidgettest.h PRE-CREATION 
   autotests/knewpasswordwidgettest.cpp PRE-CREATION 
   src/CMakeLists.txt e03e9bbd6d73811873b0a465f86da269f4295138 
   src/knewpasswordwidget.h PRE-CREATION 
   src/knewpasswordwidget.cpp PRE-CREATION 
   src/knewpasswordwidget.ui PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124963/diff/
 
 
 Testing
 ---
 
 
 File Attachments
 
 
 knewpasswordwidget1.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/5796469c-28b7-4151-b9cb-6a327631db75__knewpasswordwidget1.png
 knewpasswordwidget2.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/b3bfc9ac-ab8e-404c-8091-b5ad9e3e054f__knewpasswordwidget2.png
 knewpasswordwidget3.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/dfaeea4e-65da-4961-b266-986a21f54ca7__knewpasswordwidget3.png
 
 
 Thanks,
 
 Elvis Angelaccio
 


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

Re: Review Request 124963: New widget: KNewPasswordWidget

[Lamarque Souza]

 On Aug. 28, 2015, 11:30 a.m., Lamarque Souza wrote:
  src/knewpasswordwidget.cpp, line 92
  https://git.reviewboard.kde.org/r/124963/diff/1/?file=399453#file399453line92
 
  Is there any documentation explaining this calculation?
 
 Elvis Angelaccio wrote:
 I don't know, sorry. The whole strength calculator code is just taken 
 from KNewPasswordDialog. I don't even know its original author (git blame 
 does not help here).

If it is not your code then you are not required to document it.


 On Aug. 28, 2015, 11:30 a.m., Lamarque Souza wrote:
  src/knewpasswordwidget.cpp, line 154
  https://git.reviewboard.kde.org/r/124963/diff/1/?file=399453#file399453line154
 
  Please write a short explanation of what is happening here. By what I 
  could understand you are calculating the number of characters with 
  different categories in the password, am I correct?
 
 Elvis Angelaccio wrote:
 As above, not my code :(
 If this is a blocker, I can look for a different and documented algorithm 
 and replace the current one.

No need to replace it. I wish it would have been documented from the begining 
:-/


- Lamarque


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


On Aug. 28, 2015, 1:56 p.m., Elvis Angelaccio wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124963/
 ---
 
 (Updated Aug. 28, 2015, 1:56 p.m.)
 
 
 Review request for KDE Frameworks and Christoph Feck.
 
 
 Repository: kwidgetsaddons
 
 
 Description
 ---
 
 This widget is a stripped-down version of KNewPasswordDialog, without any 
 dialog-specific stuff.
 
 ### Why a new widget?
 
 This widget is meant to be easily embedded in any custom password dialog, 
 including KNewPasswordDialog. It's the least common denominator of features 
 that any password dialog should offer to the user.
 
 ### Features
 
 * Password visibility action (same as RR 124698). The password verification 
 field is hidden if the user shows the password.
 * Background warning colour. The password verification field gets a coloured 
 background whenever the password and its verification don't match. (feature 
 borrowed from Keepass)
 * Password status signal. This allows the upper level dialogs to update their 
 stuff (enable/disable OK button, show warnings for low password strength, 
 etc.)
 * Password strength bar can be hidden.
 * Unit test.
 
 ### Use cases
 At least the following:
 
 * Ark (new archive dialog)
 * KNewPasswordDialog refactoring (my next RR if this one gets accepted)
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt ac708ef33e3be89db85d43911f96e536c52f741d 
   autotests/knewpasswordwidgettest.h PRE-CREATION 
   autotests/knewpasswordwidgettest.cpp PRE-CREATION 
   src/CMakeLists.txt e03e9bbd6d73811873b0a465f86da269f4295138 
   src/knewpasswordwidget.h PRE-CREATION 
   src/knewpasswordwidget.cpp PRE-CREATION 
   src/knewpasswordwidget.ui PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/124963/diff/
 
 
 Testing
 ---
 
 
 File Attachments
 
 
 knewpasswordwidget1.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/5796469c-28b7-4151-b9cb-6a327631db75__knewpasswordwidget1.png
 knewpasswordwidget2.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/b3bfc9ac-ab8e-404c-8091-b5ad9e3e054f__knewpasswordwidget2.png
 knewpasswordwidget3.png
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/08/28/dfaeea4e-65da-4961-b266-986a21f54ca7__knewpasswordwidget3.png
 
 
 Thanks,
 
 Elvis Angelaccio
 


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

Review Request 124714: Add support to qmake's options to api.kde.org

[Lamarque Souza]

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

Review request for KDE Frameworks and Alex Merry.


Repository: kapidox


Description
---

NetworkManagerQt requires some options to be added to .pro file to compile:

QT += NetworkManagerQt dbus network
CONFIG += link_pkgconfig
PKGCONFIG += libnm # from NetworkManager package
DEFINES += QT_NO_KEYWORDS # libnm declares a variable named signals

kapidox already supports the first option (QT), but not the others. This patch 
adds support to adding the other three options.


Diffs
-

  src/kapidox/data/templates/fwinfo.html a20c1d1 

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


Testing
---

I added the lines below to networkmanager-qt/metainfo.html and kapidox created 
the correct output:

qmakepro:
 - config: link_pkgconfig
 - defines: QT_NO_KEYWORDS
 - pkgconfig: libnm


Thanks,

Lamarque Souza

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

Re: [kwallet] /: Updating maintainer name

[Lamarque Souza]

Well, it does not work for multiple identity names. I have changed the
maintainer fields in MMQt and NMQt's repositories to use just grulich as
maintainer.

Lamarque V. Souza

http://planetkde.org/pt-br

On Mon, Aug 3, 2015 at 5:17 PM, David Faure fa...@kde.org wrote:

 On Monday 03 August 2015 11:16:17 Lamarque Souza wrote:
  Does the maintainer field accept more than one identity?

 That's the question. Go ahead and do that, Alex Merry volunteered
 to fix kapidox if necessary :-)

 --
 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: [kwallet] /: Updating maintainer name

[Lamarque Souza]

Does the maintainer field accept more than one identity?

Lamarque V. Souza

http://planetkde.org/pt-br

On Mon, Aug 3, 2015 at 7:11 AM, Alex Merry alex.me...@kde.org wrote:

 On Sunday 02 August 2015 19:31:36 David Faure wrote:
  Are you sure about this? All other frameworks use the identity login name
  there. This makes it possible (e.g. for api.kde.org) to determine full
 name
  and email.
 
  I just noticed that modemmanager-qt and networkmanager-qt do it wrong
   - cc'ing the maintainers.
 
  Compare
 
 http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/index.html
  where it's done right (identity login)
 
  and
 
 http://api.kde.org/frameworks-api/frameworks5-apidocs/modemmanager-qt/html/i
  ndex.html where it's not (falls back to The KDE Community when the
  maintainer name is empty or unparseable).
 
  This being said, I don't know if kapidox supports multiple names in that
  field, but it's worth a try.

 Well, if it doesn't we can easily fix that.

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

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

Re: Review Request 124413: Enable PAM opening KWallet again

[Lamarque Souza]

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



src/runtime/kwalletd/main.cpp (line 113)
https://git.reviewboard.kde.org/r/124413/#comment57076

You should use strncmp instead of strcmp.



src/runtime/kwalletd/main.cpp (line 126)
https://git.reviewboard.kde.org/r/124413/#comment57078

You should use strtol() instead of atoi() for better error checking. atoi() 
does no error checking at all.



src/runtime/kwalletd/main.cpp (line 129)
https://git.reviewboard.kde.org/r/124413/#comment57077

environment



src/runtime/kwalletd/main.cpp (line 144)
https://git.reviewboard.kde.org/r/124413/#comment57079

You should deallocate hash here if it is not null, otherwise you have a 
memory leak.


- Lamarque Souza


On July 21, 2015, 3:52 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124413/
 ---
 
 (Updated July 21, 2015, 3:52 p.m.)
 
 
 Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
 
 
 Repository: kwallet
 
 
 Description
 ---
 
 This brings back Alex's patch in commit 
 f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
 opened by PAM if kwallet-pam is present.
 
 http://quickgit.kde.org/?p=kde-runtime.gita=commith=f2fe3e75b4ba12d0f99aa09327059a1865891b14
 
 
 Diffs
 -
 
   src/runtime/kwalletd/main.cpp b4e3837 
 
 Diff: https://git.reviewboard.kde.org/r/124413/diff/
 
 
 Testing
 ---
 
 Logged in, KWallet does not ask for password anymore.
 
 
 Thanks,
 
 Martin Klapetek
 


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

Re: Review Request 124413: Enable PAM opening KWallet again

[Lamarque Souza]

 On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
  src/runtime/kwalletd/main.cpp, line 113
  https://git.reviewboard.kde.org/r/124413/diff/1/?file=386596#file386596line113
 
  You should use strncmp instead of strcmp.
 
 Martin Klapetek wrote:
 Why would you think? The whole string is being compared, what good would 
 strncmp do in here?

I was thinking about using something like

if (strncmp(argv[x], --pam-login, sizeof(--pam-login)))

It's a general rule not use use strcmp in security sensitive code since it only 
stops to compare characters when it finds a null character. If no such 
character exists in the compared string then you will have a buffer overflow. 
Since this is an argv string it probably contains a null byte, so the should 
and not have to. It is just recomendation, you can drop it if you wish.


 On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
  src/runtime/kwalletd/main.cpp, line 126
  https://git.reviewboard.kde.org/r/124413/diff/1/?file=386596#file386596line126
 
  You should use strtol() instead of atoi() for better error checking. 
  atoi() does no error checking at all.
 
 Martin Klapetek wrote:
 ...but the code does not check for errors (and does not need to)?

Actually, the error checking is done in line 135, so you can drop this one.


- Lamarque


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


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124413/
 ---
 
 (Updated July 21, 2015, 5:27 p.m.)
 
 
 Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
 
 
 Repository: kwallet
 
 
 Description
 ---
 
 This brings back Alex's patch in commit 
 f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
 opened by PAM if kwallet-pam is present.
 
 http://quickgit.kde.org/?p=kde-runtime.gita=commith=f2fe3e75b4ba12d0f99aa09327059a1865891b14
 
 
 Diffs
 -
 
   src/runtime/kwalletd/main.cpp b4e3837 
 
 Diff: https://git.reviewboard.kde.org/r/124413/diff/
 
 
 Testing
 ---
 
 Logged in, KWallet does not ask for password anymore.
 
 
 Thanks,
 
 Martin Klapetek
 


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

Re: Review Request 124413: Enable PAM opening KWallet again

[Lamarque Souza]

 On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
  src/runtime/kwalletd/main.cpp, line 113
  https://git.reviewboard.kde.org/r/124413/diff/1/?file=386596#file386596line113
 
  You should use strncmp instead of strcmp.
 
 Martin Klapetek wrote:
 Why would you think? The whole string is being compared, what good would 
 strncmp do in here?
 
 Lamarque Souza wrote:
 I was thinking about using something like
 
 if (strncmp(argv[x], --pam-login, sizeof(--pam-login)))
 
 It's a general rule not use use strcmp in security sensitive code since 
 it only stops to compare characters when it finds a null character. If no 
 such character exists in the compared string then you will have a buffer 
 overflow. Since this is an argv string it probably contains a null byte, so 
 the should and not have to. It is just recomendation, you can drop it if 
 you wish.
 
 Stefan Brüns wrote:
 But --pam-login is null terminated, so you will compare at most 
 sizeof(--pam-login) bytes anyway.

Yeah, you're right.


- Lamarque


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


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124413/
 ---
 
 (Updated July 21, 2015, 5:27 p.m.)
 
 
 Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
 
 
 Repository: kwallet
 
 
 Description
 ---
 
 This brings back Alex's patch in commit 
 f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
 opened by PAM if kwallet-pam is present.
 
 http://quickgit.kde.org/?p=kde-runtime.gita=commith=f2fe3e75b4ba12d0f99aa09327059a1865891b14
 
 
 Diffs
 -
 
   src/runtime/kwalletd/main.cpp b4e3837 
 
 Diff: https://git.reviewboard.kde.org/r/124413/diff/
 
 
 Testing
 ---
 
 Logged in, KWallet does not ask for password anymore.
 
 
 Thanks,
 
 Martin Klapetek
 


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

Re: Review Request 124133: Add dedicated Version tab to KAboutApplicationDialog

[Lamarque Souza]

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



src/kaboutapplicationdialog.cpp (line 155)
https://git.reviewboard.kde.org/r/124133/#comment56243

Please call QLabel 
pointer-setTextInteractionFlags(Qt::TextSelectableByMouse) before calling 
versionLayout-addWidget(QLabel pointer).


- Lamarque Souza


On June 20, 2015, 4:30 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124133/
 ---
 
 (Updated June 20, 2015, 4:30 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kxmlgui
 
 
 Description
 ---
 
 This replaces the version information of the version and frameworks and
 moves it into a dedicated tab. In this tab the information is extended
 by the Qt version (which is equally relevant as e.g. the frameworks
 version) in both runtime and compile time.
 
 Also windowing system is added. This will become a useful information
 for KWin developers starting in Plasma 5.4 when users start to test
 things out and we need to know whether the window they experience the
 problem with is running on wayland or xwayland.
 
 
 Diffs
 -
 
   src/kaboutapplicationdialog.cpp 5eeea7711aa4f95a9cd4191d68ad330ef795caea 
 
 Diff: https://git.reviewboard.kde.org/r/124133/diff/
 
 
 Testing
 ---
 
 
 File Attachments
 
 
 New wayland, new X11, old X11
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/06/20/b76ba6ae-b01c-4c6a-9248-29d39c652d83__snapshot_J11265.png
 
 
 Thanks,
 
 Martin Gräßlin
 


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

Re: Review Request 124133: Add dedicated Version tab to KAboutApplicationDialog

[Lamarque Souza]

 On June 20, 2015, 7:10 p.m., Martin Klapetek wrote:
  +1 for adding all that info
  -1 for putting it into its own tab; previously it was visible right away, 
  now an additional click is needed. Maybe it could be stacked all under the 
  application name? I mean it's just 2 more lines to add there...
 
 Sebastian Kügler wrote:
 Putting more and more versions there doesn't scale. Just cramming more 
 and more stuff in there doesn't really make it more beautiful, putting it 
 into its own tab means that the information is neatly organized.
 
 Also, the version is really an implementation detail, and having one 
 version there will often yield another round of asking (Which versions are 
 you running? - version x.y.z - No, I need the versions that are shown in 
 the tab, so also frameworks, Qt, and whether it runs on wayland or X11.
 
 Martin Klapetek wrote:
 Honestly the only time I've seen/told anyone open that dialog was to get 
 the app version number. This was previously visible right away, now it's 
 hidden in an additional click. Could perhaps be at least made the 
 first/default tab?
 
 and having one version there will often yield another round of asking
 
 I'm not sure I understand what you're getting at, but given there was 
 only one version string in that dialog for longlonglong time, I've never hit 
 the scenairo you're describing. My idea was to do this:
 
 KWrite
 ==
 Version 5.0.0
 Using KDE Frameworks 5.12.0
 Qt 5.4.2 (built against 5.4.2); xcb/wayland
 
 ...ie. just add one more line below the current data set in the main 
 layout and have no version tab at all; all the data visible right away after 
 opening the dialog. It doesn't need to scale as there are no more version 
 numbers to put there (unless we want to put everything in there).
 
 Martin Gräßlin wrote:
 one of the thoughts was also to give applications a possibility to easily 
 add more information about it. E.g. in KWin the supportInformation (we don't 
 have an about dialog) there are about 10 individual version/compile 
 information put out. So while you didn't have the need yet, I experienced 
 this need in the past a lot.
 
 Concerning first tab: not sure. Is this the most important information? 
 And even if: do users count the clicks? I doubt it.
 
 Alexander Potashev wrote:
 If there will be 10 versions in the main tab, it is still not a problem 
 because we can make the view scrollable.
 
 Martin Klapetek wrote:
 Is this the most important information?
 
 I'd like to think it is. You rarely need any of the other information - 
 whenever a user reports a bug without a version number, first thing he's 
 asked is what is the version you're running?. Users may look at authors, 
 but surely not repeatedly or at least not the same amount as looking for 
 version string. In my humble opinion, for us developers and bug triagers, the 
 version number is the most important information in that dialog and as such 
 should be as quickly and easily accessible as possible.
 
 This patch moves away this vital piece of information from first glance 
 into a separate non-default tab. That does not seem like an improvement to 
 the situation.
 
 I bet that if you'd ask any random KDE software user what is in that 
 dialog, they would most probably not be sure beyond the version number (which 
 should hint at what is actually most important/used information).
 
 Martin Gräßlin wrote:
 I hope that more application maintainers provide their thoughts on it. 
 After checking quite some applications I can only conclude that version 
 numbers in the KDE Applications are not maintained, incorrect and wrong. 
 Given that I don't think we do our applications a favor to expose the version 
 number that prominent in a first tab.

This is a +1 from me. I agree that this change makes about dialog cleaner. I do 
not appose to it although every time I have asked an user to open an about 
dialog I did it so that the user could send me the version string. Adding one 
more click does not help here but also does not prevent the user from reaching 
it. What our usability experts have to say about this change?


- Lamarque


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


On June 20, 2015, 4:30 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124133/
 ---
 
 (Updated June 20, 2015, 4:30 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kxmlgui
 
 
 Description
 ---
 
 This replaces the version information of the version and frameworks and
 moves it 

Re: Review Request 123533: Add public dependency on NetworkManager

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On April 27, 2015, 2:42 p.m., Jan Grulich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123533/
 ---
 
 (Updated April 27, 2015, 2:42 p.m.)
 
 
 Review request for KDE Frameworks and Lamarque Souza.
 
 
 Repository: networkmanager-qt
 
 
 Description
 ---
 
 This patch adds public dependency on NetworkManager and also uses 
 FindNetworkManager.cmake from extra-cmake-modules instead own copy.
 
 
 Diffs
 -
 
   CMakeLists.txt c9e3274 
   KF5NetworkManagerQtConfig.cmake.in cdabe8e 
   autotests/CMakeLists.txt 8d22be3 
   cmake/FindNetworkManager.cmake ece8835 
   src/CMakeLists.txt e2be7d8 
   src/fakenetwork/CMakeLists.txt 97c2867 
 
 Diff: https://git.reviewboard.kde.org/r/123533/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jan Grulich
 


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

Re: Review Request 123534: Add public dependency on ModemManager

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On April 27, 2015, 2:44 p.m., Jan Grulich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123534/
 ---
 
 (Updated April 27, 2015, 2:44 p.m.)
 
 
 Review request for KDE Frameworks and Lamarque Souza.
 
 
 Repository: modemmanager-qt
 
 
 Description
 ---
 
 This patch adds public dependency on ModemManager and also uses 
 FindModemManager.cmake from extra-cmake-modules instead own copy.
 
 
 Diffs
 -
 
   CMakeLists.txt 2af6f56 
   KF5ModemManagerQtConfig.cmake.in d60610a 
   autotests/CMakeLists.txt 03e1939 
   cmake/FindModemManager.cmake d522e93 
   src/CMakeLists.txt 29a7a63 
   src/fakemodem/CMakeLists.txt b6d4a90 
 
 Diff: https://git.reviewboard.kde.org/r/123534/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jan Grulich
 


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

Re: Review Request 123532: Add find modules for ModemManager and NetworkManager

[Lamarque Souza]

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


It worked here, so it's a +1 from me. I just would like the extra-cmake-module 
maintainer to give the final ship it.

- Lamarque Souza


On April 27, 2015, 2:38 p.m., Jan Grulich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123532/
 ---
 
 (Updated April 27, 2015, 2:38 p.m.)
 
 
 Review request for KDE Frameworks and Lamarque Souza.
 
 
 Repository: extra-cmake-modules
 
 
 Description
 ---
 
 This patch adds FindModemManager.cmake and FindNetworkManager.cmake into 
 other find modules. These are required for modemmanager-qt and 
 networkmanager-qt, which are currently missing public dependencies on 
 ModemManager/NetworkManager and to be able to add them, we need to have these 
 find modules publicly available. Note, that my knowledge of CMake is quite 
 limited, so proper review would be needed :).
 
 
 Diffs
 -
 
   find-modules/FindModemManager.cmake PRE-CREATION 
   find-modules/FindNetworkManager.cmake PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/123532/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jan Grulich
 


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

Re: Review Request 123477: Add public dependency on NetworkManager

[Lamarque Souza]

On April 23, 2015, 1:35 p.m., Jan Grulich wrote:
  And yes, this is required as long as NetworkManager headers are included by 
  NetworkManagerQt headers.
 
 Jan Grulich wrote:
 I just found out that installing FindNetworkManager.cmake into CMake 
 modules is not enough, because it still cannot find it. I guess this would 
 have to be done according to other FindFoo.cmake modules from 
 extra-cmake-modules, right?
 
 Aleix Pol Gonzalez wrote:
 That would certainly help, but then it's not very nice putting a cmake 
 file into another project's directory.
 I would suggest to either:
 
 * Contribute FindNetworkManager.cmake to extra-cmake-modules
 * Install it within the NetworkManagerQt cmake directory, together with 
 NetworkManagerQtConfig.cmake.
 
 Jan Grulich wrote:
 I actually didn't mean that I should install it to ../ECM/modules, but 
 whether using find_package_handle_standard_args() and other macros, which are 
 used in all FindFoo.cmake modules from e-c-m, are required to make this work.
 
 Aleix Pol Gonzalez wrote:
 Not required per se, but recommended. There's many things to be defined 
 on a proper cmake finder.
 OTOH, this finder seems to be working already.

I would go for moving FindNetworkManager.cmake to e-c-m.


- Lamarque


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


On April 23, 2015, 1:01 p.m., Jan Grulich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123477/
 ---
 
 (Updated April 23, 2015, 1:01 p.m.)
 
 
 Review request for KDE Frameworks, David Faure, Daniel Vrátil, and Lamarque 
 Souza.
 
 
 Repository: networkmanager-qt
 
 
 Description
 ---
 
 I'm not 100% sure how this should work and I couldn't find another framework 
 doing the same, but Daniel Vrátil pointed me out that NetworkManagerQt should 
 mention in KF5NetworkManagerQtConfig.cmake file that it requires 
 NetworkManager as dependency. Given this, we also need to install 
 FindNetworkManager.cmake into CMake modules so NetworkManager can be found by 
 find_dependency() macro, or FindNetworkManager.cmake can go into 
 extra-cmake-modules.
 
 If this is how it should be done, then similar patch would be needed for 
 ModemManagerQt.
 
 
 Diffs
 -
 
   CMakeLists.txt c9e3274 
   KF5NetworkManagerQtConfig.cmake.in cdabe8e 
 
 Diff: https://git.reviewboard.kde.org/r/123477/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jan Grulich
 


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

Re: Review Request 123401: Fix usage of the networkmanagerqt_export.h after commit 9966897

[Lamarque Souza]

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

Ship it!


The patch looks good to me. Network firewall here blocks 'git pull' (among 
serveral other services) so I could not test it myself. Plasma NM can be fixed 
later.

- Lamarque Souza


On April 17, 2015, 3:53 p.m., Hrvoje Senjan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123401/
 ---
 
 (Updated April 17, 2015, 3:53 p.m.)
 
 
 Review request for KDE Frameworks, David Faure and Jan Grulich.
 
 
 Repository: networkmanager-qt
 
 
 Description
 ---
 
 The headers include networkmanagerqt_export.h, which is now in lowercase 
 directory, and is not in public INTERFACE includes. We need to change all the 
 includes, and place where the export is generated.
 
 
 Diffs
 -
 
   src/CMakeLists.txt 0dc3f2e 
   src/accesspoint.h 99895ef 
   src/activeconnection.h 6edd0ae 
   src/adsldevice.h d7c92ba 
   src/bluetoothdevice.h 4baf4a3 
   src/bonddevice.h 82eafa2 
   src/bridgedevice.h 68b3e00 
   src/connection.h a20d1f7 
   src/device.h bca098b 
   src/dhcp4config.h 35082ba 
   src/dhcp6config.h 91ca914 
   src/genericdevice.h d95fab0 
   src/generictypes.h cd45430 
   src/gredevice.h eeb682c 
   src/infinibanddevice.h c9597cb 
   src/ipaddress.h ebc3e5f 
   src/ipconfig.h 5eb0227 
   src/iproute.h 13afe15 
   src/macvlandevice.h ab02831 
   src/manager.h e79b933 
   src/modemdevice.h e40d457 
   src/olpcmeshdevice.h 9af2342 
   src/secretagent.h 02e4c9a 
   src/settings.h f5fe486 
   src/settings/adslsetting.h 89ab5b5 
   src/settings/bluetoothsetting.h 9a32851 
   src/settings/bondsetting.h f3ae1ec 
   src/settings/bridgeportsetting.h 7a8f74a 
   src/settings/bridgesetting.h 4628cf5 
   src/settings/cdmasetting.h ecd30da 
   src/settings/connectionsettings.h 252f8e9 
   src/settings/genericsetting.h 610568a 
   src/settings/gsmsetting.h 1a152b2 
   src/settings/infinibandsetting.h 713e01e 
   src/settings/ipv4setting.h bdfcc58 
   src/settings/ipv6setting.h 7e7ca9e 
   src/settings/olpcmeshsetting.h 217682d 
   src/settings/pppoesetting.h a3c5d79 
   src/settings/pppsetting.h 4755288 
   src/settings/security8021xsetting.h 486f1bc 
   src/settings/serialsetting.h fa0bf57 
   src/settings/setting.h 86c6e83 
   src/settings/teamsetting.h ba58a5d 
   src/settings/template.h 4f9f680 
   src/settings/vlansetting.h 8432eab 
   src/settings/vpnsetting.h 67aea4e 
   src/settings/wimaxsetting.h 9fd887a 
   src/settings/wiredsetting.h fec6860 
   src/settings/wirelesssecuritysetting.h 5c9a1da 
   src/settings/wirelesssetting.h 12eed72 
   src/teamdevice.h e42bc81 
   src/tundevice.h dd66977 
   src/utils.h d091c5e 
   src/vethdevice.h c9322d0 
   src/vlandevice.h bfe2948 
   src/vpnconnection.h acd0d2e 
   src/vpnplugin.h 3a4f181 
   src/wimaxdevice.h fb202fa 
   src/wimaxnsp.h c9bd856 
   src/wireddevice.h 193710c 
   src/wirelessdevice.h 6137036 
   src/wirelessnetwork.h 1d5cb7b 
 
 Diff: https://git.reviewboard.kde.org/r/123401/diff/
 
 
 Testing
 ---
 
 Plasma-nm no longer fails due to original problem, but incorect usage of 
 nm-qt includes:
 
 vpnuiplugin.h:31:43: fatal error: NetworkManagerQt/generictypes.h: No such 
 file or directory
 
 
 Thanks,
 
 Hrvoje Senjan
 


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

Re: Review Request 123360: Introduce LoopSound flag

[Lamarque Souza]

 On April 14, 2015, 3:50 p.m., Lamarque Souza wrote:
  src/notifybyaudio.cpp, line 89
  https://git.reviewboard.kde.org/r/123360/diff/1/?file=361076#file361076line89
 
  Should not this patch be followed by another patch that set LoopSound 
  flag for Persistant notifications that *really* want sound to loop? This 
  change can be considered a regression if such a persistant notification 
  exists.
 
 Martin Klapetek wrote:
 I'm not aware of any such notification (also note that this code was 
 written from scratch for frameworks only). The only place I know I will use 
 it is ktp-call-ui but I really don't see why should that patch be posted here 
 as a follow-up...

That is fair, no more issues from my side then.


- Lamarque


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


On April 14, 2015, 2:41 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123360/
 ---
 
 (Updated April 14, 2015, 2:41 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 346148
 https://bugs.kde.org/show_bug.cgi?id=346148
 
 
 Repository: knotifications
 
 
 Description
 ---
 
 Some notifications might want to loop the sound, eg. calling apps playing the 
 ringing sound which is not wanted to play once. Currently the sound is looped 
 for all persistent notifications, but that's not ideal as not all persistent 
 notifications with sound want/need sound looping. This new LoopSound flag 
 solves that.
 
 
 Diffs
 -
 
   src/knotification.h 8efd92a 
   src/notifybyaudio.cpp ee3ef1e 
 
 Diff: https://git.reviewboard.kde.org/r/123360/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Klapetek
 


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

Re: Review Request 123360: Introduce LoopSound flag

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On April 14, 2015, 2:41 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123360/
 ---
 
 (Updated April 14, 2015, 2:41 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 346148
 https://bugs.kde.org/show_bug.cgi?id=346148
 
 
 Repository: knotifications
 
 
 Description
 ---
 
 Some notifications might want to loop the sound, eg. calling apps playing the 
 ringing sound which is not wanted to play once. Currently the sound is looped 
 for all persistent notifications, but that's not ideal as not all persistent 
 notifications with sound want/need sound looping. This new LoopSound flag 
 solves that.
 
 
 Diffs
 -
 
   src/knotification.h 8efd92a 
   src/notifybyaudio.cpp ee3ef1e 
 
 Diff: https://git.reviewboard.kde.org/r/123360/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Klapetek
 


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

Re: Review Request 123349: NetworkManagerQt: comment out unneeded lines from example/CMakeLists.txt

[Lamarque Souza]

 On April 13, 2015, 7:13 a.m., David Faure wrote:
  examples/CMakeLists.txt, line 5
  https://git.reviewboard.kde.org/r/123349/diff/1/?file=361005#file361005line5
 
  well i thought maybe someone wants to see how to use this in their 
  app... but ok IIRC it also shows that on api.kde.org
 
 Jan Grulich wrote:
 +1 for removing, it should be same for all frameworks, so there is no 
 need to mention how to find the library.

In quick search I could not find those information on api.kde.org. You have got 
a point that keeping those lines in CMakeLists.txt makes things easier for 
beginners. Let's keep them commented. This patch has +1 after changing 
examples/createconnection/CMakeLists.txt.


- Lamarque


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


On April 12, 2015, 7:57 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123349/
 ---
 
 (Updated April 12, 2015, 7:57 p.m.)
 
 
 Review request for KDE Frameworks and Jan Grulich.
 
 
 Repository: networkmanager-qt
 
 
 Description
 ---
 
 This is built as part of the framework, so it should not do
 find_package on itself, it won't work the first time.
 
 I assume this was there in order to all build separately, so I left
 it commented out, but it's wrong to have that in.
 
 Maybe with the last two commits, the examples can be built by default,
 like in the other frameworks, so that we ensure that they always do build?
 
 
 Diffs
 -
 
   examples/CMakeLists.txt c2484b346b70596ddeb87778c7dd3881851441fb 
 
 Diff: https://git.reviewboard.kde.org/r/123349/diff/
 
 
 Testing
 ---
 
 rebuilt the examples from scratch, worked
 
 
 Thanks,
 
 David Faure
 


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

Re: Review Request 123349: NetworkManagerQt: comment out unneeded lines from example/CMakeLists.txt

[Lamarque Souza]

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



examples/CMakeLists.txt (line 5)
https://git.reviewboard.kde.org/r/123349/#comment53935

You are right, it was there to allow to enable examples separately. You can 
remove those lines instead of commenting them.

You need to do this same change in the other example 
(examples/createconnection/CMakeLists.txt) or it will fail to build.


- Lamarque Souza


On April 12, 2015, 7:57 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123349/
 ---
 
 (Updated April 12, 2015, 7:57 p.m.)
 
 
 Review request for KDE Frameworks and Jan Grulich.
 
 
 Repository: networkmanager-qt
 
 
 Description
 ---
 
 This is built as part of the framework, so it should not do
 find_package on itself, it won't work the first time.
 
 I assume this was there in order to all build separately, so I left
 it commented out, but it's wrong to have that in.
 
 Maybe with the last two commits, the examples can be built by default,
 like in the other frameworks, so that we ensure that they always do build?
 
 
 Diffs
 -
 
   examples/CMakeLists.txt c2484b346b70596ddeb87778c7dd3881851441fb 
 
 Diff: https://git.reviewboard.kde.org/r/123349/diff/
 
 
 Testing
 ---
 
 rebuilt the examples from scratch, worked
 
 
 Thanks,
 
 David Faure
 


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

Re: [Kde-hardware-devel] solid bugzilla product marked obsolete

[Lamarque Souza]

Hi all,

Following the obsolete action, could someone mark Network Management
product as obsolete too? It is related to old Plasma NM 0.9.0.x series,
which is now unmaintained.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br

On Wed, Mar 11, 2015 at 7:57 PM, Albert Astals Cid aa...@kde.org wrote:

 El Dimecres, 11 de març de 2015, a les 20:24:00, Jonathan Riddell va
 escriure:
  I've labelled the solid bugzilla product as obsolete and added a comment
 to
  any bugs that it's for kdelibs4 code which is now in maintenance mode.
  Replacement products are bluedevil, powerdevil, plasma-nm and
  frameworks-solid.

 You forgot to email kde-hardware-de...@kde.org

 
  Jonathan

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

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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Hi,

Sure I think they are, I am asking all those questions about dependencies
and implications of adding the libraries to KF5 because of that too. I just
want to make sure the other developers still think the same. I did not make
that decision alone back in 2011 in Madrid.

I think they are useful for embedded programs that use NetworkManager to
connect. Sharing MMQt between Plasma NM and KDE Telepathy is also a good
example. Using dbus-send for creating NetworkManager connections used to be
very troublesome because of the settings parsing code required to
send/retrieve the connection settings to/from NetworkManager. Creating a
small program using NMQt is simpler since we already implement that parsing
code. There is even an example in libnm-qt/examples/createconnection about
that.

Ok, so let's move on. NMQt/MMQt are going to be part of KF5, I will try to
compile the framework branch next weekend and see what is still needed to
be done to make that happen.

Thanks to all for answering my questions.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Wed, Apr 9, 2014 at 2:20 AM, Kevin Ottens er...@kde.org wrote:

 Hello,

 On Tuesday 08 April 2014 19:51:05 Lamarque Souza wrote:
   I understood that, I just do not know all the other things we need to do
  to make NMQt/MMQt part of KF5. And yes, I agree in making NMQt/MMQt part
 of
  KF5. The other doubt I still have is  where _kde_add_platform_definitions
  is defined. By what I could figure out it is not in ECM, so something
 else
  is needed to parse the CMakeLists.txt file in current NMQt's framework
  branch.

 _kde_add_platform_definitions is available in ECM. So from your
 perspective it
 means ECM is the only extra dependency as stated several times.

  As for my questioning about merging NMQt/MMQt into plasma-nm repo is
  because every time I argue that we should make them usable for non-KDE
  users arguments like this appears: I would be really interested how many
  distributions would have it [NMQt] without Plasma NM.. Now I am
 wondering
  if I am the only one that still thinks NMQt/MMQt are useful for other
  software besides Plasma NM.

 Honestly, if you really think NMQt/MMQt are to be useful outside of
 plasma-nm,
 then KDE Frameworks is likely a good place for them, if not you'd better
 merge
 them in plasma-nm.

 Personally I doubt they're useful outside of plasma-nm though. But that's
 your
 job as maintainer to pick a choice.

 Regards.
 --
 Kévin Ottens, http://ervin.ipsquad.net

 KDAB - proud supporter of KDE, http://www.kdab.com


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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Ok, so let's go make a better NMQt/MMQt :-)

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Wed, Apr 9, 2014 at 3:44 AM, Jan Grulich jgrul...@redhat.com wrote:

 Hi,

 On Wednesday 09 of April 2014 07:20 Kevin Ottens wrote:
  Hello,
 
  On Tuesday 08 April 2014 19:51:05 Lamarque Souza wrote:
I understood that, I just do not know all the other things we need to
 do
  
   to make NMQt/MMQt part of KF5. And yes, I agree in making NMQt/MMQt
 part
   of
   KF5. The other doubt I still have is  where
 _kde_add_platform_definitions
   is defined. By what I could figure out it is not in ECM, so something
 else
   is needed to parse the CMakeLists.txt file in current NMQt's framework
   branch.
 
  _kde_add_platform_definitions is available in ECM. So from your
 perspective
  it means ECM is the only extra dependency as stated several times.
 
   As for my questioning about merging NMQt/MMQt into plasma-nm repo is
   because every time I argue that we should make them usable for non-KDE
   users arguments like this appears: I would be really interested how
 many
   distributions would have it [NMQt] without Plasma NM..

 I meant that nobody would not know about them, but I didn't mean that they
 are
 not useful.

   Now I am wondering
   if I am the only one that still thinks NMQt/MMQt are useful for other
   software besides Plasma NM.
 
  Honestly, if you really think NMQt/MMQt are to be useful outside of
  plasma-nm, then KDE Frameworks is likely a good place for them, if not
  you'd better merge them in plasma-nm.
 
  Personally I doubt they're useful outside of plasma-nm though. But that's
  your job as maintainer to pick a choice.
 

 I'm convinced that they could be useful for others, i.e ModemManagerQt
 could
 be used by KTp for sending sms (this was even implemented with the old
 version
 of MMQT).

 Cheers,
 Jan

 --
 Jan Grulich
 Red Hat Czech, s.r.o
 jgrul...@redhat.com

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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Hi,

 I understood that, I just do not know all the other things we need to do
to make NMQt/MMQt part of KF5. And yes, I agree in making NMQt/MMQt part of
KF5. The other doubt I still have is  where _kde_add_platform_definitions
is defined. By what I could figure out it is not in ECM, so something else
is needed to parse the CMakeLists.txt file in current NMQt's framework
branch.

As for my questioning about merging NMQt/MMQt into plasma-nm repo is
because every time I argue that we should make them usable for non-KDE
users arguments like this appears: I would be really interested how many
distributions would have it [NMQt] without Plasma NM.. Now I am wondering
if I am the only one that still thinks NMQt/MMQt are useful for other
software besides Plasma NM.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Mon, Apr 7, 2014 at 10:11 AM, Mario Fux kde...@unormal.org wrote:

 Am Montag, 07. April 2014, 14.38:14 schrieb Lamarque Souza:
  Hi all,

 Morning Lamarque

  I have cloned ECM git repo and looked at it. I agree that it is small and
  it has useful features for NMQt/MMQt. I like the fact that it provides a
  FindNetworkManager.cmake. Ok, we can make ECM a hard dependency for
  NMQt/MMQt.
 
  My only concern now is the kde-modules that Jan used in NMQt/MMQt's
  framework branches. Is it necessary to have KF5 installed to use those

 Here still seems to be a misunderstanding about KDE Frameworks (KF5). The
 goal
 and idea was and is it to modularize kdelibs so that people can pick the
 frameworks (e.g. KConfig and Sonnet) they need for an app and don't need to
 use and install all of them. So for the case of our (KDE) apps a lot (or
 most)
 of them will depend on many or all of the Frameworks in KF5. But all other
 Qt
 apps and libs that want to use the KDE Frameworks can just pick the once
 they
 need.

 Hope that clarifies KF5 a bit
 Mario

  modules? I am asking it because I did not find where in ECM repo the
  macro _kde_add_platform_definitions is defined.
 
  Lamarque V. Souza
 
  KDE's Network Management maintainer
 
  http://planetkde.org/pt-br
 
  On Mon, Apr 7, 2014 at 5:31 AM, Martin Gräßlin mgraess...@kde.org
 wrote:
   On Monday 07 April 2014 10:14:12 David Faure wrote:
On Monday 07 April 2014 09:47:43 Jan Grulich wrote:
 You are still talking about users, but I'm sure that 99% of them
 will
 install it from distro repositories and because e-c-m is build
  
   dependency,
  
 they won't notice that. For remaining 1% of users you are talking
 about will be e-c-m available from distro repositories as well, so
 what's the problem? Now those libraries are compiled mostly because
 of Plasma NM, which requires unreleased versions (i.e. for
 frameworks version) and in this case they have already e-c- m
 installed.

 I don't want to have libnm-qt/libmm-qt as separated libraries, I
 think that
 a lot of distributions have them in their repositories, because
 they
  
   are
  
 as
 dependency for Plasma NM. I would be really interested how many
 distributions would have it without Plasma NM. It makes sense to me
 be part
 of KF5, don't be separated and be more visible and available for
 other developers and users and probably less confusing for
 packagers.
   
I agree. ECM is a very tiny dependency to have, and in return it
 solves
a large number of issues for you (deployment, cmake config files,
qmake
  
   .pri
  
file, dependency handling for users of your libs, forwarding headers,
versioning, releasing, etc. etc.).
  
   This is IMHO the important point. Just look at what ECM provides and
 you
   will
   realize that it's extremely useful and that even if those libraries
 will
   not
   end up in KF5, they will use ECM. How can I do such a bold statement?
   Because
   ECM solves real world problems you want to have solved in your library,
   too.
   The developers using your library will expect a proper Config.cmake
 file
   and a
   proper Version.cmake file and ECM solves that for you. I'm using ECM in
   libraries not intended to ever be part of KF5 exactly for that and
 before
   I added that dependency it was just not possible to reliable use the
   library.
  
   Of course you can also write that CMake Foo all by yourself but I have
   the feeling that the knowledge to write proper CMake Foo is not wide
   spread among
   our developers. I personally prefer to use the CMake Foo written by
   people who
   know their stuff instead of doing my own.
  
   Cheers
   Martin


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

Re: Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Hi all,

I have cloned ECM git repo and looked at it. I agree that it is small and
it has useful features for NMQt/MMQt. I like the fact that it provides a
FindNetworkManager.cmake. Ok, we can make ECM a hard dependency for
NMQt/MMQt.

My only concern now is the kde-modules that Jan used in NMQt/MMQt's
framework branches. Is it necessary to have KF5 installed to use those
modules? I am asking it because I did not find where in ECM repo the
macro _kde_add_platform_definitions is defined.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Mon, Apr 7, 2014 at 5:31 AM, Martin Gräßlin mgraess...@kde.org wrote:

 On Monday 07 April 2014 10:14:12 David Faure wrote:
  On Monday 07 April 2014 09:47:43 Jan Grulich wrote:
   You are still talking about users, but I'm sure that 99% of them will
   install it from distro repositories and because e-c-m is build
 dependency,
   they won't notice that. For remaining 1% of users you are talking about
   will be e-c-m available from distro repositories as well, so what's the
   problem? Now those libraries are compiled mostly because of Plasma NM,
   which requires unreleased versions (i.e. for frameworks version) and in
   this case they have already e-c- m installed.
  
   I don't want to have libnm-qt/libmm-qt as separated libraries, I think
   that
   a lot of distributions have them in their repositories, because they
 are
   as
   dependency for Plasma NM. I would be really interested how many
   distributions would have it without Plasma NM. It makes sense to me be
   part
   of KF5, don't be separated and be more visible and available for other
   developers and users and probably less confusing for packagers.
 
  I agree. ECM is a very tiny dependency to have, and in return it solves a
  large number of issues for you (deployment, cmake config files, qmake
 .pri
  file, dependency handling for users of your libs, forwarding headers,
  versioning, releasing, etc. etc.).

 This is IMHO the important point. Just look at what ECM provides and you
 will
 realize that it's extremely useful and that even if those libraries will
 not
 end up in KF5, they will use ECM. How can I do such a bold statement?
 Because
 ECM solves real world problems you want to have solved in your library,
 too.
 The developers using your library will expect a proper Config.cmake file
 and a
 proper Version.cmake file and ECM solves that for you. I'm using ECM in
 libraries not intended to ever be part of KF5 exactly for that and before I
 added that dependency it was just not possible to reliable use the library.

 Of course you can also write that CMake Foo all by yourself but I have the
 feeling that the knowledge to write proper CMake Foo is not wide spread
 among
 our developers. I personally prefer to use the CMake Foo written by people
 who
 know their stuff instead of doing my own.

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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

How are they are going to change? The etc here is important too. Remember
that NMQt follows NetworkManager's release numbers, the same is true for
MMQt and ModemManager. That is for simplify things for those who are used
to NetworkManager's release number, I prefer to keep that. Is there any
document explaining what is needed for a library to be part of KF5? I
prefer to know all the implications beforehand.

Keeping the dependencies for NMQt/MMQt to the minimum is important to make
them more available to other users. I know Plasma NM is the main user of
NMQt/MMQt. However we moved them to separated libraries to make them
usefull for non-KDE users as well. Adding ECM's kde-modules as dependencies
seems to break that goal. If it was not to make NMQt/MMQt usefull for
non-KDE users it would be simpler to merge them into Plasma NM again (like
they are in Plasma NM 0.9.0.x). That would remove the burden of keeping
binary compatibility from us.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Mon, Apr 7, 2014 at 9:46 AM, Jan Grulich jgrul...@redhat.com wrote:

 Do you agree also with making libmm-qt/libnm-qt as KDE Frameworks? That
 means
 probably change versions, releasing etc.

 Jan

 On Monday 07 of April 2014 09:38 Lamarque Souza wrote:
  Hi all,
 
  I have cloned ECM git repo and looked at it. I agree that it is small and
  it has useful features for NMQt/MMQt. I like the fact that it provides a
  FindNetworkManager.cmake. Ok, we can make ECM a hard dependency for
  NMQt/MMQt.
 
  My only concern now is the kde-modules that Jan used in NMQt/MMQt's
  framework branches. Is it necessary to have KF5 installed to use those
  modules? I am asking it because I did not find where in ECM repo the
  macro _kde_add_platform_definitions is defined.
 
  Lamarque V. Souza
 
  KDE's Network Management maintainer
 
  http://planetkde.org/pt-br
 
  On Mon, Apr 7, 2014 at 5:31 AM, Martin Gräßlin mgraess...@kde.org
 wrote:
   On Monday 07 April 2014 10:14:12 David Faure wrote:
On Monday 07 April 2014 09:47:43 Jan Grulich wrote:
 You are still talking about users, but I'm sure that 99% of them
 will
 install it from distro repositories and because e-c-m is build
  
   dependency,
  
 they won't notice that. For remaining 1% of users you are talking
 about
 will be e-c-m available from distro repositories as well, so what's
 the
 problem? Now those libraries are compiled mostly because of Plasma
 NM,
 which requires unreleased versions (i.e. for frameworks version)
 and
 in
 this case they have already e-c- m installed.

 I don't want to have libnm-qt/libmm-qt as separated libraries, I
 think
 that
 a lot of distributions have them in their repositories, because
 they
  
   are
  
 as
 dependency for Plasma NM. I would be really interested how many
 distributions would have it without Plasma NM. It makes sense to
 me be
 part
 of KF5, don't be separated and be more visible and available for
 other
 developers and users and probably less confusing for packagers.
   
I agree. ECM is a very tiny dependency to have, and in return it
 solves
a
large number of issues for you (deployment, cmake config files, qmake
  
   .pri
  
file, dependency handling for users of your libs, forwarding headers,
versioning, releasing, etc. etc.).
  
   This is IMHO the important point. Just look at what ECM provides and
 you
   will
   realize that it's extremely useful and that even if those libraries
 will
   not
   end up in KF5, they will use ECM. How can I do such a bold statement?
   Because
   ECM solves real world problems you want to have solved in your library,
   too.
   The developers using your library will expect a proper Config.cmake
 file
   and a
   proper Version.cmake file and ECM solves that for you. I'm using ECM in
   libraries not intended to ever be part of KF5 exactly for that and
 before
   I
   added that dependency it was just not possible to reliable use the
   library.
  
   Of course you can also write that CMake Foo all by yourself but I have
 the
   feeling that the knowledge to write proper CMake Foo is not wide spread
   among
   our developers. I personally prefer to use the CMake Foo written by
 people
   who
   know their stuff instead of doing my own.
  
   Cheers
   Martin

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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

NMQt and MMQt used to be backends for Solid. We moved them from Solid so
they can be used by more people, even people that do not use KDE software.
Forcing everybody to install KF5 just to compile them does not sound a good
thing to me. Imagine this talk with someone that want to use those
libraries and do not use KDE software:

User: I want to use it, what should I install to compile it?
Us: a c++ compiler, Qt (and its dependencies), NetworkManager (and its
dependencies), cmake and KF5.
User: What is KF5 and why is it needed.
Us: It's the next version of libraries used by KDE software.
User: Does NMQt use it?
Us: No.
User: Why do I need to install it?
Us: Because we want it.

There is absolutely no technical reason for NMQt/MMQt to depend on KF5. You
can make that dependency optional but do not force it over everybody like
you did in NMQt/MMQt's frameworks branches.

Besides depending on KF5's cmake modules what else does being part of KDE
frameworks require?

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Fri, Apr 4, 2014 at 5:53 AM, Jan Grulich jgrul...@redhat.com wrote:

 And what is the problem depending on e-c-m? It's the base package, which
 will
 be available everywhere and being a part of KDE frameworks will make our
 libraries more visible and connected to KDE. We should be definitely part
 of
 frameworks, like Solid. Well, libnm-qt/libmm-qt are  basically Solid
 libraries.

 Those libraries are reusable, they are basically Qt API for
 NetworkManager/ModemManager, so you can manage connections and devices.

 Jan

 On Friday 04 of April 2014 05:29 Lamarque Souza wrote:
  Both libraries are meant to be reusable. What I meant with merge is the
  fact that the branches frameworks in NMQt and MMQt depends on KF5's
 cmake
  modules. I still want NMQt/MMQt usable for those that use Qt but not
 KDE's
  libraries (kdelibs and KF5).
 
  Lamarque V. Souza
 
  Em 04/04/2014 02:55, Kevin Ottens er...@kde.org escreveu:
   Hello,
  
   On Thursday 03 April 2014 20:19:45 Lamarque Souza wrote:
Well, NetworkManagerQt and ModemManagerQt are Qt only libraries since
the
beginning. They are not meant to depend on any KDE libraries as I
 said,
  
   so
  
they are not meant to be merged to KF5.
  
   Note this is a blatant logic mistake. All the tier 1 frameworks depend
   only on
   Qt too, but still they are very much part of KF5.
  
   There might be reasons to not have those two in KF5, but the one you
   advance
   is clearly the wrong one.
  
   Regards.
   --
   Kévin Ottens, http://ervin.ipsquad.net
  
   KDAB - proud supporter of KDE, http://www.kdab.com

 --
 Jan Grulich
 Red Hat Czech, s.r.o
 jgrul...@redhat.com

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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Both libraries are meant to be reusable. What I meant with merge is the
fact that the branches frameworks in NMQt and MMQt depends on KF5's cmake
modules. I still want NMQt/MMQt usable for those that use Qt but not KDE's
libraries (kdelibs and KF5).

Lamarque V. Souza
Em 04/04/2014 02:55, Kevin Ottens er...@kde.org escreveu:

 Hello,

 On Thursday 03 April 2014 20:19:45 Lamarque Souza wrote:
  Well, NetworkManagerQt and ModemManagerQt are Qt only libraries since the
  beginning. They are not meant to depend on any KDE libraries as I said,
 so
  they are not meant to be merged to KF5.

 Note this is a blatant logic mistake. All the tier 1 frameworks depend
 only on
 Qt too, but still they are very much part of KF5.

 There might be reasons to not have those two in KF5, but the one you
 advance
 is clearly the wrong one.

 Regards.
 --
 Kévin Ottens, http://ervin.ipsquad.net

 KDAB - proud supporter of KDE, http://www.kdab.com


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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

 In CMakeLists.txt:

find_package(ECM 0.0.12 REQUIRED NO_MODULE)
include(KDEInstallDirs)
include(KDEFrameworkCompilerSettings)
include(KDECMakeSettings)

The way it is now you need to install KF'5's cmake modules to parse NMQt's
CMakeLists.txt, nothing more from KF5 is used. In master branch we use
modules from cmake package only since the beginning to avoid depending on
kdelibs. I think KF5's cmake modules should be optional dependency and not
hard dependency.

PS: I assuming the three includes above came from KF5. If they are included
in cmake package so there will be no problem.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Sat, Apr 5, 2014 at 12:08 PM, Albert Astals Cid aa...@kde.org wrote:

 El Divendres, 4 d'abril de 2014, a les 07:57:26, Lamarque Souza va
 escriure:
  NMQt and MMQt used to be backends for Solid. We moved them from Solid so
  they can be used by more people, even people that do not use KDE
 software.
  Forcing everybody to install KF5 just to compile them does not sound a
 good
  thing to me. Imagine this talk with someone that want to use those
  libraries and do not use KDE software:
 
  User: I want to use it, what should I install to compile it?
  Us: a c++ compiler, Qt (and its dependencies), NetworkManager (and its
  dependencies), cmake and KF5.
  User: What is KF5 and why is it needed.
  Us: It's the next version of libraries used by KDE software.
  User: Does NMQt use it?
  Us: No.
  User: Why do I need to install it?
  Us: Because we want it.

 That seems a bit strange. Can you point at where in the framework branch of
 libnm-qt KF5 is required but not used?

 Cheers,
   Albert

 
  There is absolutely no technical reason for NMQt/MMQt to depend on KF5.
 You
  can make that dependency optional but do not force it over everybody like
  you did in NMQt/MMQt's frameworks branches.
 
  Besides depending on KF5's cmake modules what else does being part of
 KDE
  frameworks require?
 
  Lamarque V. Souza
 
  KDE's Network Management maintainer
 
  http://planetkde.org/pt-br
 
  On Fri, Apr 4, 2014 at 5:53 AM, Jan Grulich jgrul...@redhat.com wrote:
   And what is the problem depending on e-c-m? It's the base package,
 which
   will
   be available everywhere and being a part of KDE frameworks will make
 our
   libraries more visible and connected to KDE. We should be definitely
 part
   of
   frameworks, like Solid. Well, libnm-qt/libmm-qt are  basically Solid
   libraries.
  
   Those libraries are reusable, they are basically Qt API for
   NetworkManager/ModemManager, so you can manage connections and devices.
  
   Jan
  
   On Friday 04 of April 2014 05:29 Lamarque Souza wrote:
Both libraries are meant to be reusable. What I meant with merge is
the
fact that the branches frameworks in NMQt and MMQt depends on KF5's
  
   cmake
  
modules. I still want NMQt/MMQt usable for those that use Qt but not
  
   KDE's
  
libraries (kdelibs and KF5).
   
Lamarque V. Souza
   
Em 04/04/2014 02:55, Kevin Ottens er...@kde.org escreveu:
 Hello,

 On Thursday 03 April 2014 20:19:45 Lamarque Souza wrote:
  Well, NetworkManagerQt and ModemManagerQt are Qt only libraries
  since
  the
  beginning. They are not meant to depend on any KDE libraries as I
  
   said,
  
 so

  they are not meant to be merged to KF5.

 Note this is a blatant logic mistake. All the tier 1 frameworks
 depend
 only on
 Qt too, but still they are very much part of KF5.

 There might be reasons to not have those two in KF5, but the one
 you
 advance
 is clearly the wrong one.

 Regards.
 --
 Kévin Ottens, http://ervin.ipsquad.net

 KDAB - proud supporter of KDE, http://www.kdab.com
  
   --
   Jan Grulich
   Red Hat Czech, s.r.o
   jgrul...@redhat.com


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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Is using ECM required to be part of KF5?

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Sat, Apr 5, 2014 at 12:56 PM, Albert Astals Cid aa...@kde.org wrote:

 El Dissabte, 5 d'abril de 2014, a les 12:39:27, Lamarque Souza va escriure:
   In CMakeLists.txt:
 
  find_package(ECM 0.0.12 REQUIRED NO_MODULE)
  include(KDEInstallDirs)
  include(KDEFrameworkCompilerSettings)
  include(KDECMakeSettings)
 
  The way it is now you need to install KF'5's cmake modules to parse
 NMQt's
  CMakeLists.txt, nothing more from KF5 is used.

 That is not KF5, that is extra-cmake-modules

 http://quickgit.kde.org/?p=extra-cmake-modules.git

 Cheers,
   Albert

  In master branch we use
  modules from cmake package only since the beginning to avoid depending on
  kdelibs. I think KF5's cmake modules should be optional dependency and
 not
  hard dependency.
 
  PS: I assuming the three includes above came from KF5. If they are
 included
  in cmake package so there will be no problem.
 
  Lamarque V. Souza
 
  KDE's Network Management maintainer
 
  http://planetkde.org/pt-br
 
  On Sat, Apr 5, 2014 at 12:08 PM, Albert Astals Cid aa...@kde.org
 wrote:
   El Divendres, 4 d'abril de 2014, a les 07:57:26, Lamarque Souza va
  
   escriure:
NMQt and MMQt used to be backends for Solid. We moved them from
 Solid so
they can be used by more people, even people that do not use KDE
  
   software.
  
Forcing everybody to install KF5 just to compile them does not sound
 a
  
   good
  
thing to me. Imagine this talk with someone that want to use those
libraries and do not use KDE software:
   
User: I want to use it, what should I install to compile it?
Us: a c++ compiler, Qt (and its dependencies), NetworkManager (and
 its
dependencies), cmake and KF5.
User: What is KF5 and why is it needed.
Us: It's the next version of libraries used by KDE software.
User: Does NMQt use it?
Us: No.
User: Why do I need to install it?
Us: Because we want it.
  
   That seems a bit strange. Can you point at where in the framework
 branch
   of
   libnm-qt KF5 is required but not used?
  
   Cheers,
  
 Albert
  
There is absolutely no technical reason for NMQt/MMQt to depend on
 KF5.
  
   You
  
can make that dependency optional but do not force it over everybody
like
you did in NMQt/MMQt's frameworks branches.
   
Besides depending on KF5's cmake modules what else does being part
 of
  
   KDE
  
frameworks require?
   
Lamarque V. Souza
   
KDE's Network Management maintainer
   
http://planetkde.org/pt-br
   
On Fri, Apr 4, 2014 at 5:53 AM, Jan Grulich jgrul...@redhat.com
 wrote:
 And what is the problem depending on e-c-m? It's the base package,
  
   which
  
 will
 be available everywhere and being a part of KDE frameworks will
 make
  
   our
  
 libraries more visible and connected to KDE. We should be
 definitely
  
   part
  
 of
 frameworks, like Solid. Well, libnm-qt/libmm-qt are  basically
 Solid
 libraries.

 Those libraries are reusable, they are basically Qt API for
 NetworkManager/ModemManager, so you can manage connections and
 devices.

 Jan

 On Friday 04 of April 2014 05:29 Lamarque Souza wrote:
  Both libraries are meant to be reusable. What I meant with
 merge
  is
  the
  fact that the branches frameworks in NMQt and MMQt depends on
  KF5's

 cmake

  modules. I still want NMQt/MMQt usable for those that use Qt but
 not

 KDE's

  libraries (kdelibs and KF5).
 
  Lamarque V. Souza
 
  Em 04/04/2014 02:55, Kevin Ottens er...@kde.org escreveu:
   Hello,
  
   On Thursday 03 April 2014 20:19:45 Lamarque Souza wrote:
Well, NetworkManagerQt and ModemManagerQt are Qt only
 libraries
since
the
beginning. They are not meant to depend on any KDE libraries
 as
I

 said,

   so
  
they are not meant to be merged to KF5.
  
   Note this is a blatant logic mistake. All the tier 1 frameworks
  
   depend
  
   only on
   Qt too, but still they are very much part of KF5.
  
   There might be reasons to not have those two in KF5, but the
 one
  
   you
  
   advance
   is clearly the wrong one.
  
   Regards.
   --
   Kévin Ottens, http://ervin.ipsquad.net
  
   KDAB - proud supporter of KDE, http://www.kdab.com

 --
 Jan Grulich
 Red Hat Czech, s.r.o
 jgrul...@redhat.com


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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Hi all,

1) I do not see a real need to rename (again) ModemManagerQt and
NetworkManagerQt. They are named that way to indicate they depend only on
Qt and not on any library created by KDE, so they are tier1 since the
beginning. I am not familiar with KF5's library naming conventions but I
would like to keep the names as they are.

2) At least for now we can sync the release schedule of NMQt 0.9.10 with
KF5's even if NMQt is not officially part of KF5 yet. Besides, the biggest
user of NMQt is Plasma NM. ModemManagerQt is used by both Plasma NM and KDE
Telepathy. Is KDE Telepathy already ported to KF5? If porting Plasma NM to
KF5 (and Plasma 2) is going to take long there will be no real testing
programs for the KF5 versions of NMQt and MMQt, so no big pressure to
release them. I also think the first KF5 compatible release of NMQt should
be compatible with the first KF5 release of Plasma NM.

3) I will take a look at them when I get home. I cannot access KDE's git
repos from my work (no ssh/irc/smtp connections are allowed from here, just
https).

I plan to do changes in Plasma NM's wpa2 enterprise handling in the near
future. I can help porting it to KF5 too. Is there any big reason (besides
the usual lack of time) for not porting Plasma NM to KF5 now?

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Thu, Apr 3, 2014 at 11:47 AM, Jan Grulich jgrul...@redhat.com wrote:

 Hi,

 I would like to make libmm-qt and libnm-qt as KDE frameworks, they are
 based
 only on Qt, so they could be in Tier 1. I've already started (you can see
 framework branches) and I would like to ask to a few questions.

 1) How to name them? Until now we named them ModemManagerQt and
 NetworkManagerQt, but if we add KF5 prefix, it would be maybe better to
 name it
 only ModemManager/NetworkManager (with KF5 prefix), but there is a
 problem, if
 you will use it in the source code. Imagine including
 NetworkManager/WirelessDevice, it will work, but it kinda conflicts with
 NetworkManager headers and it's not clear which one is used, whether
 KF5::NetworkManager or NetworkManager.

 2) When to release them? We are not probably ready to be released with the
 first wave of KDE frameworks and it's probably too late. I don't know how
 often
 you plan to make releases, but at least for libnm-qt I would like to have
 support for NetworkManager 0.9.10, which should be probably released soon
 (or
 not) and it's also possible we will change something during porting
 plasma-nm
 to KF5/Plasma2 to our needs or make the API better.

 3) If you can take a look at it and tell what is wrong or necessary to
 change.
 I just took inspiration from some Tier1 frameworks and changed it
 accordingly.

 NOTE: plasma-nm is not ported to use framework branches, it is still using
 master branches and I don't want to port it yet.

 Thanks a lot.

 Cheers,
 Jan

 --
 Jan Grulich
 Red Hat Czech, s.r.o
 jgrul...@redhat.com

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

Re: libmm-qt/libnm-qt as KF5

[Lamarque Souza]

Well, NetworkManagerQt and ModemManagerQt are Qt only libraries since the
beginning. They are not meant to depend on any KDE libraries as I said, so
they are not meant to be merged to KF5. I looked the branches frameworks
and you basically did just that (make them depend on KF5). I think we
should keep them Qt only and not depend on KF5, the opposite is ok though.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br


On Thu, Apr 3, 2014 at 1:11 PM, Jan Grulich jgrul...@redhat.com wrote:

 On Thursday 03 of April 2014 12:52 Lamarque Souza wrote:
  Hi all,
 
  1) I do not see a real need to rename (again) ModemManagerQt and
  NetworkManagerQt. They are named that way to indicate they depend only on
  Qt and not on any library created by KDE, so they are tier1 since the
  beginning. I am not familiar with KF5's library naming conventions but I
  would like to keep the names as they are.

 It's just maybe a little bit strange having KF5 prefix and Qt suffix, but
 it's
 fine with me.

 
  2) At least for now we can sync the release schedule of NMQt 0.9.10 with
  KF5's even if NMQt is not officially part of KF5 yet. Besides, the
 biggest
  user of NMQt is Plasma NM. ModemManagerQt is used by both Plasma NM and
 KDE
  Telepathy. Is KDE Telepathy already ported to KF5? If porting Plasma NM
 to
  KF5 (and Plasma 2) is going to take long there will be no real testing
  programs for the KF5 versions of NMQt and MMQt, so no big pressure to
  release them. I also think the first KF5 compatible release of NMQt
 should
  be compatible with the first KF5 release of Plasma NM.
 

 KDE Telepathy uses the old version of ModemManagerQt, so Plasma NM is the
 only
 one using it actually. I guess the only thing we need is at least some
 testing
 tarballs when Plasma Next is finally out.

  3) I will take a look at them when I get home. I cannot access KDE's git
  repos from my work (no ssh/irc/smtp connections are allowed from here,
 just
  https).
 
  I plan to do changes in Plasma NM's wpa2 enterprise handling in the near
  future. I can help porting it to KF5 too. Is there any big reason
 (besides
  the usual lack of time) for not porting Plasma NM to KF5 now?
 

 Plasma NM is already ported to KF5/Plasma2 and it is working perfectly,
 except
 a few things that needs to be polished or finished. I meant it is not
 ported to
 my latest changes in libmm-qt/libnm-qt (framework branches).

 Jan

  On Thu, Apr 3, 2014 at 11:47 AM, Jan Grulich jgrul...@redhat.com
 wrote:
   Hi,
  
   I would like to make libmm-qt and libnm-qt as KDE frameworks, they are
   based
   only on Qt, so they could be in Tier 1. I've already started (you can
 see
   framework branches) and I would like to ask to a few questions.
  
   1) How to name them? Until now we named them ModemManagerQt and
   NetworkManagerQt, but if we add KF5 prefix, it would be maybe better to
   name it
   only ModemManager/NetworkManager (with KF5 prefix), but there is a
   problem, if
   you will use it in the source code. Imagine including
   NetworkManager/WirelessDevice, it will work, but it kinda conflicts
 with
   NetworkManager headers and it's not clear which one is used, whether
   KF5::NetworkManager or NetworkManager.
  
   2) When to release them? We are not probably ready to be released with
 the
   first wave of KDE frameworks and it's probably too late. I don't know
 how
   often
   you plan to make releases, but at least for libnm-qt I would like to
 have
   support for NetworkManager 0.9.10, which should be probably released
 soon
   (or
   not) and it's also possible we will change something during porting
   plasma-nm
   to KF5/Plasma2 to our needs or make the API better.
  
   3) If you can take a look at it and tell what is wrong or necessary to
   change.
   I just took inspiration from some Tier1 frameworks and changed it
   accordingly.
  
   NOTE: plasma-nm is not ported to use framework branches, it is still
 using
   master branches and I don't want to port it yet.
  
   Thanks a lot.
  
   Cheers,
   Jan
  
   --
   Jan Grulich
   Red Hat Czech, s.r.o
   jgrul...@redhat.com

 --
 Jan Grulich
 Red Hat Czech, s.r.o
 jgrul...@redhat.com

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

Re: Review Request 116098: Drop unneccessary dependency on extra-cmake-modules and use GNUInstallDirs

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On March 4, 2014, 4:13 p.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116098/
 ---
 
 (Updated March 4, 2014, 4:13 p.m.)
 
 
 Review request for KDE Frameworks, Daniel Nicoletti, Lamarque Souza, and 
 Sebastian Kügler.
 
 
 Repository: libnm-qt
 
 
 Description
 ---
 
 Drop unneccessary dependency on extra-cmake-modules and use GNUInstallDirs
 
 This means there is no longer a need for passing -DLIB_SUFFIX=64 on e.g.
 openSuSE, since CMake will detect the correct install directory for most
 distributions. If for some reason CMake doesn't detect the correct
 directory it can be overriden using e.g. -DCMAKE_INSTALL_LIBDIR=lib32
 
 
 Diffs
 -
 
   CMakeLists.txt 9918278 
   NetworkManagerQt.pc.cmake 2c3ab07 
   include/CMakeLists.txt 2b51ced 
   include/settings/CMakeLists.txt 1a00bdb 
 
 Diff: https://git.reviewboard.kde.org/r/116098/diff/
 
 
 Testing
 ---
 
 Installed into the right directories after applying the patch.
 
 
 Thanks,
 
 Alexander Richardson
 


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

Re: Review Request 116098: Drop unneccessary dependency on extra-cmake-modules and use GNUInstallDirs

[Lamarque Souza]

 On March 4, 2014, 4:42 p.m., Lamarque Souza wrote:
  Ship It!
 
 Lamarque Souza wrote:
 This should be ported to branches master and NM/0.9.8 as well.
 
 Alexander Richardson wrote:
 How should I commit it? Commit to oldest branch and then merge? Or rather 
 put this into qt5 and then apply patches to the other branches? The latter 
 might make merging a bit harder.

I usually apply to master e cherry-pick individual commits to NM/0.9.8. The 
CMakeListst.txt in qt5 branch is quit different from the ones in the other 
branches, your patch does not apply.


- Lamarque


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


On March 4, 2014, 4:13 p.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116098/
 ---
 
 (Updated March 4, 2014, 4:13 p.m.)
 
 
 Review request for KDE Frameworks, Daniel Nicoletti, Lamarque Souza, and 
 Sebastian Kügler.
 
 
 Repository: libnm-qt
 
 
 Description
 ---
 
 Drop unneccessary dependency on extra-cmake-modules and use GNUInstallDirs
 
 This means there is no longer a need for passing -DLIB_SUFFIX=64 on e.g.
 openSuSE, since CMake will detect the correct install directory for most
 distributions. If for some reason CMake doesn't detect the correct
 directory it can be overriden using e.g. -DCMAKE_INSTALL_LIBDIR=lib32
 
 
 Diffs
 -
 
   CMakeLists.txt 9918278 
   NetworkManagerQt.pc.cmake 2c3ab07 
   include/CMakeLists.txt 2b51ced 
   include/settings/CMakeLists.txt 1a00bdb 
 
 Diff: https://git.reviewboard.kde.org/r/116098/diff/
 
 
 Testing
 ---
 
 Installed into the right directories after applying the patch.
 
 
 Thanks,
 
 Alexander Richardson
 


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

Re: Review Request 116098: Use KDEInstallDirs

[Lamarque Souza]

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



CMakeLists.txt
https://git.reviewboard.kde.org/r/116098/#comment36213

NMQt is supposed to depend on Qt only.

What is E-C-M?

Does GNUInstallDirs support all platforms that NetworkManager support? I do 
not want to break things for those that have NetworkManager but not 
GNUInstallDirs.

Using -DLIB_SUFFIX=64 is simple and works for everybody. Is there any real 
issue solved by GNUInstallDirs that cannot be solved by -DLIB_SUFFIX=64?


- Lamarque Souza


On March 1, 2014, 11:14 a.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116098/
 ---
 
 (Updated March 1, 2014, 11:14 a.m.)
 
 
 Review request for KDE Frameworks, Daniel Nicoletti, Lamarque Souza, and 
 Sebastian Kügler.
 
 
 Repository: libnm-qt
 
 
 Description
 ---
 
 When I build libnm-qt on my openSuSE 64bit system libnm-qt ends up installing 
 into $prefix/lib instead of $prefix/lib64.
 I know this can be fixed using -DLIB_SUFFIX=64, but KDEInstallDirs already 
 handles this so why not use it.
 
 E-C-M is already required, so that is no problem. Not sure however whether it 
 is okay to use one of the KDE modules for libnm-qt.
 If not I will update this request to use GNUInstallDirs which also handles 
 the openSuSE case.
 
 Not sure who is responsible for the qt5 branch, so I just added kdeframeworks 
 as reviewers.
 
 
 Diffs
 -
 
   CMakeLists.txt 9918278 
   NetworkManagerQt.pc.cmake 2c3ab07 
 
 Diff: https://git.reviewboard.kde.org/r/116098/diff/
 
 
 Testing
 ---
 
 Installed into the right directories after applying the patch.
 grep -irn LIB_SUFFIX * returns nothing 
 
 
 Thanks,
 
 Alexander Richardson
 


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