D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

This revision was automatically updated to reflect the committed changes. Closed by commit R116:999f0ad9fb8b: Update L2TP to NetworkManager-l2tp 1.8.0 features (authored by jgrulich). REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

jgrulich added a comment. I would say it's not a big deal as we use groupboxes quite a lot in plasma-nm. We can get rid of them in future when rewriting the KCM to QML. REVISION DETAIL https://phabricator.kde.org/D27764 To: dkosovic, jgrulich Cc: ngraham, plasma-devel, Orage, LeGast00n,

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

ngraham added a comment. It's just that in general we're trying to move away from group boxes and towards the use of whitespace to logically group sections. It's not a huge deal, but it would be appreciated. :) REVISION DETAIL https://phabricator.kde.org/D27764 To: dkosovic, jgrulich Cc:

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

dkosovic added a comment. When I was updating this client's UI, I was pretty conflicted about using group boxes, many of the other VPN clients were using group boxes for their main window and at one stage I was thinking of introducing group boxes for the main window of this client. It was

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

dkosovic updated this revision to Diff 76933. dkosovic added a comment. updates based on review comments CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27764?vs=76731=76933 REVISION DETAIL https://phabricator.kde.org/D27764 AFFECTED FILES vpn/l2tp/CMakeLists.txt

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

dkosovic added a comment. I'm of mixed minds on the group boxes. With L2TP/IPsec connection setup instructions for macOS and iOS, they typically have screenshots of the "User Authentication" and "Machine Authentication" settings. Many Linux users try to match what they see for other

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

ngraham added a comment. -1 for using group boxes. :) I don't think they're needed at all. Just change the "Type:" label to "Authentication:" and that section is clear enough, then just separate the logical sections with whitespace REPOSITORY R116 Plasma Network Management Applet

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

jgrulich added a comment. In D27764#621204 , @dkosovic wrote: > In D27764#621173 , @jgrulich wrote: > > > @dkosovic will you update the review to address my comments? > > > I agree with all

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

dkosovic added a comment. In D27764#621173 , @jgrulich wrote: > @dkosovic will you update the review to address my comments? I agree with all your comments and they are great nitpicks and suggestions. Sorry I didn't have time tonight

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

jgrulich added a comment. @dkosovic will you update the review to address my comments? REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D27764 To: dkosovic, jgrulich Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh,

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

jgrulich accepted this revision. jgrulich added a comment. This revision is now accepted and ready to land. Only nitpicking, otherwise it looks good to me. INLINE COMMENTS > l2tpauth.cpp:34 > +#include "nm-l2tp-service.h" > +#include "debug.h" > + This include doesn't seem to be used. >

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

dkosovic added a comment. This patch is depends on D27763 being applied first. Old l2tp: F8144374: old-l2tp.png Old l2tp IPsec Settings : F8144378: old-l2tp-ipsec.png

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

dkosovic created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added a reviewer: jgrulich. dkosovic requested review of this revision. REVISION SUMMARY - Update to NetworkManager-l2tp 1.8.0 features which include: - NetworkManager-l2tp 1.8.0