Review Request 128093: Make network manager version checks in runtime (to avoid compile vs. run-time discrepancies)

Discussion:

Palo Kisa

2016-06-06 07:01:32 UTC

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

(Updated June 6, 2016, 7:01 a.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments (updated)
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Jan Grulich

2016-06-06 09:22:23 UTC

Permalink

src/manager.cpp, line 248
<https://git.reviewboard.kde.org/r/128093/diff/1/?file=467582#file467582line248>
Consider -1 and +1 *fits* if(compareVersion(...))
All -1 in this function must be 0 for patch to work.
Ex.
NetworkManager 1.0.2
if(compareVeresion(0, 9, 6)) {
this block will execute
}

You're right... I didn't check it well and assumed it returns bool. I need to create `checkVersion` which will only `return 0 <= checkVersion(x, y, z);` and use this in all the added checks.
Should I change the patch that way?

Yes please, also fix the coding style so it matches the rest of the library.

- Jan

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

-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated Äer. 6, 2016, 7:01 dop.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Jan Grulich

2016-06-06 09:38:10 UTC

Permalink

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

src/CMakeLists.txt (line 54)
<https://git.reviewboard.kde.org/r/128093/#comment65025>

I'm afraid that most of the new devices/settings won't compile against older NM versions as there are some NM macros used.

src/CMakeLists.txt (line 56)
<https://git.reviewboard.kde.org/r/128093/#comment65024>

WiMAX needs to stay there, otherwise we would break the ABI compatibility again.

src/accesspoint.cpp (line 222)
<https://git.reviewboard.kde.org/r/128093/#comment65026>

No need to check the version.

src/activeconnection.cpp (line 344)
<https://git.reviewboard.kde.org/r/128093/#comment65027>

This should check for older NM version than 0.9.10 and not newer or equal.

src/activeconnection.cpp (line 364)
<https://git.reviewboard.kde.org/r/128093/#comment65028>

I guess that this check is also not needed.

src/activeconnection.cpp (line 377)
<https://git.reviewboard.kde.org/r/128093/#comment65029>

Same here.

src/activeconnection.cpp (line 390)
<https://git.reviewboard.kde.org/r/128093/#comment65030>

Same here.

src/activeconnection.cpp (line 402)
<https://git.reviewboard.kde.org/r/128093/#comment65031>

Same here.

src/activeconnection.cpp (line 414)
<https://git.reviewboard.kde.org/r/128093/#comment65032>

Same here.

src/activeconnection.cpp (line 420)
<https://git.reviewboard.kde.org/r/128093/#comment65033>

Same here.

src/device.cpp (line 335)
<https://git.reviewboard.kde.org/r/128093/#comment65034>

No need for this check.

src/device.cpp (line 341)
<https://git.reviewboard.kde.org/r/128093/#comment65035>

Same here.

src/device.cpp (line 347)
<https://git.reviewboard.kde.org/r/128093/#comment65036>

Same here.

src/device.cpp (line 551)
<https://git.reviewboard.kde.org/r/128093/#comment65037>

Coding style.

- Jan Grulich

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated Äer. 6, 2016, 7:01 dop.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Jan Grulich

2016-06-06 12:31:00 UTC

Permalink

What about making NetworkManager 0.9.10 the minimum required version? It should be available everywhere these days,there is already NM 1.2.2 available, which means that 0.9.10 is 3 major releases back. It would reduce the number of checks for NM versions to half. What do you think Lamarque?

- Jan

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

Lamarque Souza

2016-06-06 13:03:28 UTC

Permalink

Post by Jan Grulich
What about making NetworkManager 0.9.10 the minimum required version? It should be available everywhere these days,there is already NM 1.2.2 available, which means that 0.9.10 is 3 major releases back. It would reduce the number of checks for NM versions to half. What do you think Lamarque?

I am Ok with bumping minimum required version to 0.9.10. Even Debian stable already ships with 0.9.10.

- Lamarque

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

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 6, 2016, 7:01 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Jan Grulich

2016-06-06 13:06:01 UTC

Permalink

Palo, could you please change your patch to address that? Just bump the required NM version in main CMakeLists.txt and remove all NM_CHECK_VERSION(0, 9, 10) checks.

- Jan

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

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated Äer. 6, 2016, 7:01 dop.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-06 15:56:06 UTC

Permalink

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

(Updated June 6, 2016, 3:56 p.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Palo Kisa

2016-06-06 16:00:11 UTC

Permalink

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

(Updated June 6, 2016, 4 p.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Lamarque Souza

2016-06-07 16:34:08 UTC

Permalink

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

src/device.cpp (line 125)
<https://git.reviewboard.kde.org/r/128093/#comment65045>

This should be checkVersion(1, 0, 6).

src/manager.h (line 122)
<https://git.reviewboard.kde.org/r/128093/#comment65046>

Use tag @deprecated here to indicate that.

src/manager.h (line 137)
<https://git.reviewboard.kde.org/r/128093/#comment65047>

Same here.

src/manager.h (line 224)
<https://git.reviewboard.kde.org/r/128093/#comment65048>

Please fix my spelling error :-)

s/othervise/otherwise/

src/manager.h (line 293)
<https://git.reviewboard.kde.org/r/128093/#comment65049>

@deprecated

src/manager.h (line 303)
<https://git.reviewboard.kde.org/r/128093/#comment65050>

@deprecated

src/manager.h (line 420)
<https://git.reviewboard.kde.org/r/128093/#comment65051>

Well, the correct way to do that is adding the Q_DECL_DEPRECATED modified after NETWORKMANAGER_QT_EXPORT macro. However, that will issue several compiler warnings during compilation, and some people my not like that, me doesn't. Maybe there is a way to suppress those warning messages using cmake.

src/manager.cpp (line 298)
<https://git.reviewboard.kde.org/r/128093/#comment65052>

This line and the next can be removed.

src/settings.cpp (line 143)
<https://git.reviewboard.kde.org/r/128093/#comment65053>

Shouldn't you call iface.ReloadConnections() here?

src/settings/gsmsetting.cpp (line 24)
<https://git.reviewboard.kde.org/r/128093/#comment65055>

This line looks wrong. There should be no negation in it.

src/settings/infinibandsetting.cpp (line 24)
<https://git.reviewboard.kde.org/r/128093/#comment65056>

Same here.

src/settings/teamsetting.cpp (line 25)
<https://git.reviewboard.kde.org/r/128093/#comment65057>

Here too.

src/settings/vlansetting.cpp (line 24)
<https://git.reviewboard.kde.org/r/128093/#comment65058>

Same here.

src/settings/wirelesssetting.cpp (line 24)
<https://git.reviewboard.kde.org/r/128093/#comment65059>

Same here.

src/vlandevice.cpp (line 83)
<https://git.reviewboard.kde.org/r/128093/#comment65054>

Use Q_NULLPTR instead of nullptr.

- Lamarque Souza

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 6, 2016, 4 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-07 22:39:40 UTC

Permalink

Post by Palo Kisa

src/manager.h, line 441
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467916#file467916line441>
Well, the correct way to do that is adding the Q_DECL_DEPRECATED modified after NETWORKMANAGER_QT_EXPORT macro. However, that will issue several compiler warnings during compilation, and some people my not like that, me doesn't. Maybe there is a way to suppress those warning messages using cmake.
src/settings.cpp, line 161
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467920#file467920line161>
Shouldn't you call iface.ReloadConnections() here?

Yes, I've left the wrong branch after the "0.9.10 check removal"....

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Why? Was the pre-existing logic wrong?

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 6, 2016, 4 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-07 23:28:37 UTC

Permalink

Post by Palo Kisa

By what I can see in other frameworks 5 using @deprecated plus a comment saying why it is deprecated is enough. You can add a line like this:

@deprecated Wimax support was removed from NetworkManager 1.2.

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Why? Was the pre-existing logic wrong?

Yes, that pre-existing logic was wrong.

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 7, 2016, 10:39 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-08 04:50:25 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Why? Was the pre-existing logic wrong?
Yes, that pre-existing logic was wrong.

Shouldn't that be fixed in separate commit rather than hidden in this unrelated one?

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 7, 2016, 10:39 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-08 12:10:18 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Why? Was the pre-existing logic wrong?
Yes, that pre-existing logic was wrong.
Shouldn't that be fixed in separate commit rather than hidden in this unrelated one?

Yes, it can be fixed in a separated patch.

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 7, 2016, 10:39 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-13 15:06:38 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

While changing the logic I've realized, that the version check negation seems to be correct -> consider this commit https://github.com/NetworkManager/NetworkManager/commit/d0b05b34d5f1ad8f52a15fed48cc2c02d2251145 . Are you sure, you want to change it?

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 13, 2016, 3:06 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Jan Grulich

2016-06-13 15:28:25 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

It's correct, there is no need to change it.

- Jan

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated Äer. 13, 2016, 3:06 odp.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-13 18:16:47 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Ok, the negation is right but I think we should do what the comment in https://github.com/NetworkManager/NetworkManager/commit/d0b05b34d5f1ad8f52a15fed48cc2c02d2251145 says and use the new headers instead of manually #defining the strings we need. So change these lines to:

#if NM_CHECK_VERSION(1, 0, 0)
#include <libnm/NetworkManager.h>
#else
#include <nm-setting-gsm.h>
#endif

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 13, 2016, 3:32 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-14 06:36:30 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

You are seeing it wrong:
1. We are not (re)defining anything that the NetworkManager provides on its own. We are just defining the deprecated&dropped properties, see e.g. this commit https://github.com/NetworkManager/NetworkManager/commit/a5ac95ca4bab34cfafd94e3f785bbe51e9b0b223
2. All src/setting/*setting.cpp are including src/setting/*setting.h -> src/setting/setting.h -> \<libnm/NetworkManager.h>

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 6:36 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-14 11:17:57 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Why? Was the pre-existing logic wrong?
Yes, that pre-existing logic was wrong.
Shouldn't that be fixed in separate commit rather than hidden in this unrelated one?
Yes, it can be fixed in a separated patch.
While changing the logic I've realized, that the version check negation seems to be correct -> consider this commit https://github.com/NetworkManager/NetworkManager/commit/d0b05b34d5f1ad8f52a15fed48cc2c02d2251145 . Are you sure, you want to change it?
It's correct, there is no need to change it.
#if NM_CHECK_VERSION(1, 0, 0)
#include <libnm/NetworkManager.h>
#else
#include <nm-setting-gsm.h>
#endif
1. We are not (re)defining anything that the NetworkManager provides on its own. We are just defining the deprecated&dropped properties, see e.g. this commit https://github.com/NetworkManager/NetworkManager/commit/a5ac95ca4bab34cfafd94e3f785bbe51e9b0b223
2. All src/setting/*setting.cpp are including src/setting/*setting.h -> src/setting/setting.h -> \<libnm/NetworkManager.h>

I see. Well, I prefer to avoid negations in if clauses as much as possible for readability. Can you change this to

#if NM_CHECK_VERSION(1, 0, 0)
#define...
#else
#include <nm-setting-gsm.h>
#endif

We should also use @deprecation tag and /** comment start in src/settings/gsmsetting.h instead of a regular comment so the deprecation information is included in documentation generated by doxygen. Yes, I know there is almost no doxygen documentation in that header. We will fix that in the future.

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 6:36 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-14 12:12:20 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 24
<https://git.reviewboard.kde.org/r/128093/diff/4/?file=467926#file467926line24>
This line looks wrong. There should be no negation in it.

Updated the @deprecated comments. Separated the defining deprecated macros in *setting.cpp files (i think it should not be coupled with the #include in common #if #else).

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 12:08 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-07 22:39:52 UTC

Permalink

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

(Updated June 7, 2016, 10:39 p.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Jan Grulich

2016-06-13 07:40:14 UTC

Permalink

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

src/CMakeLists.txt (line 87)
<https://git.reviewboard.kde.org/r/128093/#comment65122>

As I said before, this won't compile against older NM versions, because TunSettings uses some macros from NetworkManager header files.

src/device.cpp (line 334)
<https://git.reviewboard.kde.org/r/128093/#comment65124>

I assume that this check is not needed.

src/device.cpp (line 340)
<https://git.reviewboard.kde.org/r/128093/#comment65125>

Shouldn't be needed.

src/vlandevice.cpp (line 106)
<https://git.reviewboard.kde.org/r/128093/#comment65126>

This check is not needed.

- Jan Grulich

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated Äer. 7, 2016, 10:39 odp.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-13 15:06:28 UTC

Permalink

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

(Updated June 13, 2016, 3:06 p.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt 35d4282
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Palo Kisa

2016-06-13 15:32:32 UTC

Permalink

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

(Updated June 13, 2016, 3:32 p.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Palo Kisa

2016-06-14 06:36:24 UTC

Permalink

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

(Updated June 14, 2016, 6:36 a.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Palo Kisa

2016-06-14 12:08:07 UTC

Permalink

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

(Updated June 14, 2016, 12:08 p.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Lamarque Souza

2016-06-14 14:18:23 UTC

Permalink

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

src/settings/gsmsetting.cpp (line 28)
<https://git.reviewboard.kde.org/r/128093/#comment65172>

Hmmm you should used just of #if instead keeping the above one and creating this new one.

src/settings/teamsetting.cpp (line 29)
<https://git.reviewboard.kde.org/r/128093/#comment65174>

Same here.

src/settings/vlansetting.cpp (line 28)
<https://git.reviewboard.kde.org/r/128093/#comment65175>

Same here.

src/settings/wirelesssetting.cpp (line 28)
<https://git.reviewboard.kde.org/r/128093/#comment65176>

Same here.

- Lamarque Souza

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 12:08 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-14 14:26:43 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 28
<https://git.reviewboard.kde.org/r/128093/diff/9/?file=468707#file468707line28>
Hmmm you should used just of #if instead keeping the above one and creating this new one.

As I've said, IMO the "#include" #if is independent from the (newly added) "#define" #if (the same version is a coincidence) and I believe, it will be dropped independently in the future.

I also think, we are going to spend too much time on this... Do you realy think this is such a crucial part of this commit?

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 12:08 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Jan Grulich

2016-06-16 07:40:06 UTC

Permalink

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

I think it looks good now. Lamarque, have you spotted some other issue or is this review already good for you?

- Jan Grulich

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated Äer. 14, 2016, 12:08 odp.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-16 08:19:54 UTC

Permalink

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

src/device.cpp (line 536)
<https://git.reviewboard.kde.org/r/128093/#comment65226>

This is C++11 syntax, we should avoid it in Frameworks.

src/settings/connectionsettings.cpp (line 77)
<https://git.reviewboard.kde.org/r/128093/#comment65228>

This should be defined for NM < 1.2 too. The way it is now NM 1.0.x fails to compile.

src/settings/gsmsetting.cpp (line 28)
<https://git.reviewboard.kde.org/r/128093/#comment65229>

Yes, I do.

- Lamarque Souza

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 12:08 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-16 08:20:54 UTC

Permalink

Post by Palo Kisa

src/settings/connectionsettings.cpp, line 77
<https://git.reviewboard.kde.org/r/128093/diff/9/?file=468704#file468704line77>
This should be defined for NM < 1.2 too. The way it is now NM 1.0.x fails to compile.

I mean, NMQt fails to compile against NM 1.0.x.

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 12:08 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-16 08:38:52 UTC

Permalink

Post by Lamarque Souza

I mean, NMQt fails to compile against NM 1.0.x.

Will fix it instantly...

Post by Lamarque Souza

src/settings/gsmsetting.cpp, line 28
<https://git.reviewboard.kde.org/r/128093/diff/9/?file=468707#file468707line28>
Yes, I do.

I don't get this...

Post by Lamarque Souza

src/device.cpp, line 564
<https://git.reviewboard.kde.org/r/128093/diff/9/?file=468691#file468691line564>
This is C++11 syntax, we should avoid it in Frameworks.

Will be fixed instantly

- Palo

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

Post by Lamarque Souza
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 14, 2016, 12:08 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-16 08:48:00 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 28
<https://git.reviewboard.kde.org/r/128093/diff/9/?file=468707#file468707line28>
Yes, I do.

I don't get this...

I mean, you do what?

- Palo

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 16, 2016, 8:42 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-06-16 16:57:29 UTC

Permalink

Post by Palo Kisa

src/settings/gsmsetting.cpp, line 28
<https://git.reviewboard.kde.org/r/128093/diff/9/?file=468707#file468707line28>
Yes, I do.

I don't get this...
I mean, you do what?

Sorry, that was not supposed to go to this review request.

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 16, 2016, 8:42 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-16 08:42:05 UTC

Permalink

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

(Updated June 16, 2016, 8:42 a.m.)

Review request for Network Management, Jan Grulich and Lamarque Souza.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs (updated)
-----

CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

Lamarque Souza

2016-06-17 13:52:59 UTC

Permalink

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 16, 2016, 8:42 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-17 14:00:16 UTC

Permalink

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 16, 2016, 8:42 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-19 17:43:12 UTC

Permalink

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

Sorry, I don't know much about submitting/pushing (reviewed) patches to main repo... Should/can I somehow push to the NetworkManager-qt repository or are you in charge of that?

- Palo Kisa

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 16, 2016, 8:42 a.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Palo Kisa

2016-06-19 20:03:44 UTC

Permalink

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

(Updated June 19, 2016, 10:03 p.m.)

Status
------

This change has been marked as submitted.

Review request for Network Management, Jan Grulich and Lamarque Souza.

Changes
-------

Submitted with commit 751f79ea7b054c2fa7d5c1e7d996b70198e2dcc9 by Lamarque V. Souza on behalf of Palo Kisa to branch master.

Repository: networkmanager-qt

Description
-------

https://bugs.kde.org/show_bug.cgi?id=362736

Diffs
-----

CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606

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

Testing
-------

It's a relatively big change... so everything needs to be double checked.

File Attachments
----------------

0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch

Thanks,

Palo Kisa

David Faure

2016-07-02 08:06:01 UTC

Permalink

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

Could it be that this commit is the reason for the unittest failure in the CI? Please investigate.

https://build.kde.org/view/Frameworks%20kf5-qt5/job/networkmanager-qt%20master%20kf5-qt5/20/PLATFORM=Linux,compiler=gcc/testReport/%28root%29/TestSuite/managertest/

PASS : ManagerTest::testDevices()
FAIL! : ManagerTest::testManager() Compared values are not the same
Actual (NetworkManager::isWimaxEnabled()): 0
Expected (true) : 1
Loc: [/home/jenkins/sources/networkmanager-qt/kf5-qt5/autotests/managertest.cpp(119)]

- David Faure

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 19, 2016, 8:03 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-07-02 10:06:34 UTC

Permalink

Post by Palo Kisa

Post by David Faure
Could it be that this commit is the reason for the unittest failure in the CI? Please investigate.
https://build.kde.org/view/Frameworks%20kf5-qt5/job/networkmanager-qt%20master%20kf5-qt5/20/PLATFORM=Linux,compiler=gcc/testReport/%28root%29/TestSuite/managertest/
PASS : ManagerTest::testDevices()
FAIL! : ManagerTest::testManager() Compared values are not the same
Actual (NetworkManager::isWimaxEnabled()): 0
Expected (true) : 1
Loc: [/home/jenkins/sources/networkmanager-qt/kf5-qt5/autotests/managertest.cpp(119)]

I have found the problem. See the below messages.

- Lamarque

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

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 19, 2016, 8:03 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa

Lamarque Souza

2016-07-02 10:06:39 UTC

Permalink

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

src/manager.cpp (line 407)
<https://git.reviewboard.kde.org/r/128093/#comment65546>

This should be swapped:

return checkVersion(1, 2, 0) ? false : m_isWimaxEnabled;

src/manager.cpp (line 412)
<https://git.reviewboard.kde.org/r/128093/#comment65547>

Here too.

- Lamarque Souza

Post by Palo Kisa
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128093/
-----------------------------------------------------------
(Updated June 19, 2016, 8:03 p.m.)
Review request for Network Management, Jan Grulich and Lamarque Souza.
Repository: networkmanager-qt
Description
-------
https://bugs.kde.org/show_bug.cgi?id=362736
Diffs
-----
CMakeLists.txt bbd5868
src/CMakeLists.txt 0470d8f
src/accesspoint.h 51a1b2f
src/accesspoint.cpp 62cbc6c
src/accesspoint_p.h 521f8ce
src/activeconnection.h 7516ad6
src/activeconnection.cpp e833f51
src/activeconnection_p.h 79a0055
src/connection.h 5dd7a16
src/connection.cpp 57c5aa0
src/connection_p.h fb0b90b
src/device.h f465f78
src/device.cpp a3f6fad
src/device_p.h 4d7bcb6
src/ipconfig.h 24de6d2
src/ipconfig.cpp d1316fc
src/manager.h 0f36ba0
src/manager.cpp bd8752c
src/manager_p.h 8de789d
src/settings.h 76cdd8d
src/settings.cpp a15165f
src/settings/bondsetting.h 400ed11
src/settings/bondsetting.cpp 3d52611
src/settings/bridgesetting.cpp 05ce74c
src/settings/connectionsettings.h 2dd00af
src/settings/connectionsettings.cpp b62641e
src/settings/connectionsettings_p.h d337c99
src/settings/gsmsetting.h e8dc4ea
src/settings/gsmsetting.cpp b13274b
src/settings/infinibandsetting.h c0c854e
src/settings/infinibandsetting.cpp 6ced9b1
src/settings/infinibandsetting_p.h 238cd76
src/settings/setting.cpp ba75623
src/settings/teamsetting.h c11e1c6
src/settings/teamsetting.cpp 60ff218
src/settings/tunsetting.cpp ead153c
src/settings/vlansetting.h 244cef5
src/settings/vlansetting.cpp 5c39dc9
src/settings/wirelesssetting.h 4f84a77
src/settings/wirelesssetting.cpp aa143c4
src/settings_p.h aec5423
src/vlandevice.h cec84ea
src/vlandevice.cpp 029cab0
src/vlandevice_p.h 83b6b6f
src/wimaxdevice.cpp 468f50c
src/wirelessdevice.h 65313a7
src/wirelessdevice.cpp 9439606
Diff: https://git.reviewboard.kde.org/r/128093/diff/
Testing
-------
It's a relatively big change... so everything needs to be double checked.
File Attachments
----------------
0001-Make-the-networkmanager-version-checks-in-runtime.patch
https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/54f5a458-701f-4590-b9b4-21ea0c04f682__0001-Make-the-networkmanager-version-checks-in-runtime.patch
Thanks,
Palo Kisa