D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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:

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#328246 , @pino wrote: > In D15093#328241 , @andersonbruce wrote: > > > In D15093#327917 , @pino wrote: > > > >

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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:

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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'

D15093: Add WireGuard capability.

jgrulich added a comment. In D15093#320672 , @andersonbruce wrote: > In D15093#320577 , @jgrulich wrote: > > > > > > > > > Does wg-quick support both, like simple commands and script

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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.

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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,

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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:

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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

D15093: Add WireGuard capability.

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