subject:"D15093\: Add WireGuard capability."

D15093: Add WireGuard capability.

2018-11-07 Thread Nathaniel Graham

ngraham added a comment.


  Thanks for the explanation! Just what I needed to feature this in 
https://pointieststick.wordpress.com/2018/11/07/this-week-in-usability-productivity-part-44
 (link will work starting this Sunday).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-07 Thread Bruce Anderson

andersonbruce added a comment.


  In D15093#355941 , @ngraham wrote:
  
  > Ah, thanks. And presumably shipping that networkmanager plugin would be up 
to distros, right?
  
  
  I assume this is the case.
  
  There are several NetworkManager VPN plugins that are handled this way where 
the code is not part of the official release but are referenced on the Gnome 
homepage for the project as being supported by third parties. Most if not all 
of these are already supported in the plasma-nm release and this just adds 
WireGuard to that group. I know that at least openSuse Tumbleweed supplies some 
of these third party plugins (although not WireGuard yet) so hopefully as 
WireGuard becomes more popular it will join this group as well.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-07 Thread Nathaniel Graham

ngraham added a comment.


  Ah, thanks. And presumably shipping that networkmanager plugin would be up to 
distros, right?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-07 Thread Bruce Anderson

andersonbruce added a comment.


  In D15093#355860 , @ngraham wrote:
  
  > Very cool! Does this actually implement full native WireGuard support, or 
only add support for the NetworkManager plugin available at 
https://github.com/max-moser/network-manager-wireguard?
  
  
  It only provides a front end for the NetworkManager plugin. This keeps the 
network handling all in NetworkManager but allows setup and display to be 
integrated into the KDE/Plasma applet.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-07 Thread Nathaniel Graham

ngraham added a comment.


  Very cool! Does this actually implement full native WireGuard support, or 
only add support for the NetworkManager plugin available at 
https://github.com/max-moser/network-manager-wireguard?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-07 Thread Jan Grulich

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R116:389a5e195bce: Add WireGuard capability. (authored by 
andersonbruce, committed by jgrulich).

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=45001=45010

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-06 Thread Bruce Anderson

andersonbruce added a comment.


  In D15093#354899 , @jgrulich wrote:
  
  > I still don't like the way how to get QPalette in the advanced dialog, can 
you please just simply construct it the same way you do it in the standard 
dialog?
  
  
  Fair enough. I changed it so the palettes are created in the advanced widget 
as well as the main interface.
  I fixed one of the coding style errors which I just missed last time. The 
other ones I will let you fix because I'm not sure what the correct format 
would be.
  
  Thanks for your patience with all my mistakes in this process.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-06 Thread Bruce Anderson

andersonbruce updated this revision to Diff 45001.
andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  - Allocate palettes in advanced widget rather than passing them from main 
interface

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=44198=45001

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-11-06 Thread Jan Grulich

jgrulich added a comment.


  I still don't like the way how to get QPalette in the advanced dialog, can 
you please just simply construct it the same way you do it in the standard 
dialog? Other than that it looks good and I think it's ready to go. Those 
mentioned coding style can be fixed later, I can go through that after it's 
merged.

INLINE COMMENTS

> wireguardadvancedwidget.cpp:48
> +
> +WireGuardAdvancedWidget::Private::Private() : fwMarkValid(true),
> +  presharedKeyValid(true),

Coding style, but can be fixed afterwards.

> wireguardwidget.cpp:52
> +
> +WireGuardSettingWidget::Private::Private(void) : addressValid(false),
> + privateKeyValid(false),

Coding style, can be fixed afterwards.

> wireguardwidget.cpp:75
> +KColorScheme::adjustBackground(d->warningPalette
> +   , KColorScheme::NegativeBackground
> +   , QPalette::Base

Coding style

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-25 Thread Bruce Anderson

andersonbruce marked 9 inline comments as done.
andersonbruce added inline comments.

INLINE COMMENTS

> jgrulich wrote in plasmanetworkmanagement_wireguardui.desktop:16
> Name could be just WireGuard I guess

I left this alone because it is consistent with the names of other VPNs.

> jgrulich wrote in wireguardadvancedwidget.cpp:87
> This validation doesn't seem to work, I can click on "ok" button even when 
> the advanced widget is empty. Also it's a bit weird  that the function name 
> suggests something else then it's doing. I would suggest splitting this into 
> something like "updatePresharedKeyValidation()" and to the one you have now. 
> One would really do just the check, the other would update the color.
> 
> Same please do for classic wireguard widget.

I did this slightly different from the suggestion but it only does the checks 
once per change and I think it is correctly enabling/disabling the "Ok" button. 
By the way, the "Ok" button is enabled when all entries on the Advanced widget 
are blank because all of the entries are optional and therefore all blank is 
perfectly valid.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-25 Thread Bruce Anderson

andersonbruce added a comment.


  A couple of notes on the latest upload:
  
  1. I left the background color change code alone. Doing searches on changing 
widget background color, it looks like the preferred method is to use 
setPalette() which requires a QPalette, and after trying a couple of things I 
decided it made more sense to me to pass around two palettes by reference than 
to pass just the colors and create palettes when they were needed.
  
  2. I backed out the change that used a blank setting rather than the stored 
values. I agree that it would be nice to not hang on to any extraneous elements 
if they happened to be created by some other application but what I found was 
that I had somehow not tested it after making this change and when I did, I 
found that it had broken the entire "Advanced" widget settings. The problem is 
that if you start with a blank setting and don't open the Advanced Widget then 
all the settings in that are lost. To fix this I would have to duplicate a 
bunch of functionality in both widgets which I don't like doing. Also, if the 
underlying NetworkManager app adds something I think that it is probably better 
to not delete it if plasma-nm hasn't been updated yet so I ended up deciding to 
put it back to the way it was.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-25 Thread Bruce Anderson

andersonbruce updated this revision to Diff 44198.
andersonbruce added a comment.


  - Address review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=42290=44198

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-18 Thread Jan Grulich

jgrulich added inline comments.

INLINE COMMENTS

> andersonbruce wrote in wireguardwidget.cpp:60
> My apologies if I sound a little frustrated on this but I spent more than 4 
> hours trying to follow the very unclear documentation on handling colors 
> before making this change. I thought the whole idea was that I was supposed 
> to use the scheme the user picked to tell what color to make the background 
> when it wasn't valid rather than assigning an arbitrary color.  KColorScheme 
> allows you to do this using the "NegativeBackground" role but there doesn't 
> appear to be any corresponding concept using QPalette. What would you suggest 
> that I use as the background for invalid entries? Or better yet. do you know 
> of an example of something that uses QPalette in a similar context since I 
> have been unable to find one?
> 
> As far as putting it in a class variable, I only did that so that I wouldn't 
> be creating them each time I wanted to change a background on one of the 
> widgets and to be able to pass them to the advanced widget and not have to 
> create them there as well. If you think that  palette creation is a 
> relatively time efficient process, I'll just create them on the stack in the 
> setBackground() function each time they are needed.

I'm sorry, I was wrong and I thought that KColorPalette is deprecated, but it's 
actually not and it's part of KConfigWidgets. Still, given you just use those 
two colors, you don't need to save full palette, just construct a one to get 
the colors and save these as member variables. You can do basically same for 
the advanced widget, you don't need to pass them in constructor.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-18 Thread Bruce Anderson

andersonbruce added inline comments.

INLINE COMMENTS

> jgrulich wrote in wireguardwidget.cpp:60
> Do not use KColorScheme. Use QPalette instead and you don't need to keeping 
> it as class variable.

My apologies if I sound a little frustrated on this but I spent more than 4 
hours trying to follow the very unclear documentation on handling colors before 
making this change. I thought the whole idea was that I was supposed to use the 
scheme the user picked to tell what color to make the background when it wasn't 
valid rather than assigning an arbitrary color.  KColorScheme allows you to do 
this using the "NegativeBackground" role but there doesn't appear to be any 
corresponding concept using QPalette. What would you suggest that I use as the 
background for invalid entries? Or better yet. do you know of an example of 
something that uses QPalette in a similar context since I have been unable to 
find one?

As far as putting it in a class variable, I only did that so that I wouldn't be 
creating them each time I wanted to change a background on one of the widgets 
and to be able to pass them to the advanced widget and not have to create them 
there as well. If you think that  palette creation is a relatively time 
efficient process, I'll just create them on the stack in the setBackground() 
function each time they are needed.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-17 Thread Jan Grulich

jgrulich added inline comments.

INLINE COMMENTS

> CMakeLists.txt:8
> +wireguardauth.cpp
> +wireguardadvancedwidget.cpp
> +)

Missing wireguardkeyvalidator.cpp

> CMakeLists.txt:29
> +KF5::KIOWidgets
> +KF5::CoreAddons
> +)

Linking against following ones is enough:

  plasmanm_internal
  plasmanm_editor
  KF5::ConfigCore
  KF5::CoreAddons
  KF5::I18n
  KF5::KIOWidgets
  KF5::WidgetsAddons

> plasmanetworkmanagement_wireguardui.desktop:16
> +X-KDE-PluginInfo-EnabledByDefault=false
> +Name=WireGuard based VPN
> +Comment=Compatible with WireGuard VPN servers

Name could be just WireGuard I guess

