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

Re: [Kde-hardware-devel] Do we want to drop support for "enterprise" devices in plasma-nm applet?

[Lamarque Souza]

Hi,

I think adding an option in the editor to disable the filters is better
than just reverting the patches. Just reverting the patches is going to be
another big visual change that might annoy someone else who will ask to
change it again. See the picture?

Lamarque V. Souza
Linux User #57137 - https://www.linuxcounter.net/user/57137

On Thu, Aug 10, 2017 at 3:31 AM, Jan Grulich  wrote:

> Hi,
>
> I would like to bring this for discussion again, there are apparently
> people
> [1] using those devices on desktop and even when there might be just a few
> of
> them we maybe want to reconsider supporting them. I've never removed the
> relevant code to support enterprise devices, they are just ignored and
> filtered out, but still the setting widgets are there. Problem is that I
> don't
> want to really show them in the applet or in the new kcm as it might bother
> 99% of users who actually don't need them. Instead, we might want to add an
> option to the applet to allow showing and creating such devices. What do
> you
> think? It shouldn't be extra work and everyone will be satisfied.
>
> [1] - https://bugs.kde.org/show_bug.cgi?id=376664
>
> Regards,
> Jan
>
> On čtvrtek 17. prosince 2015 9:53:27 CEST Jan Grulich wrote:
> > Done. I pushed a change which should now filter out mentioned
> > device/connection types from the applet, notifications etc. except the
> > connection editor.
> >
> > Regards,
> > Jan
> >
> > On Wednesday 16 of December 2015 15:26:51 Jan Grulich wrote:
> > > Hi,
> > >
> > > in the upcoming NetworkMager version they decided to drop support for
> > > "enterprise" devices in nm-applet, like bond, bridge, infiniband, team
> or
> > > vlan. The reason for this is that their usage on desktop is not that
> > > common
> > > and mostly they just take space, like virbr0 (bridge) connection which
> is
> > > created automatically by virtualbox or libvirt and you cannot remove
> it or
> > > do anything with it. Their preferred way to configure/manage those
> devices
> > > is now nmcli.
> > >
> > > I propose ignoring those devices only in the applet and keeping them
> only
> > > in the editor where you should be still able to configure them and even
> > > activated them.
> > >
> > > NM change:
> > > https://git.gnome.org/browse/network-manager-applet/commit/
> ?id=4b8eade[1]
> > >
> > > NM bug:
> > > https://bugzilla.gnome.org/show_bug.cgi?id=753369[2]
> > >
> > > And btw. gnome-shell also ignores those devices.
> > >
> > > What do you think?
> > >
> > > Regards,
> > > Jan
> >
> > ___
> > Kde-hardware-devel mailing list
> > Kde-hardware-devel@kde.org
> > https://mail.kde.org/mailman/listinfo/kde-hardware-devel
>
>

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: kdereview - xdg-desktop-portal-kde

[Lamarque Souza]

Hi,

My review:

. there are several code style inconsistencies, like "QDialog* parent" and
"Ui::AppChooserDialog * m_dialog". In some places you use app_id while in
other places you use appId.

. you use 0 in same places that you should use nullptr.

. there is no doxygen documentation at all.

. you can optimize

QString("%1:%2").arg(app_id).arg(id)

by doing this:

QString("%1:%2").arg(app_id, id)

since both app_id and id are QStrings.

. in AppChooser::chooseApplication() you do

QString heading;
...
heading = options.value(QLatin1String("heading")).toBool();

shouldn't heading be declared as bool?

. in the same method you do:

if (appDialog->exec()) {
results.insert(QLatin1String("choice"), appDialog->
selectedApplication());
appDialog->deleteLater();
return 0;
}

That means if the user rejects the dialog you never deletes it. That's a
memory leak. You do the same thing in FileChooser::openFile(),
FileChooser::saveFile() and Print::preparePrint().

. in DesktopPortal::handleMessage() you use message.arguments().at(4)
without checking if there are at least four arguments in message (a
QDBusMessage object). That is risky, if someone does not provide all the
required arguments your code can crash.

Lamarque V. Souza

http://planetkde.org/pt-br

On Fri, May 12, 2017 at 6:23 AM, Jan Grulich  wrote:

> Hi,
>
> it's been now three weeks and nobody either looked at the code or found any
> problem and raised objections. Can we proceed next and move this to place
> where is the rest of Plasma repositories? Or is there a longer period for
> which I have to wait until this can move on?
>
> Regards,
> Jan
>
> On pátek 21. dubna 2017 8:10:36 CEST Jan Grulich wrote:
> > Hi,
> >
> > I would like to request review of xdg-desktop-portal-kde [1]. We would
> like
> > to make it part of Plasma releases, see [2].
> >
> > What is xdg-desktop-portal-kde:
> > It's a KDE implementation of Flatpak portals backend [3], currently with
> > support of AppChooser, FileChooser, Notification and Print portals.
> >
> > One mentioned issue on plasma-devel mailing list was usage of Qt's
> private
> > print API. This will most likely go away if I find out it's useless,
> > otherwise I'll have to keep it as it's used to set CUPS properties which
> > are not available to the outside world.
> >
> > [1] - https://cgit.kde.org/xdg-desktop-portal-kde.git/
> > [2] - https://mail.kde.org/pipermail/plasma-devel/2017-April/069506.html
> > [3] -
> > http://flatpak.org/xdg-desktop-portal/portal-docs.
> html#idm140258860052032
> >
> > Regards,
> > Jan
>
>
>

Re: Review Request 130123: Fix the skip disabled backlight device

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On May 10, 2017, 1:58 a.m., AceLan Kao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130123/
> ---
> 
> (Updated May 10, 2017, 1:58 a.m.)
> 
> 
> Review request for Solid.
> 
> 
> Repository: powerdevil
> 
> 
> Description
> ---
> 
> Only raw backlight interface have the "enabled" file under "device"
> directory.
> So the commit
>5c0d35c skip the disabled backlight device
> affects all types of backlight interfaces is wrong, it will drop out all
> other type of backlight interfaces except raw tyep backlight.
> 
> To fix this, we just need to check the enabled file for raw backlight
> interfaces only.
> 
> Signed-off-by: AceLan Kao <ace...@acelan.idv.tw>
> 
> 
> Diffs
> -
> 
>   daemon/backends/upower/backlighthelper.h 
> 1382dd09938840371dbf34312fe2e8093abfbe10 
>   daemon/backends/upower/backlighthelper.cpp 
> e0eb6c461a7aac50533e833953977d46a7c8e4f3 
> 
> Diff: https://git.reviewboard.kde.org/r/130123/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> AceLan Kao
> 
>

Re: Review Request 130123: Fix the skip disabled backlight device

[Lamarque Souza]

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




daemon/backends/upower/backlighthelper.h (line 58)
<https://git.reviewboard.kde.org/r/130123/#comment68681>

static bool isRawBacklightEnabled(const QString );



daemon/backends/upower/backlighthelper.cpp (line 68)
<https://git.reviewboard.kde.org/r/130123/#comment68673>

You can pass the file path directly to the constructor:

QFile file(BACKLIGHT_SYSFS_PATH + interface + "/device/enabled");



daemon/backends/upower/backlighthelper.cpp (line 69)
<https://git.reviewboard.kde.org/r/130123/#comment68675>

Remove this line and declare this variable when it is really needed below.



daemon/backends/upower/backlighthelper.cpp (line 70)
<https://git.reviewboard.kde.org/r/130123/#comment68677>

This variable is not needed.



daemon/backends/upower/backlighthelper.cpp (line 72)
<https://git.reviewboard.kde.org/r/130123/#comment68674>

This line can be removed after the change above.



daemon/backends/upower/backlighthelper.cpp (line 74)
<https://git.reviewboard.kde.org/r/130123/#comment68678>

return false;



daemon/backends/upower/backlighthelper.cpp (line 77)
<https://git.reviewboard.kde.org/r/130123/#comment68676>

QByteBarry buffer = file.readLine().trimmed();



daemon/backends/upower/backlighthelper.cpp (line 79)
<https://git.reviewboard.kde.org/r/130123/#comment68679>

return true.

QFile's destructor already closes the file.



daemon/backends/upower/backlighthelper.cpp (line 83)
<https://git.reviewboard.kde.org/r/130123/#comment68680>

return false.



daemon/backends/upower/backlighthelper.cpp (line 110)
<https://git.reviewboard.kde.org/r/130123/#comment68682>

Code style: add a { at the and of this line and a } after the line below.


- Lamarque Souza


On May 9, 2017, 6:49 a.m., AceLan Kao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130123/
> ---
> 
> (Updated May 9, 2017, 6:49 a.m.)
> 
> 
> Review request for Solid.
> 
> 
> Repository: powerdevil
> 
> 
> Description
> ---
> 
> Only raw backlight interface have the "enabled" file under "device"
> directory.
> So the commit
>5c0d35c skip the disabled backlight device
> affects all types of backlight interfaces is wrong, it will drop out all
> other type of backlight interfaces except raw tyep backlight.
> 
> To fix this, we just need to check the enabled file for raw backlight
> interfaces only.
> 
> Signed-off-by: AceLan Kao <ace...@acelan.idv.tw>
> 
> 
> Diffs
> -
> 
>   daemon/backends/upower/backlighthelper.h 
> 1382dd09938840371dbf34312fe2e8093abfbe10 
>   daemon/backends/upower/backlighthelper.cpp 
> e0eb6c461a7aac50533e833953977d46a7c8e4f3 
> 
> Diff: https://git.reviewboard.kde.org/r/130123/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> AceLan Kao
> 
>

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 130115: skip the disabled backlight device

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On May 5, 2017, 1:16 p.m., AceLan Kao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130115/
> ---
> 
> (Updated May 5, 2017, 1:16 p.m.)
> 
> 
> Review request for Solid.
> 
> 
> Repository: powerdevil
> 
> 
> Description
> ---
> 
> While adjusting the brightness, if there are more than one graphics card
> on the machine, then there might be more than one backlight interface
> directories under /sys/backlight. Only one of them is active, and in the
> code, KDE picks the first one in the list as the backlight device. It
> might pick the wrong one if the active device is the second one.
> So, we try skipping the disabled one and can fix the brightness issue on
> some machines with more than one graphics cards.
> 
> Signed-off-by: AceLan Kao <ace...@acelan.idv.tw>
> 
> 
> Diffs
> -
> 
>   daemon/backends/upower/backlighthelper.cpp 
> fb5f7ed991c320b242224930e0109d03cb328a6f 
> 
> Diff: https://git.reviewboard.kde.org/r/130115/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> AceLan Kao
> 
>

