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

Discussion:

Jan Grulich

2016-09-06 08:53:42 UTC

Permalink

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

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

Jan Grulich

2016-09-06 11:59:58 UTC

Permalink

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128843/
-----------------------------------------------------------
(Updated ZÃ¡Å. 6, 2016, 11:56 dop.)
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

Lamarque Souza

2016-09-06 12:01:35 UTC

Permalink

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

Post by Jan Grulich
-----------------------------------------------------------
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

Jan Grulich

2016-09-06 12:17:21 UTC

Permalink

Every DeviceType inherits from Device, same as DeviceTypePrivate inherits from DevicePrivate so when DeviceType is connected to PropertiesChanged signal and doesn't implement the slot itself, then it will call the DevicePrivate::dbusPropertiesChanged() slot which is common for all devices. In this slot it calls normal propertiesChanged() slot which calls propertyChanged() implemented by DeviceTypePrivate. This somehow works given inheritance. Even previously the base Device class doesn't connect to PropertiesChanged signal.

- Jan

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

Jan Grulich

2016-09-06 13:00:54 UTC

Permalink

Post by Jan Grulich
Every DeviceType inherits from Device, same as DeviceTypePrivate inherits from DevicePrivate so when DeviceType is connected to PropertiesChanged signal and doesn't implement the slot itself, then it will call the DevicePrivate::dbusPropertiesChanged() slot which is common for all devices. In this slot it calls normal propertiesChanged() slot which calls propertyChanged() implemented by DeviceTypePrivate. This somehow works given inheritance. Even previously the base Device class doesn't connect to PropertiesChanged signal.

Issues discussed personally. And btw. QDBusConnection::connect() doesn't have an option for connection type as QObject::connect() so I cannot use Qt::UniqueConnection.

- Jan

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

Jan Grulich

2016-09-06 14:17:01 UTC

Permalink

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

(Updated Sept. 6, 2016, 4:17 p.m.)

Status
------

This change has been marked as submitted.

Review request for KDE Frameworks, Network Management and Lamarque Souza.

Changes
-------

Submitted with commit 7671424efb02e7194a18a4913da16bc2044a4cb9 by Jan Grulich to branch master.

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

Lamarque Souza

2016-09-06 14:07:55 UTC

Permalink

Post by Jan Grulich
-----------------------------------------------------------
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