> wireguardadvancedwidget.cpp:41
> +WireGuardAdvancedWidget::WireGuardAdvancedWidget(const 
> NetworkManager::VpnSetting::Ptr 
> + , QPalette normalPalette
> + , QPalette warningPalette

Coding style

> wireguardadvancedwidget.cpp:58
> +connect(d->ui.buttonBox, ::rejected, this, 
> ::reject);
> +connect(d->ui.presharedKeyLineEdit
> +, ::textChanged

Coding style

> wireguardadvancedwidget.cpp:63
> +connect(d->ui.tableLineEdit
> +, ::textChanged
> +, this

Coding style

> wireguardadvancedwidget.cpp:87
> +}
> +isPresharedKeyValid();
> +}

This validation doesn't seem to work, I can click on "ok" button even when the 
advanced widget is empty. Also it's a bit weird  that the function name 
suggests something else then it's doing. I would suggest splitting this into 
something like "updatePresharedKeyValidation()" and to the one you have now. 
One would really do just the check, the other would update the color.

Same please do for classic wireguard widget.

> wireguardadvancedwidget.cpp:92
> +{
> +delete d->keyValidator;
> +delete d->listenPortValidator;

Perhaps you should write a destructor for the private widget and here just 
delete the d-pointer.

> wireguardadvancedwidget.h:42
> +explicit WireGuardAdvancedWidget(const NetworkManager::VpnSetting::Ptr 
> 
> + , QPalette normalPalette
> + , QPalette warningPalette

Coding style

> wireguardwidget.cpp:30
> +#include 
> +#include 
> +

KColorScheme has been removed/deprecated in KDE Frameworks and it's only in 
KDELibs4support which we don't want to use. You should use QPalette instead.

> wireguardwidget.cpp:39
> +NetworkManager::VpnSetting::Ptr setting;
> +KSharedConfigPtr config;
> +QPalette warningPalette;

You also will not need this, because QPalette already should have used colors 
thanks to our KDE platform theme.

> wireguardwidget.cpp:60
> +
> +KColorScheme::adjustBackground(d->warningPalette
> +   , KColorScheme::NegativeBackground

Do not use KColorScheme. Use QPalette instead and you don't need to keeping it 
as class variable.

> wireguardwidget.cpp:73
> +
> +connect(d->ui.addressIPv4LineEdit, ::textChanged, this, 
> ::slotWidgetChanged);
> +connect(d->ui.addressIPv4LineEdit, ::textChanged, this, 
> ::isAddressValid);

Split isFooValid() functions, as mentioned in the advanced widget. Also 
slotWidgetChanged() will call isValid() I think, which means that each 
isFooValid() is called twice.

> wireguardwidget.cpp:196
> +QPointer adv = new 
> WireGuardAdvancedWidget(d->setting
> +, 
> d->normalPalette
> +, 
> d->warningPalette

Coding style

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-15 Thread Bruce Anderson

andersonbruce added a comment.


  Well for the first time since July there has been activity on 
network-manager-wireguard including the addition of an additional parameter 
supported so I am going to get going on adding it to this. Also, it looks like 
the pre- and post- options now do something so against my better judgment I am 
going to add them back in as well.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-10-01 Thread Bruce Anderson

andersonbruce added a comment.


  I've uploaded everything I wanted to get done now. Are there any new 
comments? And by the way, what is the process once the all the comments do get 
cleared?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-25 Thread Nathaniel Graham

ngraham added a comment.


  In D15093#331505 , @andersonbruce 
wrote:
  
  > - Add entry widget background color change based on entry validity
  > - Change QSpinBoxes back to QLineEdits
  
  
  Awesome, +1 from me on these changes!

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-25 Thread Bruce Anderson

andersonbruce updated this revision to Diff 42290.
andersonbruce added a comment.


  - Add entry widget background color change based on entry validity
  - Change QSpinBoxes back to QLineEdits

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=42118=42290

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-24 Thread Jan Grulich

jgrulich added a comment.


  I agree with @ngraham, I was also planning a similar approach for other 
connection types.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-23 Thread Nathaniel Graham

ngraham added a comment.


  How about changing the outline or background of the text box to the Positive 
or Negative color from the active color theme? That way (for people with 
sensible color themes), the text box would show red if the value is invalid. I 
believe GNOME does something similar to this for their IP address validator, 
and in my experience it works quite nicely.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-21 Thread Bruce Anderson

andersonbruce added a comment.


  Okay, I just uploaded changes for most of the rest of the comments but I 
would like to revisit a couple of earlier issues.
  The first is the SpinBoxes. As I said in another comment, I don't think that 
the SpinBoxes are appropriate for any of the entries and would like to return 
these to be plain LineEdit widgets. The smallest range for any of these is the 
port number box at 0-65535 and I don't think any of them will be entered using 
the SpinBox arrows so I think simple text entry is better from an HMI 
perspective.
  
  Secondly I would like to reconsider entry validation. Currently it is using 
Pino's suggestion of using Validators on each entry to not allow incorrect keys 
to be entered and I think this is a good approach but I don't think it is 
sufficient. Right now there are 5 items checked for validity on the main screen 
(plus a couple on the Advanced settings widget) which can be checked for 
validity and when they are all valid, the "Save" button is enabled.
  
  These are obviously different in that one tests to see if the entry is 
completely valid while the other only checks to see if it can still be made to 
be complete and valid. My problem is that if there are five things that need to 
be checked and only one bit of information (whether the Save button is enabled 
or disabled) to indicate to the user that there is a problem and no indication 
of which element might be in error.
  
  I would like to add some type of indicator on each entry line to indicate 
whether that particular line is valid so if the user has entered: 
":0:1223:" for an IPv6 address (which is not valid) and all the other lines 
are valid, the user would be able to see immediately which line they need to 
look at to go forward. This was the idea behind my initial code which changed 
the background color of widgets to indicate validity. I realize that there are 
problems with this approach so I would like to come up with some other means to 
supply this information to the user. I've done three mockups of some 
possibilities: F6278509: Y-N_Lables.png 
  
  F6278513: EnabledCheckBox.png 
  
  F6278516: DisabledCheckBox.png 
  The difference between the last two is whether the CheckBoxes are enabled or 
not. I think that the enabled CheckBoxes is the best looking although it 
requires a little more programming because since it would be utilized as a 
strictly output widget I would have to intercept any clicks on the check box 
and ensure that it stayed in the correct state.
  
  Any comments would be appreciated.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-21 Thread Bruce Anderson

andersonbruce updated this revision to Diff 42118.
andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  - Update from more (but not all) review comments
  - Change 'setting' functions to use blank NMStringMap rather than incoming 
data.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=41874=42118

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-18 Thread Nathaniel Graham

ngraham added a comment.


  We generally try to operate on the principle of consensus here, so if there's 
a disagreement, we talk about it until someone changes their mind or we arrive 
at a compromise.FWIW, I would tend to agree that using a spinbox doesn't make 
sense when the range is enormous and there isn't much of a chance that it would 
ever make sense to use the arrows to adjust the value.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-18 Thread Bruce Anderson

andersonbruce added a comment.

  In D15093#328246 , @pino wrote:

  > In D15093#328241 , 
@andersonbruce wrote:
  >
  > > In D15093#327917 , @pino wrote:
  > >
  > > > note there are still few "not done" comments around (eg using QSpinBox 
for fwMark)
  > >
  > >
  > > Yes I know, I'll get to the spinbox in the next day or so. I've only been 
putting it off because I think it is such a bad idea and I hate making 
something I wrote worse rather than better but I know that I am playing in your 
house so I need to play by your rules. I'll hold my nose and get to it soon.
  >
  >
  > What you say sounds like you are understanding my notes as "imposing on 
you" "bad choices", which is definitely not the case. What I note are things 
based on my experience, and what I read from the patch.
  >  If I see a configuration key which is validated as positive integer, then 
my conclusion is that it makes sense to use the proper widget for it, i.e. 
QSpinBox (with proper range). If you think it is not correct, please do explain 
it! I could be missing anything, or simply knowing better about it is a good 
idea regardless.
  >
  > But please do not play the card of "you are the bad guy imposing things on 
me", thanks.

  I'm sorry, I am not trying to paint you as a bad guy here. What I meant to 
say is that I disagree with you that using a QSpinBox in these cases is a good 
idea. I just realize that  different projects have different design 
philosophies / standards and that since I am working on your project which I am 
a newcomer to, that I am going to follow your recommendations even if they 
disagree with my personal design philosophy because I realize it is important 
to be consistent across multiple projects of which this is only a tiny piece of 
just one project.

  In the case of the spinbox, I personally don't think that they should be used 
for anything with more than about 100 different steps because, by their nature, 
they tell the user to use the up and down arrows to change the value. Now you 
know and I know that you can just type in a value but not everyone does and on 
a value that can range from 0 to a very large number (in this case 20+) 
I believe typing in a value makes sense 99.99% of the time and that using a 
text box tells the user: "Type a number in here" better than a spinbox does. 
The same is true to a slightly lesser extent for the other two items which have 
been changed to spinboxes. Also since all of these will almost always not be 
used, again my personal opinion is that a blank text box conveys that better 
than sticking a "default" or "automatic" down at the low end of the spinbox 
range again because not everyone is going to know to type in 0 to get back to 
"default" if somehow it gets changed.

  It was late at night when your message about not forgetting about the spinbox 
(which I hadn't forgotten) came in and it sounded at the time a little like 
someone saying "I've told you this before, now you have to do this". Obviously 
this was not your intention and I apologize that I let my frustration get to me 
a little.

  Perhaps mistakenly, I have taken most of the 'design' related comments (as 
opposed to the 'implementation' issues) as being "this is the KDE way of doing 
things" and since that is a perfectly valid reason for doing things a 
particular way, I've just said "Okay, I'll do this even though I disagree that 
it is a good idea" and got a little frustrated about them.  Maybe I should have 
pushed back a little harder on this and other issues I disagreed with. Who 
knows. It's all water under the bridge now.  Again, my apologies.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-18 Thread Pino Toscano

pino added a comment.

  In D15093#328241 , @andersonbruce 
wrote:

  > In D15093#327917 , @pino wrote:
  >
  > > note there are still few "not done" comments around (eg using QSpinBox 
for fwMark)
  >
  >
  > Yes I know, I'll get to the spinbox in the next day or so. I've only been 
putting it off because I think it is such a bad idea and I hate making 
something I wrote worse rather than better but I know that I am playing in your 
house so I need to play by your rules. I'll hold my nose and get to it soon.

  What you say sounds like you are understanding my notes as "imposing on you" 
"bad choices", which is definitely not the case. What I note are things based 
on my experience, and what I read from the patch.
  If I see a configuration key which is validated as positive integer, then my 
conclusion is that it makes sense to use the proper widget for it, i.e. 
QSpinBox (with proper range). If you think it is not correct, please do explain 
it! I could be missing anything, or simply knowing better about it is a good 
idea regardless.

  But please do not play the card of "you are the bad guy imposing things on 
me", thanks.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-18 Thread Bruce Anderson

andersonbruce added a comment.


  In D15093#327917 , @pino wrote:
  
  > note there are still few "not done" comments around (eg using QSpinBox for 
fwMark)
  
  
  Yes I know, I'll get to the spinbox in the next day or so. I've only been 
putting it off because I think it is such a bad idea and I hate making 
something I wrote worse rather than better but I know that I am playing in your 
house so I need to play by your rules. I'll hold my nose and get to it soon.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-17 Thread Jan Grulich

jgrulich added a comment.


  In D15093#327889 , @andersonbruce 
wrote:
  
  > Since I added a validator function for the WireGuard style keys, is there 
any way to assign a validator to the PasswordField widget without a fairly 
substantial rewrite of that class?
  
  
  We don't do validation inside the passwordfield widget, this is done outside 
in widgets using it, you should do the same. Given it's a QWidget, you cannot 
directly assign a validator to it, but inside is QLineEdit which can have 
validator, you would have add a method to the main class which would just 
assign the validator to the QLineEdit widget inside, still please do the 
validation outside.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-17 Thread Pino Toscano

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  note there are still few "not done" comments around (eg using QSpinBox for 
fwMark)

INLINE COMMENTS

> CMakeLists.txt:28
>  Widgets
> +Test
>  )

already added by D15520: Upgrade SimpleIpV4AddressValidator and 
SimpleIpV6AddressValidator 

> wireguard.cpp:139
> +
> +WireGuardKeyValidator keyValidator(this);
> +int keyPos = 0;

no need to specify `this` as parent, since it is on stack, and thus it will be 
deleted automatically

> wireguard.cpp:139-140
> +
> +WireGuardKeyValidator keyValidator(this);
> +int keyPos = 0;
> +

indent

> wireguard.cpp:177
> +if (!value.isEmpty()) {
> +int pos = 0;
> +SimpleIpListValidator validator(this,

since earlier there is a `keyPos` variable used for basically the same purpose 
(i.e. passing it to the `validate()` of validators), just move `pos` earlier in 
the function, and use one everywhere

> wireguardadvancedwidget.cpp:26
> +
> +#include 
> +#include 

unused

> wireguardadvancedwidget.cpp:31
> +#include 
> +#include 
> +#include 

unused

> wireguardadvancedwidget.cpp:120
> +NMStringMap data;
> +long intVal;
> +QString stringVal;

a bit confusing to call `intVal` a variable which is (not correctly) a long; 
OTOH, it can be removed altogether (see below)

> wireguardadvancedwidget.cpp:121
> +long intVal;
> +QString stringVal;
> +

unused

> wireguardadvancedwidget.cpp:123-127
> +intVal = m_ui->listenPortSpinBox->value();
> +setOrClear(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), intVal);
> +
> +intVal = m_ui->mtuSpinBox->value();
> +setOrClear(data, QLatin1String(NM_WG_KEY_MTU), intVal);

just inline the calls to `value()`, just like done below

> wireguardadvancedwidget.h:27
> +#include 
> +#include 
> +

unused

> wireguardkeyvalidator.h:26
> +
> +class Q_DECL_EXPORT WireGuardKeyValidator : public QValidator
> +{

no need for `Q_DECL_EXPORT`, as it is in a plugin, and thus it does not need to 
be exported as public symbol

> pino wrote in wireguardwidget.cpp:182
> no need for the (...) for the whole condition:
> 
>   return foo || bar;
> 
> is easier than
> 
>   return (foo || bar);

this is not "done", btw

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-17 Thread Bruce Anderson

andersonbruce added a comment.


  Since I added a validator function for the WireGuard style keys, is there any 
way to assign a validator to the PasswordField widget without a fairly 
substantial rewrite of that class?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-17 Thread Bruce Anderson

andersonbruce updated this revision to Diff 41874.
andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  - Remove changes moved to review D15520 

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=41736=41874

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  CMakeLists.txt
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-17 Thread Jan Grulich

jgrulich added a comment.


  Please, update all your reviews so they don't duplicate changes. I would 
personally have one review for all your IPv[4,6]Validator changes and one 
review just for WireGuard VPN plugin.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-16 Thread Bruce Anderson

andersonbruce marked an inline comment as done.
andersonbruce added inline comments.

INLINE COMMENTS

> jgrulich wrote in wireguardadvancedwidget.cpp:104
> I would prefer having just an empty map with data where you just set 
> everything the user configured in UI, removing options from existing data map 
> might work, but if someone configure a connection somewhere else with options 
> we don't support, they will stay there as you will not remove them. Also 
> change the setOrClear() function to something like setProperty(const 
> NMStringMap , const QString , const QString ).

Functionally I think that the current implementation does this (although I can 
change the name of the function if you want). It starts with a blank 
NMStringMap and uses setOrClear on it. Are you possibly referring to the same 
function name  used in wireguardwidget.cpp rather than here in 
wireguardadvancedwidget.cpp?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-16 Thread Andrew Crouthamel

acrouthamel added a comment.


  Thank you @andersonbruce for all of your work on this, and quickly working on 
feedback changes! This will be a great addition.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-16 Thread Bruce Anderson

andersonbruce marked 45 inline comments as done.
andersonbruce added inline comments.

INLINE COMMENTS

> pino wrote in wireguard.cpp:121
> better use qt's foreach here:
> 
>   Q_FOREACH (const QString , valueList)
> 
> and then using `address` instead of `valueList[i]`

Used plain 'for' instead because Nathaniel Graham commented that the Q_FOREACH 
is not acceptable anymore.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-16 Thread Bruce Anderson

andersonbruce updated this revision to Diff 41736.
andersonbruce added a comment.


  - Add tests for new and updated ip validators
  - Update ip validators
  - Add validator for WireGuard keys
  - Update per review comments
  - Add error messages for failure cases

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=41451=41736

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  CMakeLists.txt
  libs/editor/CMakeLists.txt
  libs/editor/simpleiplistvalidator.cpp
  libs/editor/simpleiplistvalidator.h
  libs/editor/simpleipv4addressvalidator.cpp
  libs/editor/simpleipv4addressvalidator.h
  libs/editor/simpleipv6addressvalidator.cpp
  libs/editor/simpleipv6addressvalidator.h
  libs/models/kcmidentitymodel.cpp
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp
  tests/simpleipv4test.cpp
  tests/simpleipv6test.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-12 Thread Pino Toscano

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  In D15093#324457 , @andersonbruce 
wrote:
  
  > A couple of questions on opening other review requests:
  >
  > - Should I open a bug request on bugs.kde.org before opening the review?
  
  
  No need to, they are only internal refactorings/improvements.
  
  > - Is there any special way I need to mark dependencies? That is, the 
simpleiplistvalidator is dependent on the updates to the simpleipv4/6 
validators and WireGuard is dependent on both.
  
  I think so: see on the top-right "Edit Related Revisions" -> "Edit 
Parent/Child Revisions".
  
  > - Is there a preferred unit test framework that KDE uses?
  
  Qt ships its own QtTest -- see http://doc.qt.io/qt-5/qtest-overview.html
  
  > And do you possibly have a pointer to a sample of other projects that use 
it?
  
  One simple example is in juk, see the `tests` subdirectory.

INLINE COMMENTS

> simpleiplistvalidator.cpp:27-28
> +SimpleIpListValidator::SimpleIpListValidator(QObject *parent,
> +   enum AddressStyle 
> style,
> +   enum AddressType type)
> +: QValidator(parent)

this is not C, so 'enum' is not needed

> simpleiplistvalidator.cpp:31-34
> +addressStyle = style;
> +addressType = type;
> +ipv4Validator = nullptr;
> +ipv6Validator = nullptr;

these can be moved to the constructor initialization list, so it's slightly 
more efficient

> simpleiplistvalidator.cpp:42
> +ipv4Style = SimpleIpV4AddressValidator::AddressStyle::WithCidr;
> +ipv4Validator = new SimpleIpV4AddressValidator(nullptr, ipv4Style);
> +}

this is leaked -- either you pass 'this' as parent, or explicitly delete them 
in the class destructor

> simpleiplistvalidator.cpp:50
> +ipv6Style = SimpleIpV6AddressValidator::AddressStyle::WithCidr;
> +ipv6Validator = new SimpleIpV6AddressValidator(nullptr, ipv6Style);
> +}

ditto

> simpleiplistvalidator.cpp:61
> +// Split the incoming address on commas possibly with spaces on either 
> side
> +QStringList addressList = address.split(QRegExp("\\s*,\\s*"));
> +

a regexp is not needed here, just pass QString::SkipEmptyParts to split()

> simpleiplistvalidator.cpp:67-68
> +int i = 0;
> +QValidator::State ipv4Result = QValidator::Acceptable;
> +QValidator::State ipv6Result = QValidator::Acceptable;
> +QValidator::State result = QValidator::Acceptable;

these can be moved inside the foreach loop

> simpleiplistvalidator.cpp:78
> +if (ipv4Validator != nullptr)
> +ipv4Result = ipv4Validator->validate(const_cast(addr), 
> localPos);
> +else

this const_cast is not nice... rather keep a local copy of the string

> simpleiplistvalidator.cpp:85
> +if (ipv6Validator != nullptr)
> +ipv6Result = ipv6Validator->validate(const_cast(addr), 
> localPos);
> +else

ditto

> simpleiplistvalidator.cpp:91-92
> +if (ipv6Result == QValidator::Invalid && ipv4Result == 
> QValidator::Invalid) {
> +result = QValidator::Invalid;
> +break;
> +}

i think you can just `return QValidator::Invalid` straight away

> simpleiplistvalidator.h:42-45
> +AddressStyle addressStyle;
> +AddressType addressType;
> +SimpleIpV6AddressValidator *ipv6Validator;
> +SimpleIpV4AddressValidator *ipv4Validator;

private class variables start with "m_" prefix

> simpleipv4addressvalidator.cpp:26
>  
> -SimpleIpV4AddressValidator::SimpleIpV4AddressValidator(QObject *parent)
> +SimpleIpV4AddressValidator::SimpleIpV4AddressValidator(QObject *parent, enum 
> AddressStyle style)
>  : QValidator(parent)

as above, no "enum"

> simpleipv4addressvalidator.cpp:29
>  {
> +addressStyle = style;
>  }

in constructor initialization list

> simpleipv4addressvalidator.cpp:55-66
> +v = new QRegExpValidator(QRegExp(QLatin1String("[0-9, ]{1,3}\\.[0-9, 
> ]{1,3}\\.[0-9, ]{1,3}\\.[0-9, ]{1,3}")), 0);
> +break;
> +
> +case WithCidr:
> +v = new QRegExpValidator(QRegExp(QLatin1String("([0-9, 
> ]{1,3}\\.){3,3}[0-9, ]{1,3}/[0-9]{1,2}")), 0);
> +break;
> +

all the regexps and their validators are created every time, and this is going 
to be super slow; since the type is decided at constructor time and never 
changed, just create the validator once

(alos, this code leaks all the validators)

> simpleipv4addressvalidator.cpp:120
>  
> +
>  // replace input string with the corrected version

unneeded empty line change

> simpleipv4addressvalidator.cpp:137
> +} else {
> +value += QString::number(cidrValue);
> +return QValidator::Acceptable;

there is already `cidrParts[1]` as string, so no need to create it again from 
the value

>

D15093: Add WireGuard capability.

2018-09-12 Thread Bruce Anderson

andersonbruce added a comment.


  In D15093#323095 , @pino wrote:
  
  > Thanks Bruce for all the updates, and patience! In return, I have more 
notes :)
  >
  > - I'd send the changes to the existing SimpleIpV4AddressValidator in an own 
review request, since it affects other code than just this new wireguard plugin 
(maybe with unit tests)
  > - I'd send the addition of SimpleIpListValidator in an own review request 
too, since it is an independent code (possibly with unit tests)
  
  
  A couple of questions on opening other review requests:
  
  - Should I open a bug request on bugs.kde.org before opening the review?
  - Is there any special way I need to mark dependencies? That is, the 
simpleiplistvalidator is dependent on the updates to the simpleipv4/6 
validators and WireGuard is dependent on both.
  - Is there a preferred unit test framework that KDE uses? And do you possibly 
have a pointer to a sample of other projects that use it? (Especially if they 
use QValidator since I tried to write some test code for my own use and I 
couldn't get it to not crash before it even got to main()).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-12 Thread Bruce Anderson

andersonbruce updated this revision to Diff 41451.
andersonbruce added a comment.


  - Correct capitalization
  - Remove HTML from tooltip strings
  - Update some tooltip strings for clarity

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=41241=41451

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/editor/CMakeLists.txt
  libs/editor/simpleiplistvalidator.cpp
  libs/editor/simpleiplistvalidator.h
  libs/editor/simpleipv4addressvalidator.cpp
  libs/editor/simpleipv4addressvalidator.h
  libs/editor/simpleipv6addressvalidator.cpp
  libs/editor/simpleipv6addressvalidator.h
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-09 Thread Nathaniel Graham

ngraham added a comment.


  No new instances of `Q_FOREACH`, please. See 
https://www.kdab.com/goodbye-q_foreach/

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-09 Thread Pino Toscano

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  Thanks Bruce for all the updates, and patience! In return, I have more notes 
:)
  
  - I'd send the changes to the existing SimpleIpV4AddressValidator in an own 
review request, since it affects other code than just this new wireguard plugin 
(maybe with unit tests)
  - I'd send the addition of SimpleIpListValidator in an own review request 
too, since it is an independent code (possibly with unit tests)
  - something I forgot (sorry) in the past: `QRegExp` is the "old" class for 
regexps, while in new code `QRegularExpression` should be preferred/used; 
should be mostly a transparent change for your uses
  - I notes various nits for the UI strings: this is to follow the HIG, see 
https://hig.kde.org/style/writing/index.html in particular
  - if possible, please avoid HTML markup in strings in UI files -- they make 
translations slightly more complicated (e.g. more prone to break the string 
with a typo in the markup)

INLINE COMMENTS

> wireguard.cpp:119
> +valueList = interfaceGroup.readEntry(NMV_WG_TAG_ADDRESS, QStringList());
> +if (valueList.size() == 0)
> +return result;

isEmpty()

> wireguard.cpp:121
> +return result;
> +for (int i = 0; i < valueList.size(); i++) {
> +QPair addressIn = 
> QHostAddress::parseSubnet(valueList[i].trimmed());

better use qt's foreach here:

  Q_FOREACH (const QString , valueList)

and then using `address` instead of `valueList[i]`

> wireguard.cpp:122
> +for (int i = 0; i < valueList.size(); i++) {
> +QPair addressIn = 
> QHostAddress::parseSubnet(valueList[i].trimmed());
> +if (addressIn.first.protocol() == 
> QAbstractSocket::NetworkLayerProtocol::IPv4Protocol)

addressIn can be const

> wireguard.cpp:134-135
> +if (!value.isEmpty()) {
> +QRegExp validatorRegex("[0-9a-zA-Z\\+/]{43,43}=");
> +if (validatorRegex.exactMatch(value))
> +dataMap.insert(QLatin1String(NM_WG_KEY_PRIVATE_KEY), value);

the validation of a key is done multiple times, repeating the same regexp every 
time -- I'd be worth to create an own validator for it

> wireguard.cpp:159
> +int pos = 0;
> +SimpleIpListValidator *validator = new SimpleIpListValidator(this,
> + 
> SimpleIpListValidator::WithCidr,

"validator" is leaked here -- just use it on the stack (`SimpleIpListValidator 
validator;`)

> wireguard.cpp:174
> +// Listen Port
> +intValue = interfaceGroup.readEntry(NMV_WG_TAG_LISTEN_PORT, 0);
> +if (intValue > 0) {

`0` is `int` by default, so intValue must be int too (not quint32)

> wireguard.cpp:185
> +if (!value.isEmpty()) {
> +QHostAddress testAddress(value);
> +if (testAddress.protocol() == 
> QAbstractSocket::NetworkLayerProtocol::IPv4Protocol)

const

> wireguard.cpp:195
> +if (intValue > 0) {
> +if (intValue < 65536)
> +dataMap.insert(QLatin1String(NM_WG_KEY_LISTEN_PORT), 
> QString::number(intValue));

an MTU is not a port...

> wireguard.cpp:239
> +{
> +NMStringMap dataMap;
> +NetworkManager::VpnSetting::Ptr vpnSetting = 
> connection->setting(NetworkManager::Setting::Vpn).dynamicCast();

if you move the dataMap declaration after vpnSettings, you can glue declaration 
and initialization together

> wireguard.cpp:245
> +if (!(dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))
> +  || dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4)))
> +|| !dataMap.contains(QLatin1String(NM_WG_KEY_PRIVATE_KEY))

most probably this should be `NM_WG_KEY_ADDR_IP6` (not again IP4)

> wireguard.cpp:258-265
> +if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))) {
> +value = dataMap[NM_WG_KEY_ADDR_IP4];
> +if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP6)))
> +value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> +} else if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))) {
> +value = dataMap[NM_WG_KEY_ADDR_IP4];
> +}

just like readEntry, writeEntry can output lists -- just create a QStringList, 
and pass it to writeEntry

> wireguard.cpp:262-263
> +value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> +} else if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))) {
> +value = dataMap[NM_WG_KEY_ADDR_IP4];
> +}