Re: Review Request 130115: skip the disabled backlight device

[Lamarque Souza]

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




daemon/backends/upower/backlighthelper.cpp (line 86)
<https://git.reviewboard.kde.org/r/130115/#comment68670>

Code style: this is not correctly aligned.



daemon/backends/upower/backlighthelper.cpp (line 89)
<https://git.reviewboard.kde.org/r/130115/#comment68671>

Since you're reusing the variable file here you have to close it first 
before calling file.setFileName():

...
file.close();

file.setFileName(BACKLIGHT_SYSFS_PATH + interface + "/type");
...


- Lamarque Souza


On May 5, 2017, 7:28 a.m., AceLan Kao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130115/
> ---
> 
> (Updated May 5, 2017, 7:28 a.m.)
> 
> 
> Review request for Solid.
> 
> 
> Repository: powerdevil
> 
> 
> Description
> ---
> 
> While adjusting the brightness, if there are more than one graphics card
> on the machine, then there might be more than one backlight interface
> directories under /sys/backlight. Only one of them is active, and in the
> code, KDE picks the first one in the list as the backlight device. It
> might pick the wrong one if the active device is the second one.
> So, we try skipping the disabled one and can fix the brightness issue on
> some machines with more than one graphics cards.
> 
> Signed-off-by: AceLan Kao <ace...@acelan.idv.tw>
> 
> 
> Diffs
> -
> 
>   daemon/backends/upower/backlighthelper.cpp 
> fb5f7ed991c320b242224930e0109d03cb328a6f 
> 
> Diff: https://git.reviewboard.kde.org/r/130115/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> AceLan Kao
> 
>

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 119381: not find ProvidersList

[Lamarque Souza]

> On Jan. 21, 2017, 1 a.m., Albert Astals Cid wrote:
> > this doesn't apply anymore, should it be updated or discarded?

Discarded. It was superseded by https://git.reviewboard.kde.org/r/120879/


- Lamarque


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


On July 21, 2014, 8:08 a.m., zhang jun wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119381/
> ---
> 
> (Updated July 21, 2014, 8:08 a.m.)
> 
> 
> Review request for Solid.
> 
> 
> Repository: plasma-nm
> 
> 
> Description
> ---
> 
> When the system is in Chinese, country.length () is equal to 2, will not get 
> country code. country must be converted to Local8Bit to find the appropriate 
> country code.
> 
> 
> Diffs
> -
> 
>   libs/editor/mobileproviders.cpp 886d32c 
> 
> Diff: https://git.reviewboard.kde.org/r/119381/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> zhang jun
> 
>

Re: Review Request 129855: Treat no passphrase as Solid::UserCanceled error

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On Jan. 20, 2017, 9:56 a.m., Sergey Kalinichev wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129855/
> ---
> 
> (Updated Jan. 20, 2017, 9:56 a.m.)
> 
> 
> Review request for KDE Frameworks and Solid.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> This should help to distinguish "no password" from "no errors" cases
> 
> 
> Diffs
> -
> 
>   src/solid/devices/backends/hal/halstorageaccess.cpp 13f6df3 
>   src/solid/devices/backends/udisks2/udisksstorageaccess.cpp 29a9d23 
> 
> Diff: https://git.reviewboard.kde.org/r/129855/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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 129293: Add a CMake option to switch between HAL and UDisks managers on FreeBSD.

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On Oct. 31, 2016, 6:59 p.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129293/
> ---
> 
> (Updated Oct. 31, 2016, 6:59 p.m.)
> 
> 
> Review request for Solid and Tobias Berner.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> While these backends can coexist, this would result in duplicated device 
> entries in Solid for a single real device.
> 
> 
> Diffs
> -
> 
>   src/solid/devices/CMakeLists.txt d51ebb3 
>   src/solid/devices/config-solid.h.cmake a68af32 
>   src/solid/devices/managerbase.cpp eee4de5 
> 
> Diff: https://git.reviewboard.kde.org/r/129293/diff/
> 
> 
> Testing
> ---
> 
> Compile.
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Review Request 129293: Add a CMake option to switch between HAL and UDisks managers on FreeBSD.

[Lamarque Souza]

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




src/solid/devices/CMakeLists.txt (line 98)
<https://git.reviewboard.kde.org/r/129293/#comment67407>

I think you should change this description to something like:

"Use bsdisks/UDisks2 instead of hal to manage disk devices"


- Lamarque Souza


On Oct. 31, 2016, 12:58 p.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129293/
> ---
> 
> (Updated Oct. 31, 2016, 12:58 p.m.)
> 
> 
> Review request for Solid and Tobias Berner.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> While these backends can coexist, this would result in duplicated device 
> entries in Solid for a single real device.
> 
> 
> Diffs
> -
> 
>   src/solid/devices/CMakeLists.txt d51ebb3 
>   src/solid/devices/config-solid.h.cmake a68af32 
>   src/solid/devices/managerbase.cpp eee4de5 
> 
> Diff: https://git.reviewboard.kde.org/r/129293/diff/
> 
> 
> Testing
> ---
> 
> Compile.
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Review Request 129293: Add a CMake option to switch between HAL and UDisks managers on FreeBSD.

[Lamarque Souza]

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




src/solid/devices/CMakeLists.txt (line 97)
<https://git.reviewboard.kde.org/r/129293/#comment67401>

Add "if (CMAKE_SYSTEM_NAME MATCHES FreeBSD)" since this change is FreeBSD 
specific. This option does not make sense in Linux for instance.



src/solid/devices/CMakeLists.txt (line 117)
<https://git.reviewboard.kde.org/r/129293/#comment67403>

Your patch still compiles hal even though it will not be used.


- Lamarque Souza


On Oct. 31, 2016, 11:25 a.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129293/
> ---
> 
> (Updated Oct. 31, 2016, 11:25 a.m.)
> 
> 
> Review request for Solid and Tobias Berner.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> While these backends can coexist, this would result in duplicated device 
> entries in Solid for a single real device.
> 
> 
> Diffs
> -
> 
>   src/solid/devices/CMakeLists.txt d51ebb3 
>   src/solid/devices/config-solid.h.cmake a68af32 
>   src/solid/devices/managerbase.cpp eee4de5 
> 
> Diff: https://git.reviewboard.kde.org/r/129293/diff/
> 
> 
> Testing
> ---
> 
> Compile.
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Configuring Solid Action

[Lamarque Souza]

Hi,

The code that lists devices is in /src/solid/devices/frontend/devicemanager.cpp.
Look for Solid::Device::allDevices().

In the example you gave the backend used was udisks2: /src/solid/devices/backends/udisks2/udisksmanager.cpp, which
issues a dbus call to udisks and parses its output to get the data printed
by solid-hardware.

You can add a solid action in systemsettings -> Removable Storage -> Device
Actions

Lamarque V. Souza

http://planetkde.org/pt-br

On Wed, Jun 8, 2016 at 3:39 AM, Alexander Blum  wrote:

> Dear Ladies and Gentlemen,
>
> I have a question concerning KDE Solid device monitor. On the console, when
> I type in solid-hardware details '/org/freedesktop/UDisks2/
> drives/TOSHIBA_8732TNPJT_222873248044'
>
> I get the following output:
>
> udi = '/org/freedesktop/UDisks2/drives/TOSHIBA_8732TNPJT_222873248044'
> parent = '/org/freedesktop/UDisks2' (string)
> vendor = 'TOSHIBA' (string)
> product = '8732TNPJT' (string)
> description = 'TOSHIBA 8732TNPJT' (string)
> ...
>
> How does solid-hardware query the listed attributes and their values? Is
> there any way of configuring a solid action script that uses and verifies
> the attributes listed?
>
>
> Kind regards / Mit freundlichen Grüßen
>
>
> --
> Alexander Blum
>
> LANDESAMT FÜR VERMESSUNG UND GEOBASISINFORMATION RHEINLAND-PFALZ
>
> Von-Kuhl-Straße 49
> 56070 Koblenz
> Telefon 0261 492-115
> Telefax 0261 492-492
> alexander.b...@vermkv.rlp.de
> www.lvermgeo.rlp.de
>

Re: KDev-Embedded - Identify USB peripherals

[Lamarque Souza]

Hi,

I did not understand what you mean by "programmer".

Everybody needs to compare strings when dealing with serial ports. In you
case, I think you need more than that to identify JTAGs. USB-Serial ports
usually follows the pattern
"(ttyS|ttyUSB|ttyACM|ttyAMA|rfcomm|ttyO)[0-9]{1,3}" used in the first
example you gave. I have never used JTAGs so I do not know how they work.

Lamarque V. Souza

http://planetkde.org/pt-br

