D17210: Added proxy and user settings

2018-11-29 Thread Jan Grulich 
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

2018-11-29 Thread Jan Grulich 
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

2018-11-29 Thread Pranav Gade 
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=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

2018-11-29 Thread Jan Grulich 
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

2018-11-29 Thread Pranav Gade 
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

2018-11-29 Thread Christoph Feck 
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

2018-11-29 Thread Pranav Gade 
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

2018-11-29 Thread Pranav Gade 
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

2018-11-29 Thread Pranav Gade 
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=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

2018-11-29 Thread Jan Grulich 
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

2018-11-29 Thread Pranav Gade 
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=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

2018-11-29 Thread Jan Grulich 
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

2018-11-28 Thread Pranav Gade 
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=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

2018-11-28 Thread Pranav Gade 
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