typos for IP6?

> wireguard.ui:54
> +
> + Private Key:
> +

"Private key:"

> wireguard.ui:91
> +
> + Public Key:
> +

"Public key:"

> wireguardadvanced.ui:26
> +
> + Listen Port:
> +

"Listen port:"

> wireguardadvanced.ui:72-76
> +   
> +
> + htmlhead/bodypA 32-bit 
> fwmark for outgoing packets. If set to 0 or quot;offquot;, this 
> option is disabled. May be specified in hexadecimal by prepending 
> quot;0xquot;. Optional./p/body/html
> +
> +

D15093: Add WireGuard capability.

2018-09-09 Thread Bruce Anderson

andersonbruce marked 37 inline comments as done.
andersonbruce added a comment.


  I think I've gotten most of the fixes in. I made changes to 
simpleipv4addressvalidator and simpleipv6addressvalidator to add checking of 
addresses with CIDR and Port suffixes. Please check these carefully, I think I 
did the changes such that all existing uses will not be affected but I want to 
make sure. Oh, except I made one fix in ipv6 where it was disallowing "::" at 
the beginning of the address followed by anything. This is a totally valid and 
not uncommon usage so I included that in the change.
  I also added a new file simpleiplistvalidator which allows the checking of 
comma separated lists of IPv4, IPv6, or mixed addresses.
  
  I have not yet deleted the wireguardauthwidget because I haven't figured out 