On Sun, Jun 12, 2016 at 4:49 PM, patrick JP 
wrote:

> Hello,
>
> My name is Patrick, a GSoC student working at KDevelop with KDev-Embedded
> (Embedded development plugin for KDevelop).
> Right know I am having some problem to identify the programmer in the USB
> port.
> I am using something like this
> 
>  and
> this
> 
>  with
> Solid.
> Someone can give an idea to how identify this USB interfaces like:
> USB-Serial and JTAGs ?
> Maybe by the VID/PID or using the string compare, but I'm not sure if it's
> the best solution to perform this operation.
>
> Thank you for the attention.
>
> Best regards,
>

Re: Review Request 128750: Fix compilation of kdecore

[Lamarque Souza]

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


Ship it!




Ship It!

- Lamarque Souza


On Aug. 24, 2016, 10:49 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128750/
> ---
> 
> (Updated Aug. 24, 2016, 10:49 p.m.)
> 
> 
> Review request for kdelibs and Andrea Iacovitti.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Without this I get
> 
> CMakeFiles/KF5Codecs.dir/kcharsets.cpp.o: In function 
> `KCharsets::fromEntity(QString const&)':
> kcharsets.cpp:(.text+0x5cf): undefined reference to `kde_findEntity'
> 
> Use gperf C++ generated code instead of C
> 
> See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41194 for more info
> 
> 
> Diffs
> -
> 
>   kdecore/localization/kcharsets.cpp 5b72432 
>   kdecore/localization/kentities.cc 8c18351 
>   kdecore/localization/kentities.gperf 19522fe 
> 
> Diff: https://git.reviewboard.kde.org/r/128750/diff/
> 
> 
> Testing
> ---
> 
> Compiles
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

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: [Kde-hardware-devel] 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-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-devel

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: [Kde-hardware-devel] 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-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-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: [Kde-hardware-devel] 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-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-devel

Re: [Kde-hardware-devel] MMQT API - Asynchronous mode

[Lamarque Souza]

Hi,

Jan is right, you have to use Q_OBJECT to use signals/slots in Qt.

An asynchronous pseudo code based on the example provided in MMQt source
code would look like:

SomeQWidget::someMethod()
{
ModemManager::ModemMessaging::Ptr messaging =
modemdevice->messagingInterface();

ModemManager::ModemMessaging::Message msg;
msg.number = "number";
msg.text = "message text";

QDBusReply sms = messaging->createMessage(msg);
ModemManager::Sms::Ptr tmp = messaging->findMessage(sms.value());
QDBusPendingReply<> reply = tmp->send();

// This is the important part:
QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(reply, this);

// This only works with Qt5:
connect(watcher, ::finished, [reply, this]() {
// This code run after the message has been sent.
if (reply.isError()) {
qDebug() << reply.error().message();
}
});
}

Lamarque V. Souza

http://planetkde.org/pt-br

On Thu, Jun 2, 2016 at 4:35 AM, Jan Grulich  wrote:

