D17210: Added proxy and user settings
jgrulich closed this revision. jgrulich added a comment. Closed by https://cgit.kde.org/networkmanager-qt.git/commit/?id=ee74458db6593ed05b89c257de5789d02a0fb64d. REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
jgrulich accepted this revision. This revision is now accepted and ready to land. REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade updated this revision to Diff 46488. pranavgade marked 2 inline comments as done. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17210?vs=46469&id=46488 REVISION DETAIL https://phabricator.kde.org/D17210 AFFECTED FILES autotests/settings/CMakeLists.txt autotests/settings/proxysettingtest.cpp autotests/settings/proxysettingtest.h autotests/settings/usersettingtest.cpp autotests/settings/usersettingtest.h src/CMakeLists.txt src/settings/proxysetting.cpp src/settings/proxysetting.h src/settings/proxysetting_p.h src/settings/setting.cpp src/settings/setting.h src/settings/usersetting.cpp src/settings/usersetting.h src/settings/usersetting_p.h To: pranavgade, jgrulich Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
jgrulich added inline comments. INLINE COMMENTS > proxysetting.cpp:37 > +: name(NM_SETTING_PROXY_SETTING_NAME) > +, browserOnly(true) > +, method(ProxySetting::None) Default value is false. > usersetting.cpp:94 > +} > + > + One empty line is enough. REPOSITORY R282 NetworkManagerQt REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade set the repository for this revision to R282 NetworkManagerQt. REPOSITORY R282 NetworkManagerQt REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
cfeck added a comment. Could you please set the Repository field? REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade marked 3 inline comments as done. REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade marked 3 inline comments as done. pranavgade added inline comments. INLINE COMMENTS > jgrulich wrote in proxysetting.cpp:25 > It looks that the proxy setting has been introduced in NetworkManager 1.6. > This means that for all property defines, you have to add ifdef the same way > you did for ip tunnel setting. Fixed according to: https://developer.gnome.org/libnm/stable/NMSettingProxy.html#NM-SETTING-PROXY-METHOD:CAPS REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade updated this revision to Diff 46469. pranavgade added a comment. made some changes CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17210?vs=46467&id=46469 REVISION DETAIL https://phabricator.kde.org/D17210 AFFECTED FILES autotests/settings/CMakeLists.txt autotests/settings/proxysettingtest.cpp autotests/settings/proxysettingtest.h autotests/settings/usersettingtest.cpp autotests/settings/usersettingtest.h src/CMakeLists.txt src/settings/proxysetting.cpp src/settings/proxysetting.h src/settings/proxysetting_p.h src/settings/setting.cpp src/settings/setting.h src/settings/usersetting.cpp src/settings/usersetting.h src/settings/usersetting_p.h To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
jgrulich added inline comments. INLINE COMMENTS > iptunnelsetting.cpp:35 > , encapsulationLimit(0) > -, flags(IpTunnelSetting::Unknown) > +, flags(IpTunnelSetting::None) > , flowLabel(0) This is an unrelated change, submit it in a different review, but thanks for spotting this. > proxysetting.cpp:26 > > +#if NM_CHECK_VERSION(1, 16, 0) > +#define NM_SETTING_PROXY_SETTING_NAME"proxy" I said NM 1.6, not 1.16 and you need !NM_CHECK_VERSION(1, 6, 0). > proxysetting.h:42 > typedef QList List; > +enum Mode > +{ Coding style. It should be: enum Mode { } > usersetting.cpp:26 > > +#if NM_CHECK_VERSION(1, 8, 0) > +#define NM_SETTING_USER_SETTING_NAME"user" Same here. It should be !NM_CHECK_VERSION(1, 8, 0). REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade updated this revision to Diff 46467. pranavgade marked 6 inline comments as done. pranavgade added a comment. Updated code as required, fixed a minor error in iptunnelsettings CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17210?vs=46444&id=46467 REVISION DETAIL https://phabricator.kde.org/D17210 AFFECTED FILES src/settings/iptunnelsetting.cpp src/settings/proxysetting.cpp src/settings/proxysetting.h src/settings/proxysetting_p.h src/settings/usersetting.cpp src/settings/usersetting.h To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
jgrulich added inline comments. INLINE COMMENTS > proxysetting.cpp:25 > +#include > + > +NetworkManager::ProxySettingPrivate::ProxySettingPrivate() It looks that the proxy setting has been introduced in NetworkManager 1.6. This means that for all property defines, you have to add ifdef the same way you did for ip tunnel setting. > proxysetting.h:35 > +/** > + * Represents generic setting > + */ Represents proxy setting > proxysetting.h:52 > + > +void setMethod(quint32 method); > +quint32 method() const; This can be turned into an enum. Possible values are: NM_SETTING_PROXY_METHOD_NONE = 0, NM_SETTING_PROXY_METHOD_AUTO = 1, So I would do: enum Mode { None = NM_SETTING_PROXY_METHOD_NONE, auto = NM_SETTING_PROXY_METHOD_AUTO } > usersetting.cpp:25 > +#include > + > +NetworkManager::UserSettingPrivate::UserSettingPrivate() User setting was introduced with NM 1.8. Here should be ifdef to check the version and define NM_SETTING_USER_DATA in case the version is older. > usersetting.cpp:27 > +NetworkManager::UserSettingPrivate::UserSettingPrivate() > +: name(NM_SETTING_IP_TUNNEL_SETTING_NAME) > +{ } Should be NM_SETTING_USER_SETTING_NAME > usersetting.h:35 > +/** > + * Represents generic setting > + */ Represents user setting > usersetting.h:52 > +NMStringMap data() const; > + > + No reason for such a big space. REVISION DETAIL https://phabricator.kde.org/D17210 To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade updated this revision to Diff 46444. pranavgade added a comment. Updated diff from prev commit->changes to latest commit->changes CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17210?vs=46392&id=46444 REVISION DETAIL https://phabricator.kde.org/D17210 AFFECTED FILES autotests/settings/CMakeLists.txt autotests/settings/proxysettingtest.cpp autotests/settings/proxysettingtest.h autotests/settings/usersettingtest.cpp autotests/settings/usersettingtest.h src/CMakeLists.txt src/settings/proxysetting.cpp src/settings/proxysetting.h src/settings/proxysetting_p.h src/settings/setting.cpp src/settings/setting.h src/settings/usersetting.cpp src/settings/usersetting.h src/settings/usersetting_p.h To: pranavgade, jgrulich Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D17210: Added proxy and user settings
pranavgade created this revision. pranavgade added a reviewer: jgrulich. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. pranavgade requested review of this revision. REVISION SUMMARY Added proxy and user settings according to: https://developer.gnome.org/NetworkManager/stable/settings-proxy.html https://developer.gnome.org/NetworkManager/stable/settings-user.html REPOSITORY R282 NetworkManagerQt REVISION DETAIL https://phabricator.kde.org/D17210 AFFECTED FILES autotests/settings/CMakeLists.txt autotests/settings/proxysettingtest.cpp autotests/settings/proxysettingtest.h autotests/settings/usersettingtest.cpp autotests/settings/usersettingtest.h src/CMakeLists.txt src/settings/proxysetting.cpp src/settings/proxysetting.h src/settings/proxysetting_p.h src/settings/setting.cpp src/settings/setting.h src/settings/usersetting.cpp src/settings/usersetting.h src/settings/usersetting_p.h To: pranavgade, jgrulich Cc: kde-frameworks-devel, michaelh, ngraham, bruns