what the 'askUser' function should return if it is not there.  There are also a 
couple of things I either didn't see before or forgot about that I will 
continue working on.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-09 Thread Bruce Anderson

andersonbruce updated this revision to Diff 41241.
andersonbruce added a comment.


  - Fixed comment
  - Expand capabilities of SimpleIP validators.
  - Changes per review comments
  - Update for updated ip validators

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=40885=41241

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/editor/CMakeLists.txt
  libs/editor/simpleiplistvalidator.cpp
  libs/editor/simpleiplistvalidator.h
  libs/editor/simpleipv4addressvalidator.cpp
  libs/editor/simpleipv4addressvalidator.h
  libs/editor/simpleipv6addressvalidator.cpp
  libs/editor/simpleipv6addressvalidator.h
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-05 Thread Bruce Anderson

andersonbruce added inline comments.

INLINE COMMENTS

> jgrulich wrote in wireguard.cpp:71
> You can use simpleipv[4,6]validator we have in plasma-nm instead of using 
> everything below. Or maybe QHostAddress can validate it for you?

The problem with using the simpleipv[4,6]validator is that WireGuard requires 
that in some cases the IP address can include a CIDR at the end (e.g. 
10.2.4.6/32) and in another case the entry needs to have a port number (e.g. 
10.2.3.5:7642) and the simpleip* functions won't handle either of these as 
currently written. QHostAddress will handle the CIDR but not the port number 
version. Also I'm not really thrilled with how QHostAddress does its 
verification. I know that it is technically correct but my personal opinion is 
that "1/32" should not be accepted as a valid IPv4 address which QHostAddress 
does.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-05 Thread Bruce Anderson