> Hi,
>
> On pondělí 11. dubna 2016 11:11:52 CEST Charles A wrote:
> > Hi!
> > I have been using the MMQT api in order to send SMS messages.
> > I have been using MMQT version 5.2.2 and it works great! I recently found
> > an issue where the signalQuality (in ModemManager::Modem Interface) would
> > return 0 even though the signal quality returned by dbus was not 0.
> > Since then I decided to upgrade MMQT to a more recent version, I tried
> MMQT
> > 5.18.0 and my old code seem to not work at all with the MMQT 5.18.0
> > version.
>
> The API changed a bit after modemmanager-qt became a KDE framework.
>
> > After reading a bit, I stumbled upon the following bug report (
> > https://bugs.kde.org/show_bug.cgi?id=358261) and I realized I need to
> use
> > to Async mode to make it work since I was using the sync method.
> >
> > I wanted to know if there are any examples on how to use Async mode in
> > order to send an SMS message?
>
> Unfortunately there is no example for that particular case, but if you go
> through plasma-nm, there's a lot of stuff using networkmanager-qt/
> modemmanager-qt code (check src/libs/models/networkmodel.cpp).
>
> > I have been trying to use SIGNAL/SLOT method but I am not sure I am doing
> > it right. I am trying to connect to the signal without being a Q_OBJECT.
>
> I think you need to derive your class from QObject to be able to use Qt's
> signal/slot mechanism.
>
> You can also send us your code so we can take a look and help you more with
> that.
>
> > Thank you!
> > Charles
>
>
> Regards,
> Jan
> --
> Jan Grulich 
> Software Engineer, Desktop team
> Red Hat Czech
> ___
> Kde-hardware-devel mailing list
> Kde-hardware-devel@kde.org
> https://mail.kde.org/mailman/listinfo/kde-hardware-devel
>
___
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-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: [Kde-hardware-devel] Do we want to drop support for "enterprise" devices in plasma-nm applet?

[Lamarque Souza]

Hi,

I am Ok with your both suggestions (dropping support for them from the
applet and keeping the support in the editor). I am also Ok if you prefer
to also drop the support from the editor. I, for one, have never used those
"enterprise" devices myself.

Lamarque V. Souza

http://planetkde.org/pt-br

On Wed, Dec 16, 2015 at 12:26 PM, Jan Grulich  wrote:

> Hi,
>
>
>
> in the upcoming NetworkMager version they decided to drop support for
> "enterprise" devices in nm-applet, like bond, bridge, infiniband, team or
> vlan. The reason for this is that their usage on desktop is not that common
> and mostly they just take space, like virbr0 (bridge) connection which is
> created automatically by virtualbox or libvirt and you cannot remove it or
> do anything with it. Their preferred way to configure/manage those devices
> is now nmcli.
>
>
>
> I propose ignoring those devices only in the applet and keeping them only
> in the editor where you should be still able to configure them and even
> activated them.
>
>
>
> NM change:
>
> https://git.gnome.org/browse/network-manager-applet/commit/?id=4b8eade
>
>
>
> NM bug:
>
> https://bugzilla.gnome.org/show_bug.cgi?id=753369
>
>
>
> And btw. gnome-shell also ignores those devices.
>
>
>
> What do you think?
>
>
>
> Regards,
>
> Jan
>
> --
>
> Jan Grulich 
>
> Software Engineer, Desktop team
>
> Red Hat Czech
>
> ___
> Kde-hardware-devel mailing list
> Kde-hardware-devel@kde.org
> https://mail.kde.org/mailman/listinfo/kde-hardware-devel
>
>
___
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-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: [Kde-hardware-devel] Review Request 126096: Fix memory leak in kscreen backend of libkscreen

[Lamarque Souza]

> On Nov. 17, 2015, 4:08 p.m., Martin Gräßlin wrote:
> > backends/xrandr/xrandrscreen.cpp, line 67
> > <https://git.reviewboard.kde.org/r/126096/diff/1/?file=417181#file417181line67>
> >
> > Suggestion QScopedPointer<xcb_blah, QScopedPointerPodDeleter>
> 
> Daniel Vrátil wrote:
> Yes please, but use XCB::ScopedPointer from xcbwrapper.h instead.

Do you think it is safe to delete the pointer returned by 
xcb_randr_get_screen_resources_current_reply() in XRandR::screenResources()? I 
seems so, just wondering if this patch can side effects in the "hack" used in 
XRandR::screenResources().


- Lamarque


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


On Nov. 17, 2015, 4:31 p.m., Lamarque Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126096/
> ---
> 
> (Updated Nov. 17, 2015, 4:31 p.m.)
> 
> 
> Review request for Plasma, Solid and Daniel Vrátil.
> 
> 
> Repository: libkscreen
> 
> 
> Description
> ---
> 
> libkscreen needs to free memory of xcb_randr_get_screen_resources_reply.
> 
> 
> Diffs
> -
> 
>   backends/xrandr/xrandrscreen.cpp 8df957e 
> 
> Diff: https://git.reviewboard.kde.org/r/126096/diff/
> 
> 
> Testing
> ---
> 
> No memory leak and everything still works.
> 
> 
> Thanks,
> 
> Lamarque Souza
> 
>

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

Re: [Kde-hardware-devel] Review Request 126096: Fix memory leak in kscreen backend of libkscreen

[Lamarque Souza]

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

(Updated Nov. 17, 2015, 4:41 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma, Solid and Daniel Vrátil.


Changes
---

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


Repository: libkscreen


Description
---

libkscreen needs to free memory of xcb_randr_get_screen_resources_reply.


Diffs
-

  backends/xrandr/xrandrscreen.cpp 8df957e 

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


Testing
---

No memory leak and everything still works.


Thanks,

Lamarque Souza

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

[Kde-hardware-devel] Review Request 126096: Fix memory leak in kscreen backend of libkscreen

[Lamarque Souza]

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

Review request for Solid and Daniel Vrátil.


Repository: libkscreen


Description
---

libkscreen needs to free memory of xcb_randr_get_screen_resources_reply.


Diffs
-

  backends/xrandr/xrandrscreen.cpp 8df957e 

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


Testing
---

No memory leak and everything still works.


Thanks,

Lamarque Souza

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

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: [Kde-br] Cmake error with VTK[Br-Print3D Project]

[Lamarque Souza]

Hi Lays,

It looks like you did not compile qt support in your vtk installation. What
parameters did you pass do cmake when you compiled vtk?

Lamarque V. Souza

http://planetkde.org/pt-br

On Fri, Oct 30, 2015 at 8:40 PM, Lays Rodrigues <
laysrodriguessi...@gmail.com> wrote:

> Hi folks! Good night!
> I'm trying to compile my qt app with a new class with VTK, but this error
> occurs and i dont find solution.
> http://pastebin.com/dsvPDbdz
>
> The paths of files contents with vtk are:
> /usr/local/lib/cmake/vtk-6.3
> /usr/local/include/vtk-6.3
>
> The CmakeLists.txt is:
> http://pastebin.com/MGWihAHT
>
> I'm really lost! Any help?
>
> --
> __
> *Lays Rodrigues*
> *Developer Front-End at Br-Print3D Project*
> *Student of Computer Science at UFF/PURO*
> Organizadora da Semana da Computação UFF-PURO
> www.facebook.com/semanacomputacaopuro
> www.facebook.com/brprint3d
>
>
>
>
> ___
> Kde-br mailing list
> kde...@kde.org
> https://mail.kde.org/mailman/listinfo/kde-br
>
>

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

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: RFC: Enabling -DECM_ENABLE_SANITIZERS='address' in jenkins

[Lamarque Souza]

I agree but there is a problem: it can catch a lot of errors in our
dependency libraries (upstream bugs). I had this problem when I used it
with a program I develop at my work. Enabling it for all programs at once
and fixing all those upstream bugs can be overwhelming. Maybe we should do
it for a limited number of programs first and add more programs as we fix
the errors.

Lamarque V. Souza

http://planetkde.org/pt-br

On Thu, Sep 10, 2015 at 5:36 PM, Albert Astals Cid  wrote:

> We have this nice ECM module that gives us the option to compile with ASAN.
>
> I'd like to propose that we enable it by default in jenkins.
>
> This way we get all the autotests run with ASAN and potentially catch more
> bugs/regressions.
>
> Comments?
>
> Cheers,
>   Albert
>

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

Re: Using nullptr instead of Q_NULLPTR

[Lamarque Souza]

On Thu, Aug 13, 2015 at 7:59 AM, Ivan Čukić ivan.cu...@kde.org wrote:

  I prefer the first option, it's the way forward and if someone was using
 
  I'd say that requiring a newer gcc just like that would go against the
  nature of the KF5 project.

 I don't really see why it is against the nature of KF5. It would not
 be the first time we require a higher compiler version than the one
 required by the Qt version we require.

 Qt 5.5 requires gcc 4.8 for linux and windows. So even they increment
 the versions from time to time.

 We can switch everything to Q_NULLPTR even without anyone complaining
 about it, but we will have to switch back in a few releases.

 Cheerio,
 Ivan


Qt 5.1 is still officially supported [1] and its minimum requirement is gcc
4.6.1 on Linux platform, 4.4.1 on Arm and 4.2 on OS X (?!). Qt 5.5 can be
compiled with gcc 4.6.3 according to [1], so bumping the requirement to
4.6.1 seems reasonable.

[1] http://doc.qt.io/QtSupportedPlatforms/

Lamarque V. Souza

http://planetkde.org/pt-br

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: Review Request 105831: Allow symlink creation for kio protocols that support it

[Lamarque Souza]

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

(Updated Aug. 8, 2015, 2:47 p.m.)


Status
--

This change has been discarded.


Review request for kdelibs.


Repository: kdelibs


Description
---

Some kio protocols support creating symlinks but a change in 
kio/kio/copyjob.cpp hardcoded symlink creation to only when source and 
destination protocols are the same. That change was added to fix a problem with 
ftp protocol creating symlinks instead of copying files (there is a comment in 
the source code about that). I think instead of forcing source and destination 
use the same protocol we should test if the destination protocol supports 
creating symlinks (ftp protocol does not). That would allow kio's like fish and 
nfs create symlinks. I am also working on some other changes that could use 
this feature.


Diffs
-

  kio/kio/copyjob.cpp 8dde763 

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


Testing
---


Thanks,

Lamarque Souza

Re: Review Request 104526: Make nepomuk runner remove undesired matches

[Lamarque Souza]

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

(Updated Aug. 8, 2015, 2:47 p.m.)


Status
--

This change has been discarded.


Review request for KDE Runtime.


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


Repository: kde-workspace


Description
---

Nepomuk runner returns matches like global namespace, local namespace and 
PaginatedTextDocument which pollutes the query's returned matches. This patch 
adds a NegationTerm to the nepomuk query to filter them. If somebody has a 
better way to prevent those matches from appearing please let me know.


Diffs
-

  plasma/generic/runners/nepomuksearch/queryclientwrapper.h 48c1eaa 
  plasma/generic/runners/nepomuksearch/queryclientwrapper.cpp 0b828b0 

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


Testing
---

Works in my Plasma Active installation.


Thanks,

Lamarque Souza

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: [Kde-hardware-devel] Review Request 102983: Added support for rfkill

[Lamarque Souza]

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



bluedevil/bluedevilmanager.h (line 109)
https://git.reviewboard.kde.org/r/102983/#comment56751

We use camel case for enumerate's names too.



bluedevil/bluedevilmanager.h (line 136)
https://git.reviewboard.kde.org/r/102983/#comment56755

isRfkillSwitchSupported() const;



bluedevil/bluedevilmanager.cpp (line 41)
https://git.reviewboard.kde.org/r/102983/#comment56756

Is rfkill supported in BSD? If so then this dev path may be different in 
that platform.



bluedevil/bluedevilmanager.cpp (line 41)
https://git.reviewboard.kde.org/r/102983/#comment56757

Is rfkill supported in BSD? If so then this dev path may be different in 
that platform.



bluedevil/bluedevilmanager.cpp (line 41)
https://git.reviewboard.kde.org/r/102983/#comment56758

Is rfkill supported in BSD? If so then this dev path may be different in 
that platform.



bluedevil/bluedevilmanager.cpp (line 156)
https://git.reviewboard.kde.org/r/102983/#comment56760

bool isValid() const



bluedevil/bluedevilmanager.cpp (line 156)
https://git.reviewboard.kde.org/r/102983/#comment56761

bool isValid() const



bluedevil/bluedevilmanager.cpp (line 383)
https://git.reviewboard.kde.org/r/102983/#comment56762

Use normalized signal/slots [1]:

connect(d-m_rfkill.getFileSystemWatcher(), SIGNAL(fileChanged(QString)),
   this, SLOT(_k_rfkillChanged(QString)));

[1] http://doc.qt.io/qt-4.8/signalsandslots.html



bluedevil/test/bluedeviltest.cpp (line 103)
https://git.reviewboard.kde.org/r/102983/#comment56752

Please fix identation. I think the text should RfkillSwitch supported! 
Status: 



bluedevil/test/bluedeviltest.cpp (line 119)
https://git.reviewboard.kde.org/r/102983/#comment56753

} else {



bluedevil/test/bluedeviltest.cpp (line 121)
https://git.reviewboard.kde.org/r/102983/#comment56754

RfkillSwitch not supported in this platform


- Lamarque Souza


On July 11, 2015, 8:43 p.m., Rüdiger Sonderfeld wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/102983/
 ---
 
 (Updated July 11, 2015, 8:43 p.m.)
 
 
 Review request for Plasma and Solid.
 
 
 Repository: libbluedevil
 
 
 Description
 ---
 
 rfkill is a subsystem of the Linux Kernel to disable radio transmitters 
 (including Bluetooth). This patch adds support for rfkill to libbluedevil. An 
 interface is added to the Manager class.
 
 rfkill support is important because gnome uses rfkill to deactivate 
 bluetooth. Therefore if bluetooth is currently disabled in the 
 gnome-control-center it can not be used by kde. With this patch the proper 
 API to reenable bluetooth is provided.
 
 Disabling bluetooth with rfkill has the advantage that some computers switch 
 off the bluetooth LED. Which does not happen (at least on my Thinkpad) if 
 it's just powered off (as kde does at the moment).
 
 rfkill can also detect if bluetooth has been deactivated by a hardware switch.
 
 btw. please mark https://gitorious.org/libbluedevil/libbluedevil as obsolete 
 and update http://projects.ufocoders.com/projects/libbluedevil/wiki. I used 
 the old gitorious code base at first.
 
 
 Diffs
 -
 
   bluedevil/bluedevilmanager.h 0386b8e 
   bluedevil/bluedevilmanager.cpp 594d3bd 
   bluedevil/test/bluedeviltest.cpp 2d3d0e2 
 
 Diff: https://git.reviewboard.kde.org/r/102983/diff/
 
 
 Testing
 ---
 
 I added a test to bluedeviltest and it works for me.
 
 
 Thanks,
 
 Rüdiger Sonderfeld
 


___
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-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: [Kde-hardware-devel] Review Request 123263: WIP: KDE Connect backend for Solid

[Lamarque Souza]

 On April 5, 2015, 5:54 p.m., Lamarque Souza wrote:
  src/solid/devices/backends/kdeconnect/kdeconnectbattery.cpp, line 79
  https://git.reviewboard.kde.org/r/123263/diff/1/?file=360131#file360131line79
 
  Is it possible to retrieve a correct value for this and the other 
  values below (timeToEmpty, timeToFull, etc)?
 
 Kai Uwe Broulik wrote:
 I suppose, but that needs implementing on the KDE Connect side
 
 Albert Vaca Cintora wrote:
 From Android we could expose any of these: 
 http://developer.android.com/reference/android/os/BatteryManager.html
 
 Which ones would be useful in solid?

According to /usr/include/solid/battery.h the ones we really need are: for 
isPlugged we need BATTERY_PLUGGED_AC, BATTERY_PLUGGED_USB and 
BATTERY_PLUGGED_WIRELESS. isPowerSupply: I think it is always false in this 
case. type: always PhoneBattery. chargePercent: BATTERY_PROPERTY_CAPACITY. 
capacity: BATTERY_PROPERTY_CHARGE_COUNTER. isRechargeable: always true. 
chargeState: BATTERY_STATUS_CHARGING, BATTERY_STATUS_DISCHARGING, 
BATTERY_STATUS_FULL and BATTERY_STATUS_NOT_CHARGING; BATTERY_STATUS_UNKNOWN can 
also be used, but Solid does not expose such a value.


- Lamarque


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


On April 5, 2015, 8:56 p.m., Kai Uwe Broulik wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123263/
 ---
 
 (Updated April 5, 2015, 8:56 p.m.)
 
 
 Review request for kdeconnect, Solid, Albert Vaca Cintora, and Emmanuel 
 Pescosta.
 
 
 Repository: solid
 
 
 Description
 ---
 
 This adds a KDE Connect backend to Solid enabling us to show phone battery 
 status in the device notifier, and have the phone appear in the device 
 notifier like an external storage.
 
 It is pretty synchronous at the moment. Any ideas how I could improve that? I 
 guess when someone wants to get devices, it creates a new device interface 
 and calls allDevices() or devicesFromQuery() immediately afterwards, so I 
 cannot just do that stuff async and defer population? Or call deviceAdded for 
 each of the initial ones once I have them?
 
 It requires adjustment in the kdeconnect KIO slave so it can handle addresses 
 like kdeconnect://org/kde/kdeconnect/123456 or 
 kdeconnect:udi=/org/kde/kdeconnect/123456 (like mtp kio does) because all 
 Dolphin or the device notifier have is the UDI. It also needs a new device 
 action that matches Portable Media Player with the kdeconnect protocol, 
 similar to mtp.
 
 By adjusting the predicate in Dolphin's Places sidebar (and probably the file 
 dialog) to query for the kdeconnect protocol, we can have its places 
 automatically updated without having KDE Connect manually mess with 
 KFilePlaces.
 
 
 Diffs
 -
 
   src/solid/devices/CMakeLists.txt 9271ae1 
   src/solid/devices/backends/kdeconnect/CMakeLists.txt PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectbattery.h PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectbattery.cpp PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdevice.h PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdevice.cpp PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdeviceinterface.h 
 PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdeviceinterface.cpp 
 PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectmanager.h PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectmanager.cpp PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectportablemediaplayer.h 
 PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectportablemediaplayer.cpp 
 PRE-CREATION 
   src/solid/devices/managerbase.cpp eee4de5 
 
 Diff: https://git.reviewboard.kde.org/r/123263/diff/
 
 
 Testing
 ---
 
 With some crash fixes already pushed to plasma-workspace it works pretty well.
 
 
 Thanks,
 
 Kai Uwe Broulik
 


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

Re: [Kde-hardware-devel] Review Request 123388: BluetoothMonitor: Simplify addBluetoothConnection + add haveBluetoothConnection

[Lamarque Souza]

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



kded/bluetoothmonitor.h (line 37)
https://git.reviewboard.kde.org/r/123388/#comment54069

bluetoothConnectionExists() is a better name for this method.



kded/bluetoothmonitor.h (line 38)
https://git.reviewboard.kde.org/r/123388/#comment54071

Is passing connectionName really necessary? The old code queries BlueZ to 
get the device name and use it as connectionName. That is something simular to 
using essid as connection name for wifi connections. Besides this particular 
change breaks compatibility with old Bluedevil versions. Is there a good reason 
for breaking compatibility here?



kded/bluetoothmonitor.cpp (line 82)
https://git.reviewboard.kde.org/r/123388/#comment54070

Are you sure regular expression rfcomm? does not appear in service 
variable anymore? Years ago  when I tested this it used to appear sometimes.


- Lamarque Souza


On April 17, 2015, 11:40 a.m., David Rosca wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123388/
 ---
 
 (Updated April 17, 2015, 11:40 a.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 This patch removes all Bluez related code from BluetoothMonitor. 
 It was used only to check if the device with specified address exists and if 
 it supports nap/dun profile.
 The addBluetoothConnection method is currently not used anywhere, so this 
 shouldn't be an issue. If the check is important, we can do one call on 
 Bluedevil DBus interface.
 
 Bluedevil was using this method in Bluez 4 versions, but it got killed during 
 porting to Bluez 5. 
 NAP connections are now automatically added on successful pairing with the 
 device.
 
 I plan to bring the functionality back in Bluedevil, so that user can 
 manually add a network connection (DUN/NAP) for Bluetooth devices.
 
 lxr search: 
 http://lxr.kde.org/search?v=kf5-qt5_filestring=_string=%22addBluetoothConnection%22
 
 
 Diffs
 -
 
   kded/bluetoothdbustype.h 1eeb3b2 
   kded/bluetoothdbustype.cpp 4eee8af 
   kded/bluetoothmonitor.h f5e74ec 
   kded/bluetoothmonitor.cpp 9d833d2 
   kded/dbus/org.freedesktop.DBus.ObjectManager.xml fa58c72 
   kded/dbus/org.freedesktop.DBus.Properties.xml a71f409 
   kded/CMakeLists.txt 51aa370 
   kded/monitor.h ae89d3e 
   kded/monitor.cpp e33b918 
 
 Diff: https://git.reviewboard.kde.org/r/123388/diff/
 
 
 Testing
 ---
 
 Works as before, mobile connection wizard appears when trying to add dun 
 connection.
 
 
 Thanks,
 
 David Rosca
 


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

Re: [Kde-hardware-devel] Review Request 123388: BluetoothMonitor: Simplify addBluetoothConnection + add haveBluetoothConnection

[Lamarque Souza]

 On April 17, 2015, 12:41 p.m., Lamarque Souza wrote:
  kded/bluetoothmonitor.h, line 41
  https://git.reviewboard.kde.org/r/123388/diff/2/?file=361568#file361568line41
 
  Is passing connectionName really necessary? The old code queries BlueZ 
  to get the device name and use it as connectionName. That is something 
  simular to using essid as connection name for wifi connections. Besides 
  this particular change breaks compatibility with old Bluedevil versions. Is 
  there a good reason for breaking compatibility here?
 
 David Rosca wrote:
 Old Bluedevil is using Bluez 4 and this code is for Bluez 5, so it's 
 broken already.
 I'd rather make sure that it will be called for valid device from 
 Bluedevil, besides Network Manager won't add connection for invalid device.
 
 Lamarque Souza wrote:
 I mean, using bluetooth device name for connection name is usefull, why 
 just remove this feature alltogether instead of porting it to Bluez 5?
 
 David Rosca wrote:
 The connection name will be provided by caller (Bluedevil) and it will be 
 Device name Network (same name as connection automatically added by Network 
 Manager)

That's what I thought. Ok then. Just update the copyright headers and this 
patch is good to go.


- Lamarque


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


On April 17, 2015, 11:40 a.m., David Rosca wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123388/
 ---
 
 (Updated April 17, 2015, 11:40 a.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 This patch removes all Bluez related code from BluetoothMonitor. 
 It was used only to check if the device with specified address exists and if 
 it supports nap/dun profile.
 The addBluetoothConnection method is currently not used anywhere, so this 
 shouldn't be an issue. If the check is important, we can do one call on 
 Bluedevil DBus interface.
 
 Bluedevil was using this method in Bluez 4 versions, but it got killed during 
 porting to Bluez 5. 
 NAP connections are now automatically added on successful pairing with the 
 device.
 
 I plan to bring the functionality back in Bluedevil, so that user can 
 manually add a network connection (DUN/NAP) for Bluetooth devices.
 
 lxr search: 
 http://lxr.kde.org/search?v=kf5-qt5_filestring=_string=%22addBluetoothConnection%22
 
 
 Diffs
 -
 
   kded/bluetoothdbustype.h 1eeb3b2 
   kded/bluetoothdbustype.cpp 4eee8af 
   kded/bluetoothmonitor.h f5e74ec 
   kded/bluetoothmonitor.cpp 9d833d2 
   kded/dbus/org.freedesktop.DBus.ObjectManager.xml fa58c72 
   kded/dbus/org.freedesktop.DBus.Properties.xml a71f409 
   kded/CMakeLists.txt 51aa370 
   kded/monitor.h ae89d3e 
   kded/monitor.cpp e33b918 
 
 Diff: https://git.reviewboard.kde.org/r/123388/diff/
 
 
 Testing
 ---
 
 Works as before, mobile connection wizard appears when trying to add dun 
 connection.
 
 
 Thanks,
 
 David Rosca
 


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

Re: [Kde-hardware-devel] Review Request 123388: BluetoothMonitor: Simplify addBluetoothConnection + add haveBluetoothConnection

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On April 17, 2015, 11:40 a.m., David Rosca wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123388/
 ---
 
 (Updated April 17, 2015, 11:40 a.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 This patch removes all Bluez related code from BluetoothMonitor. 
 It was used only to check if the device with specified address exists and if 
 it supports nap/dun profile.
 The addBluetoothConnection method is currently not used anywhere, so this 
 shouldn't be an issue. If the check is important, we can do one call on 
 Bluedevil DBus interface.
 
 Bluedevil was using this method in Bluez 4 versions, but it got killed during 
 porting to Bluez 5. 
 NAP connections are now automatically added on successful pairing with the 
 device.
 
 I plan to bring the functionality back in Bluedevil, so that user can 
 manually add a network connection (DUN/NAP) for Bluetooth devices.
 
 lxr search: 
 http://lxr.kde.org/search?v=kf5-qt5_filestring=_string=%22addBluetoothConnection%22
 
 
 Diffs
 -
 
   kded/bluetoothdbustype.h 1eeb3b2 
   kded/bluetoothdbustype.cpp 4eee8af 
   kded/bluetoothmonitor.h f5e74ec 
   kded/bluetoothmonitor.cpp 9d833d2 
   kded/dbus/org.freedesktop.DBus.ObjectManager.xml fa58c72 
   kded/dbus/org.freedesktop.DBus.Properties.xml a71f409 
   kded/CMakeLists.txt 51aa370 
   kded/monitor.h ae89d3e 
   kded/monitor.cpp e33b918 
 
 Diff: https://git.reviewboard.kde.org/r/123388/diff/
 
 
 Testing
 ---
 
 Works as before, mobile connection wizard appears when trying to add dun 
 connection.
 
 
 Thanks,
 
 David Rosca
 


___
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-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] Review Request 123262: ddccontrol support for PowerDevil

[Lamarque Souza]

 On April 5, 2015, 7:28 p.m., Lamarque Souza wrote:
  daemon/backends/upower/ddchelper.cpp, line 139
  https://git.reviewboard.kde.org/r/123262/diff/2/?file=360145#file360145line139
 
  Hmmm I do care about if this worked. People usually complains when 
  changing brightness does not work, see 
  https://bugs.kde.org/buglist.cgi?bug_status=UNCONFIRMEDbug_status=CONFIRMEDbug_status=ASSIGNEDbug_status=REOPENEDbug_status=RESOLVEDbug_status=CLOSEDcomponent=powermanagementcomponent=powermanagement-daemoncomponent=powermanagement-kcmlist_id=1240974query_format=advancedshort_desc=brightness%20workshort_desc_type=allwordssubstr
 
 Kai Uwe Broulik wrote:
 It's not like PowerDevil would do anything if setting the brightness 
 fails.

Maybe, but I think PowerDevil should at least print an error message in its log 
if something like setting brightness goes wrong. Several things can cause 
setting brightness to fail, an error message here is a big clue about what 
caused the problem.


- Lamarque


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


On April 5, 2015, 5:31 p.m., Kai Uwe Broulik wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123262/
 ---
 
 (Updated April 5, 2015, 5:31 p.m.)
 
 
 Review request for Solid.
 
 
 Repository: powerdevil
 
 
 Description
 ---
 
 This allows PowerDevil to use the ddccontrol utility to manipulate brightness 
 of external monitors through Display Data Channel.
 
 It becomes the last link in the fallback chain; it first tries XRandR, then 
 it tries the sysfs helper, and if all that fails it tries to use the 
 ddccontrol utility.
 
 dcccontrol unfortunately doesn't have machine-readable output so I have to 
 take apart its probing output to get the device name and id of the backlight 
 controls. Also it is pretty slow both initializing (kded startup, the helper 
 saves the values and writes directly to the address returned by the initial 
 probing) and setting brightness (that one doesn't block but it can take 
 seconds until your monitor actually changes brightness)
 
 
 Diffs
 -
 
   daemon/BackendConfig.cmake 295a8a2 
   daemon/backends/upower/ddc_helper_actions.actions PRE-CREATION 
   daemon/backends/upower/ddchelper.h PRE-CREATION 
   daemon/backends/upower/ddchelper.cpp PRE-CREATION 
   daemon/backends/upower/powerdevilupowerbackend.h 1c4dd59 
   daemon/backends/upower/powerdevilupowerbackend.cpp 87b9cc7 
 
 Diff: https://git.reviewboard.kde.org/r/123262/diff/
 
 
 Testing
 ---
 
 It's working pretty nicely, I can adjust brightness of my desktop monitor 
 through battery monitor and have it automatically dimmed after a timeout.
 
 
 Thanks,
 
 Kai Uwe Broulik
 


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

Re: [Kde-hardware-devel] Review Request 123262: ddccontrol support for PowerDevil

[Lamarque Souza]

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



daemon/backends/upower/ddchelper.h (line 6)
https://git.reviewboard.kde.org/r/123262/#comment53712

In kde-dev-scripts:relicensecheck.pl you stated that code you write can be 
LGPv2+, could you reflect that in this and the other files you are creating 
with this patch?



daemon/backends/upower/ddchelper.h (line 26)
https://git.reviewboard.kde.org/r/123262/#comment53713

Please do not use using inside a header file. That can lead to wierd 
compiling errors.



daemon/backends/upower/ddchelper.cpp (line 56)
https://git.reviewboard.kde.org/r/123262/#comment53714

This can lead to crash if pair is empty. At least add a Q_ASSERT(pair.size 
== 2) here.



daemon/backends/upower/ddchelper.cpp (line 79)
https://git.reviewboard.kde.org/r/123262/#comment53715

same here.



daemon/backends/upower/ddchelper.cpp (line 82)
https://git.reviewboard.kde.org/r/123262/#comment53716

You could add a Q_ASSERT(m_maximumBrightness  0) here to to catch any 
problem when converting to int.



daemon/backends/upower/ddchelper.cpp (line 135)
https://git.reviewboard.kde.org/r/123262/#comment53717

In QProcess doc: Returns true if the process finished; otherwise returns 
false (if the operation timed out, if an error occurred, or if this QProcess is 
already finished).. Already finished here is the same as not started yet, 
you should use if (!dccProcess.waitForStarted() || 
!dccProcess.waitForFinished()). I must also say that I really do not like 
synchronous calls in kded or in a plasmoid.


- Lamarque Souza


On April 5, 2015, 3:44 p.m., Kai Uwe Broulik wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123262/
 ---
 
 (Updated April 5, 2015, 3:44 p.m.)
 
 
 Review request for Solid.
 
 
 Repository: powerdevil
 
 
 Description
 ---
 
 This allows PowerDevil to use the ddccontrol utility to manipulate brightness 
 of external monitors through Display Data Channel.
 
 It becomes the last link in the fallback chain; it first tries XRandR, then 
 it tries the sysfs helper, and if all that fails it tries to use the 
 ddccontrol utility.
 
 dcccontrol unfortunately doesn't have machine-readable output so I have to 
 take apart its probing output to get the device name and id of the backlight 
 controls. Also it is pretty slow both initializing (kded startup, the helper 
 saves the values and writes directly to the address returned by the initial 
 probing) and setting brightness (that one doesn't block but it can take 
 seconds until your monitor actually changes brightness)
 
 
 Diffs
 -
 
   daemon/BackendConfig.cmake 295a8a2 
   daemon/backends/upower/ddc_helper_actions.actions PRE-CREATION 
   daemon/backends/upower/ddchelper.h PRE-CREATION 
   daemon/backends/upower/ddchelper.cpp PRE-CREATION 
   daemon/backends/upower/powerdevilupowerbackend.h 1c4dd59 
   daemon/backends/upower/powerdevilupowerbackend.cpp 87b9cc7 
 
 Diff: https://git.reviewboard.kde.org/r/123262/diff/
 
 
 Testing
 ---
 
 It's working pretty nicely, I can adjust brightness of my desktop monitor 
 through battery monitor and have it automatically dimmed after a timeout.
 
 
 Thanks,
 
 Kai Uwe Broulik
 


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

Re: [Kde-hardware-devel] Review Request 123263: WIP: KDE Connect backend for Solid

[Lamarque Souza]

 On April 5, 2015, 5:54 p.m., Lamarque Souza wrote:
  src/solid/devices/backends/kdeconnect/kdeconnectmanager.cpp, line 116
  https://git.reviewboard.kde.org/r/123263/diff/1/?file=360137#file360137line116
 
  Answering your question from this patch's description: yes, I prefer 
  that you call onDeviceAdded() slot for each device appended here. You can 
  also use a watcher in this class' constructor to update m_devices 
  asynchronously. Otherwise, no KdeConnectManager::deviceAdded() signal is 
  emitted during KdeConnectManager startup even though m_devices is not 
  empty. This method should just return the ids from m_devices.
 
 Kai Uwe Broulik wrote:
 Ok, thanks, I wasn't sure how that internally works.
 
 So I populate the list async and emit a deviceAdded signal for each of 
 them no matter whether I found them on the first sweep or when they were 
 actually added afterwards.

Yes.


- Lamarque


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


On April 5, 2015, 3:57 p.m., Kai Uwe Broulik wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123263/
 ---
 
 (Updated April 5, 2015, 3:57 p.m.)
 
 
 Review request for Solid, Albert Vaca Cintora and Emmanuel Pescosta.
 
 
 Repository: solid
 
 
 Description
 ---
 
 This adds a KDE Connect backend to Solid enabling us to show phone battery 
 status in the device notifier, and have the phone appear in the device 
 notifier like an external storage.
 
 It is pretty synchronous at the moment. Any ideas how I could improve that? I 
 guess when someone wants to get devices, it creates a new device interface 
 and calls allDevices() or devicesFromQuery() immediately afterwards, so I 
 cannot just do that stuff async and defer population? Or call deviceAdded for 
 each of the initial ones once I have them?
 
 It requires adjustment in the kdeconnect KIO slave so it can handle addresses 
 like kdeconnect://org/kde/kdeconnect/123456 or 
 kdeconnect:udi=/org/kde/kdeconnect/123456 (like mtp kio does) because all 
 Dolphin or the device notifier have is the UDI. It also needs a new device 
 action that matches Portable Media Player with the kdeconnect protocol, 
 similar to mtp.
 
 By adjusting the predicate in Dolphin's Places sidebar (and probably the file 
 dialog) to query for the kdeconnect protocol, we can have its places 
 automatically updated without having KDE Connect manually mess with 
 KFilePlaces.
 
 
 Diffs
 -
 
   src/solid/devices/CMakeLists.txt 9271ae1 
   src/solid/devices/backends/kdeconnect/CMakeLists.txt PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectbattery.h PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectbattery.cpp PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdevice.h PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdevice.cpp PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdeviceinterface.h 
 PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectdeviceinterface.cpp 
 PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectmanager.h PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectmanager.cpp PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectportablemediaplayer.h 
 PRE-CREATION 
   src/solid/devices/backends/kdeconnect/kdeconnectportablemediaplayer.cpp 
 PRE-CREATION 
   src/solid/devices/managerbase.cpp eee4de5 
 
 Diff: https://git.reviewboard.kde.org/r/123263/diff/
 
 
 Testing
 ---
 
 With some crash fixes already pushed to plasma-workspace it works pretty well.
 
 
 Thanks,
 
 Kai Uwe Broulik
 


___
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-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: [Kde-hardware-devel] Review Request 121677: Fix mobile connection wizard manual apn setting

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On Dec. 26, 2014, 7:46 a.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121677/
 ---
 
 (Updated Dec. 26, 2014, 7:46 a.m.)
 
 
 Review request for Solid and Lukáš Tinkl.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 Current problems:
 1. if user selects manual provider, apn text is not editable.
 2. if user select not listed in apn page, and go back to provider page and 
 go next again, apn text will be editable while it shouldn't.
 3. a redundant separator if there's on My plan is not listed
 
 This change reuses slotEnablePlanEditBox function to set mApnText to correct 
 state, and remove a separator if it's manually set provider.
 
 
 Diffs
 -
 
   libs/editor/widgets/mobileconnectionwizard.cpp 29ce4f7 
 
 Diff: https://git.reviewboard.kde.org/r/121677/diff/
 
 
 Testing
 ---
 
 problems above are fixed.
 
 
 Thanks,
 
 Xuetian Weng
 


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

Re: Fwd: Plasma 5.2 bits for kdereview

[Lamarque Souza]

ModemManagerQt is used by two KDE Applications, Plasma NM and KDE
Telepathy. The former uses it to retrieve information about mobile
broadband connections (operator and signal quality), unlocking modem's sim
card when activating 3G connections, listing available modems in mobile
connection wizard when creating 3G connections and, with NetworkManager 
0.9.10, Plasma NM also uses MMQt to setup a bluetooth connections (which is
process initiated by Bluedevil, so it is a feature indirectly used by
Bluedevil too). The later uses MMQt to read/send SMS using a cell phone.

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br

On Thu, Jan 8, 2015 at 1:04 PM, Sebastian Kügler se...@kde.org wrote:

 On Wednesday, January 07, 2015 20:31:57 Albert Astals Cid wrote:
   Albert has said he's fine with KDE Applications depending on Plasma (I
   think)
  
 http://mail.kde.org/pipermail/kde-frameworks-devel/2015-January/021332.htm
   l
 
  I said I'm fine with KDE Applications depending on Plasma to improve
  workpsace integration or something like that.
 
  BUT libkscreen and libmm-qt don't seem
  workpsace-integration-improvement-libs  to me but basic libs to access
  system stuff.

 libkscreen is mainly for reading and settings X configuration. That's quite
 workspace-specific. For use-cases typical for apps, there's way easier API
 (QScreen, for example).

 libmm-qt ... I dunno, why would an app use it? (I can construct a weird
 use-
 case, but in the end, information about modem setup is a system /
 workspace-
 specific feature, not something typically done in apps.

 There are no hard boundaries, much depends on interpretation, but given
 it's
 debatable, I think keeping these things together in kde/workspace makes
 sense.

 Cheers,
 --
 sebas

 http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9

Re: [Kde-hardware-devel] Review Request 121677: Fix mobile connection wizard manual apn setting

[Lamarque Souza]

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



libs/editor/widgets/mobileconnectionwizard.cpp
https://git.reviewboard.kde.org/r/121677/#comment50567

With your changes if mProvidersList-currentItem() is null here there will 
be a crash in Confirm Page. That can happen if package 
mobile-broadband-provider-info is not installed or its files were manually 
deleted for instance. You should call radioManualProvider-setChecked(true) to 
prevent that.



libs/editor/widgets/mobileconnectionwizard.cpp
https://git.reviewboard.kde.org/r/121677/#comment50566

The crash is here. mProviders-currentItem() will be a null pointer.



libs/editor/widgets/mobileconnectionwizard.cpp
https://git.reviewboard.kde.org/r/121677/#comment50563

If you remove the separator in Plan's combobox remove this line too.



libs/editor/widgets/mobileconnectionwizard.cpp
https://git.reviewboard.kde.org/r/121677/#comment50564

And this one too. Have you really tested your changes? The way your patch 
is it will always select the plan before the correct one in the list.


- Lamarque Souza


On Dec. 26, 2014, 7:46 a.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121677/
 ---
 
 (Updated Dec. 26, 2014, 7:46 a.m.)
 
 
 Review request for Solid and Lukáš Tinkl.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 Current problems:
 1. if user selects manual provider, apn text is not editable.
 2. if user select not listed in apn page, and go back to provider page and 
 go next again, apn text will be editable while it shouldn't.
 3. a redundant separator if there's on My plan is not listed
 
 This change reuses slotEnablePlanEditBox function to set mApnText to correct 
 state, and remove a separator if it's manually set provider.
 
 
 Diffs
 -
 
   libs/editor/widgets/mobileconnectionwizard.cpp 29ce4f7 
 
 Diff: https://git.reviewboard.kde.org/r/121677/diff/
 
 
 Testing
 ---
 
 problems above are fixed.
 
 
 Thanks,
 
 Xuetian Weng
 


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

Re: [Kde-hardware-devel] Review Request 121606: Port bluetooth connection support to bluez 5

[Lamarque Souza]

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



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50568

Code style: the comma at the end of this line must go to the start of the 
next line. That helps to make the commit a bit shorter.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50569

Same here. Just fix those two issues and the patch is good to go. Thanks 
for patch.


- Lamarque Souza


On Dec. 26, 2014, 7:21 a.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121606/
 ---
 
 (Updated Dec. 26, 2014, 7:21 a.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 bluez 5 + networkmanager 1.0.0 suppose to support dun connection, bluedevil 
 for kf5 IMHO will support bluez 5. So this patch also drops bluez4 support 
 completely.
 
 nm 1.0.0's dun support doesn't require modemmanager AFAIK, so that part is 
 dropped in this patch. (Not quite sure if nap is supported or not)
 
 bluetoothdbustype.cpp is used because there's metatype conflict with 
 libnm-qt, so declare the type in a separate file to avoid this.
 
 
 Diffs
 -
 
   kded/CMakeLists.txt 910f5fa 
   kded/bluetoothdbustype.h PRE-CREATION 
   kded/bluetoothdbustype.cpp PRE-CREATION 
   kded/bluetoothmonitor.h 5f43369 
   kded/bluetoothmonitor.cpp 3aaf701 
   kded/dbus/org.freedesktop.DBus.ObjectManager.xml PRE-CREATION 
   kded/dbus/org.freedesktop.DBus.Properties.xml PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/121606/diff/
 
 
 Testing
 ---
 
 qdbus org.kde.plasmanetworkmanagement /org/kde/plasmanetworkmanagement 
 org.kde.plasmanetworkmanagement.addBluetoothConnection [macaddress] dun can 
 now create connection.
 
 
 Thanks,
 
 Xuetian Weng
 


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

Re: [Kde-hardware-devel] Review Request 121606: Port bluetooth connection support to bluez 5

[Lamarque Souza]

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

Ship it!


Ship It!

- Lamarque Souza


On Dec. 26, 2014, 7:21 a.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121606/
 ---
 
 (Updated Dec. 26, 2014, 7:21 a.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 bluez 5 + networkmanager 1.0.0 suppose to support dun connection, bluedevil 
 for kf5 IMHO will support bluez 5. So this patch also drops bluez4 support 
 completely.
 
 nm 1.0.0's dun support doesn't require modemmanager AFAIK, so that part is 
 dropped in this patch. (Not quite sure if nap is supported or not)
 
 bluetoothdbustype.cpp is used because there's metatype conflict with 
 libnm-qt, so declare the type in a separate file to avoid this.
 
 
 Diffs
 -
 
   kded/CMakeLists.txt 910f5fa 
   kded/bluetoothdbustype.h PRE-CREATION 
   kded/bluetoothdbustype.cpp PRE-CREATION 
   kded/bluetoothmonitor.h 5f43369 
   kded/bluetoothmonitor.cpp 3aaf701 
   kded/dbus/org.freedesktop.DBus.ObjectManager.xml PRE-CREATION 
   kded/dbus/org.freedesktop.DBus.Properties.xml PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/121606/diff/
 
 
 Testing
 ---
 
 qdbus org.kde.plasmanetworkmanagement /org/kde/plasmanetworkmanagement 
 org.kde.plasmanetworkmanagement.addBluetoothConnection [macaddress] dun can 
 now create connection.
 
 
 Thanks,
 
 Xuetian Weng
 


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

Re: [Kde-hardware-devel] Review Request 121606: Port bluetooth connection support to bluez 5

[Lamarque Souza]

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



kded/bluetoothmonitor.h
https://git.reviewboard.kde.org/r/121606/#comment50535

Move this line to the class' private section.



kded/bluetoothmonitor.h
https://git.reviewboard.kde.org/r/121606/#comment50555

Code style: asterisk closer to variable name.



kded/bluetoothmonitor.h
https://git.reviewboard.kde.org/r/121606/#comment50556

Same here.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50557

I think this line can be removed now.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50536

Use camel case in oldowner and newowner.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50537

code style: asterisk is closer to the variable name, not the type.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50538

Same here.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50539

Code style: start comments with capital letter and end them with period.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50541

Code style: add one space between type and asterisk 
(QDBusPendingCallWatherspace*).



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50540

Code style:  closer to the variable name.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50542

Code style: space after if.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50543

Code style: asterisk closer to variable name.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50544

Same here.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50545

Comment with inicial capital letter and period at the end.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50547

Code style: comments starting with capital letter and period at the end.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50548

Code style: capital letter at the start and period at the end.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50549

Code style: asterisk closer to variable name.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50550

Code style.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50551

Code style.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50552

Code style.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50553

Code style.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50558

When I implemented support for bluetooth connection I remember Alex Fiestas 
asking me to override if there was a connection with the same bluetooth 
address. That was decided like that because Bluetooth only presents the dialog 
to activate network service for the phone right after the pairing process. If 
we do not override the connection here the user will have to delete the 
connection in Connection Managerm unpair de bluetooth device and pair it again. 
By overriding  we do not force the user to manually delete the connection.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50554

Code style: prepend new line here and move the  closer to the variable 
name.


- Lamarque Souza


On Dec. 25, 2014, 2:58 a.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121606/
 ---
 
 (Updated Dec. 25, 2014, 2:58 a.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 bluez 5 + networkmanager 1.0.0 suppose to support dun connection, bluedevil 
 for kf5 IMHO will support bluez 5. So this patch also drops bluez4 support 
 completely.
 
 nm 1.0.0's dun support doesn't require modemmanager AFAIK, so that part is 
 dropped in this patch. (Not quite sure if nap is supported or not)
 
 bluetoothdbustype.cpp is used because there's metatype conflict with 
 libnm-qt, so declare the type in a separate file to avoid this.
 
 
 Diffs
 -
 
   kded/bluetoothmonitor.cpp 3aaf701 
   kded/dbus/org.freedesktop.DBus.Properties.xml PRE-CREATION 
   kded/dbus/org.freedesktop.DBus.ObjectManager.xml PRE-CREATION 
   kded/bluetoothmonitor.h 5f43369 
   kded/bluetoothdbustype.cpp PRE-CREATION 
   kded/CMakeLists.txt 910f5fa 
   kded/bluetoothdbustype.h PRE-CREATION 
 
 Diff

Re: [Kde-hardware-devel] Review Request 121606: Port bluetooth connection support to bluez 5

[Lamarque Souza]

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



kded/bluetoothdbustype.h
https://git.reviewboard.kde.org/r/121606/#comment50518

You must add copyright and license header here and.



kded/bluetoothdbustype.cpp
https://git.reviewboard.kde.org/r/121606/#comment50523

Same here.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50524

This is wrong. QDBusPendingReply is async API, bluez.asyncCall() may not 
finish before reply.isError() is called below, that can lead to devices being 
ignored when they are available and working properly. Use QDBusReply instead. I 
did not like having to use sync API when I originally implemented support for 
bluetooth connections. If you can rewrite my code to use async API only I will 
really appreciate it.



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50525

const QString



kded/bluetoothmonitor.cpp
https://git.reviewboard.kde.org/r/121606/#comment50526

Code style: space after if


- Lamarque Souza


On Dec. 23, 2014, 4:26 p.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121606/
 ---
 
 (Updated Dec. 23, 2014, 4:26 p.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 bluez 5 + networkmanager 1.0.0 suppose to support dun connection, bluedevil 
 for kf5 IMHO will support bluez 5. So this patch also drops bluez4 support 
 completely.
 
 nm 1.0.0's dun support doesn't require modemmanager AFAIK, so that part is 
 dropped in this patch. (Not quite sure if nap is supported or not)
 
 bluetoothdbustype.cpp is used because there's metatype conflict with 
 libnm-qt, so declare the type in a separate file to avoid this.
 
 
 Diffs
 -
 
   kded/CMakeLists.txt 910f5fa 
   kded/bluetoothdbustype.h PRE-CREATION 
   kded/bluetoothdbustype.cpp PRE-CREATION 
   kded/bluetoothmonitor.h 5f43369 
   kded/bluetoothmonitor.cpp 3aaf701 
 
 Diff: https://git.reviewboard.kde.org/r/121606/diff/
 
 
 Testing
 ---
 
 qdbus org.kde.plasmanetworkmanagement /org/kde/plasmanetworkmanagement 
 org.kde.plasmanetworkmanagement.addBluetoothConnection [macaddress] dun can 
 now create connection.
 
 
 Thanks,
 
 Xuetian Weng
 


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

Re: [Kde-hardware-devel] Review Request 121606: Port bluetooth connection support to bluez 5

[Lamarque Souza]

 On Dec. 20, 2014, 8:25 a.m., Jan Grulich wrote:
  I definitely don't want to drop support for Bluez 4, it will take some time 
  to get distributions use NetworkManager 1.0.0 so your patch would break it 
  for those using older NM versions.
 
 Jan Grulich wrote:
 I mean we have to support older NM versions which requires also 
 ModemManager to make this work. Even libnm-qt supports older NM versions. 
 I'll take a look properly on your patch when I have more time.
 
 Xuetian Weng wrote:
 But think about it:
 1. network manager prior to 0.9.8 doesn't support bluez 5 dun connection. 
 (Fedora patches NM for 0.9.8)
 2. bluedevil in plasma 5 only works with bluez 5. (Though there's no 
 release yet, latest bluedevil 2.0 release can also only work with bluez 5).
 3. that function is only called from bluedevil user interface (which is 
 currently dropped in bluedevil because nm doesn't support it).
 
 Distribution who wants to use bluedevil in kf5 must use bluez 5.
 opensuse use bluez 5 since 13.1
 fedora use bluez 5 since fedora 20.
 arch is using bluez 5 since 2013.5.
 
 which means in those distribution, bluetooth dun doesn't work with NM at 
 all.
 
 The only distribution left is ubuntu, which might also use bluez 5 for 
 their next release http://www.phoronix.com/scan.php?page=news_itempx=MTgzNzM 
 .
 
 Is it possible to use old bluedevil that work with bluez 4 and bluez 4 on 
 a distribution? That means, bluedevil is not ported to kde5, and it still 
 uses kded4, the bluedevil kded module is not even loaded in non-kde4 
 environment. Why would they package plasma 5 with such old bluez then?
 
 Xuetian Weng wrote:
 Oh sorry, there's one small mistake: fedora's patch is for nm 0.9.10, 
 here's the report https://bugzilla.redhat.com/show_bug.cgi?id=1055628.
 
 Jan Grulich wrote:
 Add please Lukáš Tinkl and Lamarque Souza to reviewers.

Weng has a point. Only Bluedevil uses this code, if there is a Bluez 5 and 
Plasma 5 version of Bluedevil available then it makes sense to drop support for 
Bluez 4. Let's face it this code is a kind of horrible, it needs a rewrite 
even. It does not support more than one bluetooth controller either, luckily 
for us that is not a common setup :-) Plasma 5 is not mainstream yet, there 
will be time for distributions to add Bluez 5 to their package suites, if they 
have not done so yet.


- Lamarque


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


On Dec. 23, 2014, 4:26 p.m., Xuetian Weng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121606/
 ---
 
 (Updated Dec. 23, 2014, 4:26 p.m.)
 
 
 Review request for Solid, Jan Grulich, Lukáš Tinkl, and Lamarque Souza.
 
 
 Repository: plasma-nm
 
 
 Description
 ---
 
 bluez 5 + networkmanager 1.0.0 suppose to support dun connection, bluedevil 
 for kf5 IMHO will support bluez 5. So this patch also drops bluez4 support 
 completely.
 
 nm 1.0.0's dun support doesn't require modemmanager AFAIK, so that part is 
 dropped in this patch. (Not quite sure if nap is supported or not)
 
 bluetoothdbustype.cpp is used because there's metatype conflict with 
 libnm-qt, so declare the type in a separate file to avoid this.
 
 
 Diffs
 -
 
   kded/CMakeLists.txt 910f5fa 
   kded/bluetoothdbustype.h PRE-CREATION 
   kded/bluetoothdbustype.cpp PRE-CREATION 
   kded/bluetoothmonitor.h 5f43369 
   kded/bluetoothmonitor.cpp 3aaf701 
 
 Diff: https://git.reviewboard.kde.org/r/121606/diff/
 
 
 Testing
 ---
 
 qdbus org.kde.plasmanetworkmanagement /org/kde/plasmanetworkmanagement 
 org.kde.plasmanetworkmanagement.addBluetoothConnection [macaddress] dun can 
 now create connection.
 
 
 Thanks,
 
 Xuetian Weng
 


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

[Kde-hardware-devel] Fwd: Review Request 121217: Unbreak hal backend.

[Lamarque Souza]

Please read [1] to understand why knowing who (real name and valid e-mail)
sends patches to upstream is so important. Not that open source projects
are sued every year, but we need to know how to contact the person who
sends us patches when we need it in a formal way, not just through
nicknames.

[1] http://lwn.net/Articles/86436/

Lamarque V. Souza

KDE's Network Management maintainer

http://planetkde.org/pt-br

On Tue, Nov 25, 2014 at 3:30 AM, arrowdodger 6year...@gmail.com wrote:



 On Mon, Nov 24, 2014 at 10:00 PM, Albert Astals Cid aa...@kde.org wrote:

 El Dilluns, 24 de novembre de 2014, a les 21:51:03, arrowdodger va
 escriure:

 They are, but you need to tell them you want them to commit it, most of
 the
 times people have commit access.

 I could commit it for you, but i have a personal guideline of not
 commiting
 stuff with people that don't use their real names.

 Cheers,
   Albert


 Ok, my name is Gleb Popov.

 Honestly, i don't see much difference between real name or nick in the
 Internets. Googling for my nick, actually, would yield more information
 than googling for my name.

 Hope you would commit my patch now, thanks.

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


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