andersonbruce added a comment.


  In D15093#320599 , @jgrulich wrote:
  
  > I think you can completely remove WireguardAuth dialog if there is no use 
for it. I also spotted few trailing spaces in the patch, please remove them.
  
  
  The 'askUser' function currently returns the authWidget. Since it is pure 
virtual, I need to implement it somehow, so if I delete the authWidget, should 
it just return a nullptr instead?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-05 Thread Jan Grulich

jgrulich added a comment.

  In D15093#320672 , @andersonbruce 
wrote:

  > In D15093#320577 , @jgrulich 
wrote:
  >
  > >
  >
  >
  >
  >
  > > Does wg-quick support both, like simple commands and script files? If so, 
we should support both as well, if it supports only commands/snippets, we 
should leave it as it is.
  >
  > .
  >  Given this new information as well as the fact that there is a disconnect 
between what wg-quick wants and what the NM addon takes in, most notably, 
wg-quick specifically accepts multiple instances of each but the NM addon only 
allows one line of input.
  >
  > I would therefore propose that I remove all of the Pre/Post Up/Down entries 
for now since they won't do anything anyway and worry about adding them back in 
if the NM addon implements them properly and then match its implementation.

  Not sure this is a good plan either, problem is that once they are added, it 
will take for us some time to get them back and we will have to wait also for 
the next Plasma release, as you are not allowed to introduce new strings in 
stable releases.  I would keep them and hope that eventually they will get 
properly implemented by NM-Wireguard plugin. On the other hand, if those 
options are something not commonly used, it shouldn't bother anyone if we don't 
support them from the beginning. I'll leave this decision to you, I don't 
really have strong preference on this.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-05 Thread Bruce Anderson

andersonbruce marked an inline comment as done.
andersonbruce added a comment.

  In D15093#320577 , @jgrulich wrote:

  >

  > Does wg-quick support both, like simple commands and script files? If so, 
we should support both as well, if it supports only commands/snippets, we 
should leave it as it is.

  OK, it's more complicated than I thought.

  As a little background 'wg-quick' is a shell script that comes with the 
WireGuard kernel module and is provided to give a simple method of setting up 
basic connections. If you are trying to do something complex, I think you have 
to do it yourself with a combination of 'wg' (a lower level command that also 
come with the WireGuard package) and other ordinary networking commands. As far 
as I can tell all of this code is well done (even Linus "loved" it)
  The NetworkManager WireGuard addon on the other hand is not an official part 
of NetworkManager (even if it is linked to on their site) and they aren't doing 
any official upkeep on it so it is sort of you get what you get. Unfortunately 
that is somebody's Senior Thesis project. I want to make clear that i am not 
saying anything bad about the author, he is up front about the origins of the 
code and it certainly works well enough that I have been using it for a few 
months now and well enough that I wanted to take the time to write the 
plasma-nm code to go along with it.

  All that being said, the NM addon is not complete. I knew this before because 
it do didn't anything with the DNS field (even though it had an entry and it 
stored it) and I had to add it in myself because I needed it for my use case. 
Until tonight when I did some experimenting, however, I didn't know that the 
Pre/Post Up/Down entries are like that too where they are entered but not used 
when it calls wg-quick.

  Given this new information as well as the fact that there is a disconnect 
between what wg-quick wants and what the NM addon takes in, most notably, 
wg-quick specifically accepts multiple instances of each but the NM addon only 
allows one line of input.

  I would therefore propose that I remove all of the Pre/Post Up/Down entries 
for now since they won't do anything anyway and worry about adding them back in 
if the NM addon implements them properly and then match its implementation.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-05 Thread Jan Grulich

jgrulich added a comment.


  I think you can completely remove WireguardAuth dialog if there is no use for 
it. I also spotted few trailing spaces in the patch, please remove them.

INLINE COMMENTS

> wireguard.cpp:189
> +} else { // Error condition
> +return result;
> +}

You multiple times return just result, you should also set error message with 
the reason so users know why the import failed.

> wireguardadvancedwidget.cpp:104
> +
> +setOrClear(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), 
> m_ui->listenPortLineEdit->displayText());
> +setOrClear(data, QLatin1String(NM_WG_KEY_MTU), 
> m_ui->mTULineEdit->displayText());

I would prefer having just an empty map with data where you just set everything 
the user configured in UI, removing options from existing data map might work, 
but if someone configure a connection somewhere else with options we don't 
support, they will stay there as you will not remove them. Also change the 
setOrClear() function to something like setProperty(const NMStringMap , 
const QString , const QString ).

> wireguardwidget.cpp:31
> +
> +#include 
> +#include 

Not needed.

> wireguardwidget.cpp:129
> +{
> +// Currently WireGuard does not have any secrets
> +}

Add Q_UNUSED(setting);

> wireguardwidget.h:33
> +
> +class QUrl;
> +class QLineEdit;

QUrl and QLineEdit not needed.

> wireguardwidget.h:41
> +explicit WireGuardSettingWidget(const NetworkManager::VpnSetting::Ptr 
> ,
> +const struct WireGuardRegexStrings 
> ,
> +QWidget *parent = 0);

This paramater can be removed if you switch to using our IPv4 and IPv6 
validators, also you can completely remove wireguardstrings structure you 
defined. Even if anything like this would be needed, you basically need it in 
one class, why not defining it there? If our validators are not enought, then 
they should be improved so every class using them can benefit from that.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-05 Thread Jan Grulich

jgrulich added a comment.


  In D15093#319336 , @andersonbruce 
wrote:
  
  > In D15093#319253 , @pino wrote:
  >
  > > Much better now!
  > >
  > > - regarding the UI for all the pre/post scripts: since they are file 
paths, better use a KUrlRequester widget (limited to local existing files only, 
no URLs), so the users have a Browse button next to each line edit that can be 
used to open a file dialog
  >
  >
  > I debated with myself when I started this whether to include these at all. 
They are included in the base NetworkManager implementation which "inherited" 
them from the underlying wg-quick command but they duplicate functionality that 
NM provides directly and it seems to me that if someone is using NM then they 
can use those methods instead. Also, wg-quick specifies these as "script 
snippets" meaning actual direct commands that are executed by bash not 
necessarily a shell script. It also specifies that there can be multiple 
instances of each, a capability that the base NM implementation does not 
support. So my quandary is, do I implement this like the base NM does and 
possibly, as you suggest, force it to be a single shell script which sort of 
violates the spirit of the wg-quick command or do I delete it completely and 
not support something that base NM does, or do I leave it like it is?
  >
  > Personally I think that the base NM should get rid of these and force users 
to rely on the capability in NM to perform pre and post operations but given 
what exists, I don't think any of the alternatives are good and I'm not sure 
what the "least bad" solution is. If someone uses nm-connection-editor and 
enters something which is not a script and then opens the connection in a 
plasma-nm interface which only supports a file, I'm not sure what will happen. 
On the other hand if I delete the fields completely and open something created 
in nm-connection-editor with these fields, that's not good either.
  >
  > Since I initially was doing this only for my own use and was probably going 
to use NM for this, I admit that I took the easiest way out and duplicated what 
base NM has, which is a single string which can contain a shell script but also 
a snippet as the base WireGuard does and then said in the tool-tip that it was 
preferable to use NM capability instead.
  >
  > If you as a representative of the plasma-nm philosophy have a preference on 
which way to go or have a brilliant idea which solves all the problems, I will 
follow your lead.
  
  
  Does wg-quick support both, like simple commands and script files? If so, we 
should support both as well, if it supports only commands/snippets, we should 
leave it as it is.

INLINE COMMENTS

> plasmanetworkmanagement_wireguardui.desktop:17
> +Name=WireGuard
> +Comment=Handles WireGuard VPN connections

Maybe use better description, most of the plugins say "Compatible with Foo 
servers" so perhaps "Compatible with WireGuard VPN servers"

> wireguard.ui:46
> +
> + Address (IPv6)
> +

Use "Property name:" for each property, every label in plasma-nm use colon at 
the end of property name.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-03 Thread Pino Toscano

pino added a comment.

  In D15093#319336 , @andersonbruce 
wrote:

  > If you as a representative of the plasma-nm philosophy have a preference on 
which way to go or have a brilliant idea which solves all the problems, I will 
follow your lead.

  I'm not a plasma-nm developer, @jgrulich is :)
  Your explanation makes sense, thanks for taking the time to explain it. One 
thing I (don't) see is the configuration of pre/post scripts in plasma-nm for 
other types of connections (I can only check for wired, wireless, and openvpn). 
Maybe a possible idea is to leave them out for the first version, and implement 
them later if a) deemed appropriate for plasma-nm users b) solved their 
configuration mess as you described it.

INLINE COMMENTS

> andersonbruce wrote in wireguard.cpp:175-178
> The problem with using the KConfig method is if a space slips into the config 
> file before or after the comma then the spaces are left in one or the other 
> of the QStrings and I have to process each entry in the list to remove the 
> spaces. The files can come from elsewhere, e.g. I have some provided by my 
> VPN which have spaces in comma separated lists. Using split allows both 
> operations to be performed at the same time.

This is what `QString::trimmed()` does already. Considering you are passing the 
string directly to QHostAddress, it is just easy to write

  const QPair addressIn = 
QHostAddress::parseSubnet(addressList[i].trimmed());

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-03 Thread Bruce Anderson

andersonbruce added inline comments.

INLINE COMMENTS

> pino wrote in wireguard.cpp:175-178
> KConfig already supports comma-separated lists -- just pass QStringList() as 
> `default` value to readEntry(), so KConfigGroup knows the value is a list.

The problem with using the KConfig method is if a space slips into the config 
file before or after the comma then the spaces are left in one or the other of 
the QStrings and I have to process each entry in the list to remove the spaces. 
The files can come from elsewhere, e.g. I have some provided by my VPN which 
have spaces in comma separated lists. Using split allows both operations to be 
performed at the same time.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-03 Thread Bruce Anderson

andersonbruce added a comment.

  In D15093#319253 , @pino wrote:

  > Much better now!
  >
  > - regarding the UI for all the pre/post scripts: since they are file paths, 
better use a KUrlRequester widget (limited to local existing files only, no 
URLs), so the users have a Browse button next to each line edit that can be 
used to open a file dialog

  I debated with myself when I started this whether to include these at all. 
They are included in the base NetworkManager implementation which "inherited" 
them from the underlying wg-quick command but they duplicate functionality that 
NM provides directly and it seems to me that if someone is using NM then they 
can use those methods instead. Also, wg-quick specifies these as "script 
snippets" meaning actual direct commands that are executed by bash not 
necessarily a shell script. It also specifies that there can be multiple 
instances of each, a capability that the base NM implementation does not 
support. So my quandary is, do I implement this like the base NM does and 
possibly, as you suggest, force it to be a single shell script which sort of 
violates the spirit of the wg-quick command or do I delete it completely and 
not support something that base NM does, or do I leave it like it is?

  Personally I think that the base NM should get rid of these and force users 
to rely on the capability in NM to perform pre and post operations but given 
what exists, I don't think any of the alternatives are good and I'm not sure 
what the "least bad" solution is. If someone uses nm-connection-editor and 
enters something which is not a script and then opens the connection in a 
plasma-nm interface which only supports a file, I'm not sure what will happen. 
On the other hand if I delete the fields completely and open something created 
in nm-connection-editor with these fields, that's not good either.

  Since I initially was doing this only for my own use and was probably going 
to use NM for this, I admit that I took the easiest way out and duplicated what 
base NM has, which is a single string which can contain a shell script but also 
a snippet as the base WireGuard does and then said in the tool-tip that it was 
preferable to use NM capability instead.

  If you as a representative of the plasma-nm philosophy have a preference on 
which way to go or have a brilliant idea which solves all the problems, I will 
follow your lead.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-03 Thread Jan Grulich

jgrulich added inline comments.

INLINE COMMENTS

> wireguard.cpp:71
> +{
> +regexStrings.ip4Range = new QString(
> +"(?:[0-1]?[0-9]?[0-9]|2[0-4][0-9]|25[0-5])");

You can use simpleipv[4,6]validator we have in plasma-nm instead of using 
everything below. Or maybe QHostAddress can validate it for you?

> wireguard.cpp:185
> +}
> +else if (addressIn.first.protocol() == 
> QAbstractSocket::NetworkLayerProtocol::IPv6Protocol) {
> +dataMap.insert(QLatin1String(NM_WG_KEY_ADDR_IP6), 
> addressList[i]);

Coding style.

> wireguard.cpp:215
> +}
> +else {
> +return result;

Coding style.

> wireguard.cpp:318
> +if (!haveAddress || !havePrivateKey || !havePublicKey || 
> !haveAllowedIps) {
> +
> +mError = VpnUiPlugin::Error;

Remove space.

> wireguard.h:34
> +public:
> +explicit WireGuardUiPlugin(QObject* parent = nullptr, const 
> QVariantList& = QVariantList());
> +~WireGuardUiPlugin() override;

Coding style. You mix funcName(Bar* foo) with funcName(Bar * foo) and 
funcName(Bar *foo), plese change it all to the last one. Same goes for 
functions below.

> wireguardadvancedwidget.h:55
> +
> +private Q_SLOTS:
> +

Can be removed if you don't have any private slot.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-03 Thread Pino Toscano

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  Much better now!
  
  General notes:
  
  - there are various checks on lengths of strings like `str.length() > 0` or 
`str.length() != 0`: if all you need is check whether a string is empty or not, 
just use `str.isEmpty()`
  - regarding the UI for all the pre/post scripts: since they are file paths, 
better use a KUrlRequester widget (limited to local existing files only, no 
URLs), so the users have a Browse button next to each line edit that can be 
used to open a file dialog

INLINE COMMENTS

> nm-wireguard-service.h:2
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> +/* nm-openvpn-service - openvpn integration with NetworkManager
> + *

this comment needs to be fixed

> wireguard.cpp:25
> +#include 
> +#include 
> +#include 

unused

> wireguard.cpp:29-30
> +#include 
> +#include 
> +#include 
> +#include 

both unused

> wireguard.cpp:41
> +
> +#include 
> +

unused

> wireguard.cpp:60
> +#define NMV_WG_TAG_FWMARK"FwMark"
> +#define NMV_WG_ASSIGN"="
> +

unused

> wireguard.cpp:151-153
> +KConfig importFile(fileName, KConfig::NoGlobals);
> +KConfigGroup interfaceGroup = importFile.group(NMV_WG_TAG_INTERFACE);
> +KConfigGroup peerGroup = importFile.group(NMV_WG_TAG_PEER);;

make all 3 as `const`, so it is clear they are read-only (and attempts to use 
writeEntry() will result in build failures)

> wireguard.cpp:153
> +KConfigGroup interfaceGroup = importFile.group(NMV_WG_TAG_INTERFACE);
> +KConfigGroup peerGroup = importFile.group(NMV_WG_TAG_PEER);;
> +

extra ';'

> wireguard.cpp:166-172
> +// The config file must have both [Interface] and [Peer] sections
> +if (!importFile.groupList().contains("Interface")
> +|| !importFile.groupList().contains("Peer")) {
> +mError = VpnUiPlugin::Error;
> +mErrorMessage = i18n("Could not open file");
> +return result;
> +}

These checks can be moved right after reading the config file and getting the 
KConfigGroup objects.

> wireguard.cpp:167-168
> +// The config file must have both [Interface] and [Peer] sections
> +if (!importFile.groupList().contains("Interface")
> +|| !importFile.groupList().contains("Peer")) {
> +mError = VpnUiPlugin::Error;

You do not need to get the list of groups in the config file (twice): earlier 
you get the KConfigGroup objects for the groups, so checking eg 
`interfaceGroup.exists()` should do the job.

> wireguard.cpp:175-178
> +value = interfaceGroup.readEntry(NMV_WG_TAG_ADDRESS);
> +{
> +QStringList addressList;
> +addressList << value.split(QRegExp("\\s*,\\s*"));

KConfig already supports comma-separated lists -- just pass QStringList() as 
`default` value to readEntry(), so KConfigGroup knows the value is a list.

> wireguard.cpp:195
> +// Listen Port
> +value = interfaceGroup.readEntry(NMV_WG_TAG_LISTEN_PORT);
> +if (value.length() > 0) {

If you specify `0` as default parameter, KConfigGroup will try to decode the 
value as integer automatically. If `0` is a valid value for the port, then use 
`-1`.
After doing that, you do not need a validator anymore, just a manual range 
check will do the job.

> wireguard.cpp:209
> +QRegExp validatorRegex(*regexStrings.keySpec);
> +QRegExpValidator validator(validatorRegex);
> +int pos = 0;

this validator is not needed, just use `validatorRegex` directly

> wireguard.cpp:231
> +// MTU
> +value = interfaceGroup.readEntry(NMV_WG_TAG_MTU);
> +if (value.length() > 0) {

same as for the listen port: please read the value directly as integer.

> wireguard.cpp:271
> +QRegExp validatorRegex(*regexStrings.keySpec);
> +QRegExpValidator validator(validatorRegex);
> +int pos = 0;

as above, no need for a validator, just use the QRegExp directly

> wireguard.cpp:288
> + + ", *)*" + *regexStrings.ip4Orip6Address);
> +QRegExpValidator *validator = new QRegExpValidator(allowedIPsRegex, 
> this);
> +if (validator->validate(value, pos) != QValidator::State::Invalid) {

as above, no need for a validator, just use the QRegExp directly

> wireguard.cpp:306
> +QRegExp validatorRegex(*regexStrings.keySpec);
> +QRegExpValidator validator(validatorRegex);
> +int pos = 0;

as above, no need for a validator, just use the QRegExp directly

> wireguard.cpp:350-353
> +value = dataMap[NM_WG_KEY_ADDR_IP4];
> +if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP6))) {
> +value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> +}

as mentioned above: KConfigGroup supports lists

> wireguard.cpp:361-365
> +// Do Private Key
> +if (dataMap.contains(QLatin1String(NM_WG_KEY_PRIVATE_KEY)))
> +interfaceGroup.writeEntry(NMV_WG_TAG_PRIVATE_KEY, 
>

D15093: Add WireGuard capability.

2018-09-02 Thread Andrew Crouthamel

acrouthamel added a comment.


  In D15093#319245 , @andersonbruce 
wrote:
  
  > I'm not sure if the author or the reviewer is supposed to check the "Done" 
box on the inline comments but I think that I have addressed  all the various 
comments made, both inline and separately.
  
  
  The author can. :)

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-02 Thread Bruce Anderson

andersonbruce added a comment.


  I'm not sure if the author or the reviewer is supposed to check the "Done" 
box on the inline comments but I think that I have addressed  all the various 
comments made, both inline and separately.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-09-02 Thread Bruce Anderson

andersonbruce updated this revision to Diff 40885.
andersonbruce added a comment.


  Updates from review comments
  
  - Add copyright notices
  - Delete unneeded files
  - Formatting changed to agree with coding standards
  - Changed validation method removing color overrides
  - Removed hard coded fonts and sizes from UI files

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15093?vs=40489=40885

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardstrings.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-30 Thread Pino Toscano

pino added a comment.


  In D15093#317960 , @andersonbruce 
wrote:
  
  > I've used KDE for years but this is the first time I've written code using 
Qt so it doesn't surprise me that I didn't use some of the preferred methods of 
doing things. I have a few questions below and hopefully you will have a little 
patience with me if any seem like stupid questions.
  >
  > In D15093#315890 , @pino wrote:
  >
  > > Misc notes:
  > >
  > > - please remove all the translations (i.e. `Name=[lang]` & 
`Comment[lang]` from desktop/json files, since KDE has an automatic system to 
handle them
  >
  >
  > Do the automatic translations get added into the desktop files themselves 
at some point of the build process?
  
  
  There is an automatic system that extracts translations from desktop files 
(and alike), and injects translations back.
  
  >> - as @lbeltrame said, hardcoding colors is a bad idea, and it will not 
play nice with user choice of different color schemes, or accessibility purposes
  > 
  > What I am trying to do here is to highlight fields that don't have valid 
entries in them. I did this by turning the background in the field red if it 
wasn't valid and returning it to default when it became valid. Is there a "KDE 
way" to do something like this or should I just leave everything with the 
default background and not give the user any immediate feedback that there is a 
problem?
  
  I mentioned it one point below:
  
  >> - a better option to validate an input in a line edit is to use the 
embedded validator options, see `QLineEdit::setInputMask` and `QValidator`
  
  IOW, instead of validating the input //after// the user inputs it, do it 
//before//.
  
  >> - the better option to edit a port number is `QSpinBox` restricted to 
0-65536, instead of a line edit
  > 
  > For the one entry which specifies only a port number, the most common 
instance is to leave this field empty. In my quick read through of the QSpinBox 
docs I didn't see any way to do a mixed 'no entry'/number option so I will 
probably leave this as a line edit.
  
  IMHO having 0 as value for that is good; otherwise, a proper QIntValidator 
for the line edit restricts the input the user can insert, removing the need to 
do the validation manually.
  
  >> - there is a mixture of `virtual` & `Q_DECL_OVERRIDE` for virtual methods; 
plasma-nm uses `override` exclusively
  > 
  > Can you please give explicit references to where the problem is?
  
  `wireguardwidget.h`, for example.
  
  I am not familiar with how Q_DECL_OVERRIDE is used but I did look at how all 
the other VPN implementations did it and it looks like they use the exact same 
mix of `virtual` & `Q_DECL_OVERRIDE` as I used in WireGuard.
  
  See commit 111ac65ae79f1a777e3b4a6389e916f0dfccd35e 
.  
It looks like you are developing against an old version of plasma-nm, or not on 
master anyway.
  
  >> - please do not hardcode sizes and fonts in UI files
  > 
  > I can understand that fonts shouldn't be specified and have removed them. 
What I was trying to do was to highlight the text in a couple of labels by 
making them a little bigger than the default font and making them bold. Is 
there a "KDE way" to highlight something like this? Maybe a way to say "this is 
a header" or similar?
  
  Rather than "how can I make some text bigger", the question is "what do you 
need to do". Since you are grouping widgets, then use a QGroupBox.
  
  Oh, and that adds another thing to fix: `wireguard.ui` really needs a 
top-level layout, instead of hardcoding the size of the widgets...
  
  >> - there are various texts in UI files with double spaces between words, 
please simply them to a single space
  >> - manually calling `KAcceleratorManager::manage(this);` is not needed, why 
were they added?
  > 
  > Again this is due to my inexperience with Qt. Is there some reason that 
WireGuard doesn't need this but all the other VPN implementations do?
  
  This was added by dbb4e2d5d47a6546014d433a1533d4ef4cfb7137 
. 
Weird choice, I guess this can be left as it is.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-30 Thread Andrew Crouthamel

acrouthamel added a comment.


  Awesome feature, thank you for working on this!

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-30 Thread Bruce Anderson

andersonbruce added a comment.

  I've used KDE for years but this is the first time I've written code using Qt 
so it doesn't surprise me that I didn't use some of the preferred methods of 
doing things. I have a few questions below and hopefully you will have a little 
patience with me if any seem like stupid questions.

  In D15093#315890 , @pino wrote:

  > Misc notes:
  >
  > - please remove all the translations (i.e. `Name=[lang]` & `Comment[lang]` 
from desktop/json files, since KDE has an automatic system to handle them

  Do the automatic translations get added into the desktop files themselves at 
some point of the build process? If not, why do all the rest of the VPN 
implementations have translations in them?

  > - please use KConfig 
 to read & write 
ini-like files, instead of doing everything manually
  > - the `PasswordField` class is already available as 
`libs/editor/widgets/passwordfield.h`, part of the `plasmanm_editor` library, 
so you do not need to copy it locally
  > - `wireguard_global.h` seems not used, so just drop it
  > - `nm-wireguard-service.h` has lots of `NM_OPENVPN_` keys that are not used
  > - as @lbeltrame said, please use `QHostAddress` to parse IP addresses
  > - as @lbeltrame said, hardcoding colors is a bad idea, and it will not play 
nice with user choice of different color schemes, or accessibility purposes

  What I am trying to do here is to highlight fields that don't have valid 
entries in them. I did this by turning the background in the field red if it 
wasn't valid and returning it to default when it became valid. Is there a "KDE 
way" to do something like this or should I just leave everything with the 
default background and not give the user any immediate feedback that there is a 
problem?

  > - a better option to validate an input in a line edit is to use the 
embedded validator options, see `QLineEdit::setInputMask` and `QValidator`
  > - the better option to edit a port number is `QSpinBox` restricted to 
0-65536, instead of a line edit

  For the one entry which specifies only a port number, the most common 
instance is to leave this field empty. In my quick read through of the QSpinBox 
docs I didn't see any way to do a mixed 'no entry'/number option so I will 
probably leave this as a line edit.

  > - there is a mixture of `virtual` & `Q_DECL_OVERRIDE` for virtual methods; 
plasma-nm uses `override` exclusively

  Can you please give explicit references to where the problem is? I am not 
familiar with how Q_DECL_OVERRIDE is used but I did look at how all the other 
VPN implementations did it and it looks like they use the exact same mix of 
`virtual` & `Q_DECL_OVERRIDE` as I used in WireGuard.

  > - please do not hardcode sizes and fonts in UI files

  I can understand that fonts shouldn't be specified and have removed them. 
What I was trying to do was to highlight the text in a couple of labels by 
making them a little bigger than the default font and making them bold. Is 
there a "KDE way" to highlight something like this? Maybe a way to say "this is 
a header" or similar?

  > - there are various texts in UI files with double spaces between words, 
please simply them to a single space
  > - manually calling `KAcceleratorManager::manage(this);` is not needed, why 
were they added?

  Again this is due to my inexperience with Qt. Is there some reason that 
WireGuard doesn't need this but all the other VPN implementations do?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D15093: Add WireGuard capability.

2018-08-27 Thread Anthony Fieroni

anthonyfieroni added a comment.


  When you use background-color you combine with QPalette, at least, because 
you break all other themes.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-27 Thread Jan Grulich

jgrulich added a comment.


  That's a big change/patch, I'll look more closely as soon as possible. One 
thing I see at first look is different coding style, can you please follow Qt 
coding style? I'm sure that many parts of plasma-nm are not using consistent 
coding style, but you should try to follow this one 
https://wiki.qt.io/Qt_Coding_Style.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-27 Thread Pino Toscano

pino added a comment.


  In D15093#315885 , @andersonbruce 
wrote:
  
  > As far as the 'passwordfield' files, this was just laziness on my part, it 
being easier to just copy them than figure out how to get cmake (another tool I 
haven't used much) to add the correct include path to the flags. Any pointers 
on how to do this correctly would be appreciated.
  
  
  You do not need to do anything, basically: `vpn/CMakeLists.txt` already adds 
`libs/editor/widgets` to the includes for all the plugins, and the wireguard 
plugin already links to the `plasmanm_editor` library (which contains also that 
widget).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Pino Toscano

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  Misc notes:
  
  - please remove all the translations (i.e. `Name=[lang]` & `Comment[lang]` 
from desktop/json files, since KDE has an automatic system to handle them
  - please use KConfig 
 to read & write 
ini-like files, instead of doing everything manually
  - the `PasswordField` class is already available as 
`libs/editor/widgets/passwordfield.h`, part of the `plasmanm_editor` library, 
so you do not need to copy it locally
  - `wireguard_global.h` seems not used, so just drop it
  - `nm-wireguard-service.h` has lots of `NM_OPENVPN_` keys that are not used
  - as @lbeltrame said, please use `QHostAddress` to parse IP addresses
  - as @lbeltrame said, hardcoding colors is a bad idea, and it will not play 
nice with user choice of different color schemes, or accessibility purposes
  - a better option to validate an input in a line edit is to use the embedded 
validator options, see `QLineEdit::setInputMask` and `QValidator`
  - the better option to edit a port number is `QSpinBox` restricted to 
0-65536, instead of a line edit
  - there is a mixture of `virtual` & `Q_DECL_OVERRIDE` for virtual methods; 
plasma-nm uses `override` exclusively
  - please do not hardcode sizes and fonts in UI files
  - there are various texts in UI files with double spaces between words, 
please simply them to a single space
  - manually calling `KAcceleratorManager::manage(this);` is not needed, why 
were they added?

INLINE COMMENTS

> plasmanetworkmanagement_wireguardui.desktop:7-8
> +X-NetworkManager-Services=org.freedesktop.NetworkManager.wireguard
> +X-KDE-PluginInfo-Author=Lukáš Tinkl
> +X-KDE-PluginInfo-Email=lu...@kde.org
> +X-KDE-PluginInfo-Name=plasmanetworkmanagement_wireguardui

Surely this was not done by Lukáš... please fill in your name/email.

> wireguardadvanced.ui:39
> +   
> +Intreface
> +   

typo, "Interface:"

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich, pino
Cc: pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Bruce Anderson

andersonbruce added a comment.

  In D15093#315877 , @lbeltrame 
wrote:

  > Some smallish nitpicks I noticed there. Also, does this mean that NM has WG 
support now?

  Yes there is an add-on available. The original is at 
https://github.com/max-moser/network-manager-wireguard however I have made a 
couple of mods and issued pull requests but they have not been incorporated in 
the original yet. I anyone is interested in the latest I am using it is 
available at: 
https://github.com/Druco/network-manager-wireguard/tree/AllLocalFixes

  > I'll also let @jgrulich comment on this, but I'm not too fond of the manual 
parsing of the INI-like file used by WG. While I can't check at the moment, 
there should be KF5 or Qt classes to handle tihs.

  I will look into this. This is the first thing I have done using KF5/QT so 
there may be several places where I did it the hard way. Any pointers to more 
standard ways of doing things would be welcome.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich
Cc: lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Luca Beltrame

lbeltrame added a comment.


  In D15093#315885 , @andersonbruce 
wrote:
  
  >
  
  
  
  
  > As to copyright notice, I see that most are listed with name and email 
address.  Is the latter expected or is just name enough?  I don't have a 
business email anymore and I am somewhat hesitant to use my personal address 
but can if it is expected.
  
  This is important when the author is for example no longer involved in the 
specific development, but needs being contacted (for example, for asking for a 
relicense).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich
Cc: lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Bruce Anderson

andersonbruce added a comment.

  In D15093#315875 , @ngraham wrote:

  > Wow, what a first patch! And you even used `arc` too, how nice.
  >
  > Since this fixes https://bugs.kde.org/show_bug.cgi?id=397572, can you 
indicate it as such in the Summary section per 
https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords?
  >
  > Just add the following to the Summary section:
  >
  >   FEATURE: 397572
  >   FIXED-IN: 5.14.0
  >

  Done

  > In addition, I see one coding style issue right off the bat: we don't put 
opening braces on their own lines. That'll need to be changed in all the filed 
you've added. And speaking of those added files, I think you need to add your 
copyright to them. Finally, do we really need to duplicate `passwordfield.h` 
and `passwordfield.cpp`?

  I will start fixing the brace style.  This started off just for my own use so 
I'm afraid I didn't pay much attention to existing styles, my apologies. As far 
as the 'passwordfield' files, this was just laziness on my part, it being 
easier to just copy them than figure out how to get cmake (another tool I 
haven't used much) to add the correct include path to the flags. Any pointers 
on how to do this correctly would be appreciated.

  As to copyright notice, I see that most are listed with name and email 
address.  Is the latter expected or is just name enough?  I don't have a 
business email anymore and I am somewhat hesitant to use my personal address 
but can if it is expected.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich
Cc: lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Luca Beltrame

lbeltrame added a comment.


  Some smallish nitpicks I noticed there. Also, does this mean that NM has WG 
support now?
  
  I'll also let @jgrulich comment on this, but I'm not too fond of the manual 
parsing of the INI-like file used by WG. While I can't check at the moment, 
there should be KF5 or Qt classes to handle tihs.

INLINE COMMENTS

> passwordfield.h:2
> +/*
> +Copyright 2015 Jan Grulich 
> +

Please add your name to copyright if you wrote most of the code (likewise 
elsewhere in the code, and in the .desktop file.

> wireguardutils.cpp:42
> +// and/or a subnet (separated by the rest by a slash; 0 - 32)
> +bool WireGuardUtils::is_ip4(QString addr, bool allow_subnet, bool allow_port)
> +{

You may want to use QHostAddress instead of doing your own parsing of IPs: 
https://stackoverflow.com/questions/46853422/how-to-tell-if-qhostaddress-is-ipv4-or-ipv6-in-qt5

> wireguardwidget.cpp:296
> +{
> +d->ui.endpointLineEdit->setStyleSheet("* { background-color: 
> rgb(255,128, 128) }");
> +}

Why this? It will break proper coloring with people that use custom themes. 
IIRC you can have the widget handle it (but I can't check at the moment).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich
Cc: lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Nathaniel Graham

ngraham added reviewers: Plasma, jgrulich.
ngraham added a comment.


  Wow, what a first patch! And you even used `arc` too, how nice.
  
  Since this fixes https://bugs.kde.org/show_bug.cgi?id=397572, can you 
indicate it as such in the Summary section per 
https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords?
  
  Just add the following to the Summary section:
  
FEATURE: 397572
FIXED-IN: 5.14.0
  
  In addition, I see one coding style issue right off the bat: we don't put 
opening braces on their own lines. That'll need to be changed in all the filed 
you've added. And speaking of those added files, I think you need to add your 
copyright to them. Finally, do we really need to duplicate `passwordfield.h` 
and `passwordfield.cpp`?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15093

To: andersonbruce, #plasma, jgrulich
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15093: Add WireGuard capability.

2018-08-26 Thread Bruce Anderson

andersonbruce created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
andersonbruce requested review of this revision.

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  Feature/AddWireGuardVPN

REVISION DETAIL
  https://phabricator.kde.org/D15093

AFFECTED FILES
  libs/models/kcmidentitymodel.cpp
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/passwordfield.cpp
  vpn/wireguard/passwordfield.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguard_global.h
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardutils.cpp
  vpn/wireguard/wireguardutils.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

70 matches

Mail list logo