D29469: Add NetworkMannager::primaryConnection() call for WireGuard connections.

[Bruce Anderson]

andersonbruce created this revision.
andersonbruce added a reviewer: jgrulich.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
Herald added 1 blocking reviewer(s): jgrulich.
andersonbruce requested review of this revision.

REVISION SUMMARY
  If a WireGuard connection is made prior to start of the
  plasma-nm applet, no icon is displayed in the system tray.
  
  BUG: 420983

TEST PLAN
  1. Setup a Wired connection to autostart
  2. Setup a WireGuard connection which uses the Wired connection and set to 
autostart
  3. Logout and login to start a new Plasma session
  4. Verify that the standard Wired connection icon with a padlock symbol on it 
is shown in the system tray

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  master

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

AFFECTED FILES
  libs/declarative/connectionicon.cpp

To: andersonbruce, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D22283: Change validator for Endpoint Address entry field

[Bruce Anderson]

andersonbruce created this revision.
andersonbruce added a reviewer: jgrulich.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
Herald added 1 blocking reviewer(s): jgrulich.
andersonbruce requested review of this revision.

REVISION SUMMARY
  BUG: 408670
  
  Change validator for FQDN entry in Endpoint Address entry 
  field to allow single character domain names rather than 
  limiting them to a minimum of two characters.

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  FQDN_Fix

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

AFFECTED FILES
  libs/editor/settings/wireguardpeerwidget.cpp

To: andersonbruce, jgrulich
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  The changes to the key passwordfield menus was made to remove the AlwaysAsk 
option. This is the last change I am aware of necessary to release this.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Disable "AlwaysAsk" option in key password fields
  
  Caution: will not compile without changes to passwordfield.cpp and
  
passwordfield.h made in the current master branch.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57972=57983

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  In D20930#464412 , @jgrulich wrote:
  
  > In D20930#464403 , 
@andersonbruce wrote:
  >
  > > In D20930#464387 , @jgrulich 
wrote:
  > >
  > > > In D20930#464384 , @jgrulich 
wrote:
  > > >
  > > > > In D20930#464377 , 
@andersonbruce wrote:
  > > > >
  > > > > > Updated SecretAgent class to always try to get the secrets from 
kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses 
are 43 random characters long and we don't expect the user to enter these 
manually when trying to make a connection. If data is not available in kwallet 
then trying to make a connection will fail. Also updated the configuration 
screens to not allow a configuration with "AlwaysAsk" flags on either key.
  > > > >
  > > > >
  > > > > You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).
  > > >
  > > >
  > > > Sorry, that's not it, I added a new option to do that, you can now use 
PasswordField::setPasswordNotSavedEnabled(false).
  > >
  > >
  > > Great!  Should I make the change in this review? If so, what is the 
correct procedure (with git, arc, ...) to merge that change into this review?
  >
  >
  > Should be enough just to merge current master into your branch I guess.
  
  
  Isn't  'arc diff' going to try to include the differences between the master 
I originally checked out and the merged current master as changes in the 
WireGuard review then?  Sorry if these are stupid questions, I'm not a git 
expert and I just don't want to mess things up.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  
  
  In D20930#464387 , @jgrulich wrote:
  
  > In D20930#464384 , @jgrulich 
wrote:
  >
  > > In D20930#464377 , 
@andersonbruce wrote:
  > >
  > > > Updated SecretAgent class to always try to get the secrets from kwallet 
even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 
random characters long and we don't expect the user to enter these manually 
when trying to make a connection. If data is not available in kwallet then 
trying to make a connection will fail. Also updated the configuration screens 
to not allow a configuration with "AlwaysAsk" flags on either key.
  > >
  > >
  > > You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).
  >
  >
  > Sorry, that's not it, I added a new option to do that, you can now use 
PasswordField::setPasswordNotSavedEnabled(false).
  
  
  Great!  Should I make the change in this review? If so, what is the correct 
procedure (with git, arc, ...) to merge that change into this review?

INLINE COMMENTS

> jgrulich wrote in secretagent.cpp:432
> Shouldn't it be
> 
>   (isVpn && !isWireguard) && allowInteraction
> 
> You don't want to display password dialog in case it's a WG connection.
> 
> Or maybe it should stay and instead we should return true and send error 
> (like we do when a dialog has an error) if it's a WG connection, to let 
> NetworkManager know we cannot handle that.

Actually what I did is move the WireGuard handling above this 'else if' so it 
shouldn't get to this point if it is WireGuard so I simply returned the logic 
to what it was originally.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Return non-WireGuard logic to original state

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57970=57972

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  Updated SecretAgent class to always try to get the secrets from kwallet even 
if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 random 
characters long and we don't expect the user to enter these manually when 
trying to make a connection. If data is not available in kwallet then trying to 
make a connection will fail. Also updated the configuration screens to not 
allow a configuration with "AlwaysAsk" flags on either key.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Don't allow save of configuration with "AlwaysAsk" flag for keys

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57969=57970

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Always get encrypted data from kwallet
  
  Don't ask user for key values.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57888=57969

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  Update to fix some comments.

INLINE COMMENTS

> jgrulich wrote in connectionicon.cpp:403
> You can now use NetworkManager::Device::WireGuard.

There is still a discrepancy between the enum defined by NetworkManager and the 
one defined by NMQT.  NetworkManager is returning a value of 29 but 
NetworkManager::Device::WireGuard has a value of 30.

> jgrulich wrote in wireguardpeerwidget.cpp:52
> You can change all the validators to take all the necessary parameters as 
> first and then have a default value for the parent object so you don't need 
> to pass a nullptr.
> 
> E.g.
> 
>   explicit SimpleIpV4AddressValidator(AddressStyle style = 
> AddressStyle::Base, QObject *parent = nullptr);
> 
> I should have noticed this before. Can you change all the validators you use 
> this way?

SimpleIpV4AddressValidator and SimpleIpV6AddressValidator were intentionally 
done this way because they were existing functions that I didn't want to break 
when I added new (defaulted) parameters for use in with WireGuard functionality.

Since changing these two will involve changes to non-WireGuard code and it 
makes sense to do them all at the same time, can we write this up as a separate 
Bug?

> jgrulich wrote in networkmodelitem.cpp:480
> There is already WireGuard device in NMQT with KF5 5.58 which you should be 
> able to use.

This was just an extraneous comment so it was removed

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Correct review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57790=57888

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Update labels on Add and Remove peer buttons

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57741=57790

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#462775 , @ngraham wrote:
  
  > In D20930#462774 , 
@andersonbruce wrote:
  >
  > > Personally I like the Add and Remove buttons to be together where they 
were, and to simply change the wording to "Add New Peer" and "Remove This 
Peer". I think this still makes it clear that the peer in the tab currently 
being displayed in the dialog is the one that will be removed
  >
  >
  > All right, let's do that, then!
  
  
  I'll update to this.
  
  >> I haven't set up a large multi-user server but I imagine one could contain 
dozens if not hundreds of peers.
  > 
  > Hmm, if that's the case, then a tabbed UI may not actually be optimal. A 
list view, with a list of peers on the left side, a search above the list, and 
the peer information on the right, would be better if there may be enough peers 
to make access via tab view awkward. If so, I apologize for leading you down a 
bad path out of ignorance of the underlying technology. What do you think?
  
  I attempted to make a list view awhile ago and because of the length of some 
of the fields (in particular the key fields which are each 44 characters long) 
I couldn't come up with anything that looked decent. I think the tabbed 
interface will be fine in the vast majority of cases, especially in a desktop 
environment where typically there will only be one peer. Considering that the 
previous version only allowed one peer to be specified, this is a big 
improvement so I think at least for now and probably the foreseeable future, 
the tabbed interface is the way to go.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#462547 , @ngraham wrote:
  
  > So much better!
  >
  > I have one more little suggestion for the buttons: move the "Remove Peer" 
button into the bottom of the tab for that peer to make it clear what it 
applies to, and change the text to say, "Remove this Peer". The Remove button 
can stay where it is, but maybe should say, "Add New Peer". Actually, is "Add" 
the correct verb? Would "Connect to" be more appropriate?
  
  
  Here is a proposed version of what you asked for.  It also contains a little 
more context showing the parent dialog where the rest of the configuration is 
entered, behind the Peers dialog under discussion.
  
  Personally I like the Add and Remove buttons to be together where they were, 
and to simply change the wording to "Add New Peer" and "Remove This Peer". I 
think this still makes it clear that the peer in the tab currently being 
displayed in the dialog is the one that will be removed but am willing to do it 
the way shown below if the VDG considers it to be more consistent with the rest 
of KDE.
  
  The wording should definitely not be "Connect to" because this form is used 
to edit a configuration not to actually make a connection. Hitting "Connect" on 
a WireGuard configuration activates all the peers in that configuration at one 
time. I haven't set up a large multi-user server but I imagine one could 
contain dozens if not hundreds of peers.
  F6815862: RemoveBtnMoved.png 

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#462365 , @ngraham wrote:
  
  > I'm afraid I don't have the ability to test this; could you post a new 
screenshot? Thanks!
  
  
  Here's a screenshot of the dialog with three peers:F6814829: Tabs.png 


REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  A fairly large update was made to change the input of peer data to a use a 
QTabWidget interface to switch between peers rather than a spin box.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  Change to tab bar interface for peers

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57434=57741

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#459505 , @ngraham wrote:
  
  > When #VDG  assistance is requested, 
screenshots of the current/proposed UI are appreciated. :)
  
  
  F6804483: PeersDialog.png 
  In the dialog shown, there can be several different "Peers" defined. The 
question involves the use of the SpinBox as the means of switching between the 
various entries. Each Peer has the same set of parameters to display/modify and 
changing the SpinBox value changes which one of them is being currently 
displayed. The "Add" and "Remove" buttons add a new Peer or delete an existing 
Peer and change the "of x peers" label next to the SpinBox to correspond to the 
current total number.
  
  I have not seen this exact method of changing a dialog before and while it 
seems intuitive to me, I don't know if it violates any KDE standards and I 
think both jgrulich and I just wanted to get input from the VDG before this is 
released.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  Requested changes are made.

INLINE COMMENTS

> jgrulich wrote in connectionicon.cpp:403
> Do you want me to add this to NMQT?

Yes, please. This is part of the device type enum we discussed earlier where 
not all of the values defined in NM have been folded into NMQT yet.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Correct review comment

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57433=57434

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerswidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerswidget.cpp
  libs/editor/settings/wireguardpeerswidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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


  - Correct CamelCase of Wireguard to WireGuard
  - Disable adding WireGuard for NetworkManager < 1.16
  - Correct review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57297=57433

BRANCH
  TwoTabs

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

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerswidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerswidget.cpp
  libs/editor/settings/wireguardpeerswidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

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

REVISION SUMMARY
  In NetworkManager 1.16 handling of WireGuard interfaces was changed
  from a VPN add-on to a core interface type with a different API. This
  plasma-nm update is intended to match that change including (but not
  limited to) moving address specification to the IPv4 and IPv6 tabs and
  the ability to add multiple Peers to an interface.
  
  BUG: 405501

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  TwoTabs

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

AFFECTED FILES
  CMakeLists.txt
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerswidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerswidget.cpp
  libs/editor/settings/wireguardpeerswidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  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, jgrulich
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19434: Change entry of Endpoint UI

[Bruce Anderson]

andersonbruce added a comment.


  In D19434#424235 , @jgrulich wrote:
  
  > In D19434#423613 , 
@andersonbruce wrote:
  >
  > > In D19434#423458 , @jgrulich 
wrote:
  > >
  > > > Shouldn't we maybe use QSpinBox for the port? With the spinbox you can 
also set min and max values so you don't need to validate it.
  > >
  > >
  > > My preference is to only use a spinbox for entries with less than 100 
possible values.  I believe that the up/down arrows give the impression that 
clicking on them is the "proper" method of selecting a value and while some 
people know that you can type in a value into a spinbox, I believe that there 
are others who do not and I really don't want users to be metaphorically 
cursing my name when they are trying to click their way up to a port number of 
say, 23517. Also, using a simple LineEdit with color coding for indicating an 
unacceptable input is consistent with the rest of the interface.
  >
  >
  > In that case you can use 
https://doc.qt.io/qt-5/qlineedit.html#inputMask-prop to limit what users can 
type there. You can also assign validators to other lineedits so users will be 
able to type only correct values.
  
  
  This has been done.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D19434: Change entry of Endpoint UI

[Bruce Anderson]

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


  - Add validator for Endpoint Port field

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19434?vs=52875=53328

BRANCH
  EndPointUIChange

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

AFFECTED FILES
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardwidget.cpp

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

D19434: Change entry of Endpoint UI

[Bruce Anderson]

andersonbruce added a comment.


  In D19434#423458 , @jgrulich wrote:
  
  > Shouldn't we maybe use QSpinBox for the port? With the spinbox you can also 
set min and max values so you don't need to validate it.
  
  
  My preference is to only use a spinbox for entries with less than 100 
possible values.  I believe that the up/down arrows give the impression that 
clicking on them is the "proper" method of selecting a value and while some 
people know that you can type in a value into a spinbox, I believe that there 
are others who do not and I really don't want users to be metaphorically 
cursing my name when they are trying to click their way up to a port number of 
say, 23517. Also, using a simple LineEdit with color coding for indicating an 
unacceptable input is consistent with the rest of the interface.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D19435: Remove redundant parameter fields

[Bruce Anderson]

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

REVISION SUMMARY
  BUG: 403547
  
  Remove entry of parameters which are specific to the
  wg-quick utility rather than the actual WireGuard
  device driver. All of these items were optional and
  most can be set using different UI tabs or other
  NetworkManager supported methods.

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  RemoveRedundancy

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

AFFECTED FILES
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp

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

D19434: Change entry of Endpoint UI

[Bruce Anderson]

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

REVISION SUMMARY
  BUG: 403548
  
  Entry of the Endpoint was originally done in only
  one LineEdit which included both the host and port
  number. This update changes this so the host (as either
  an IPv4, IPv6, or Fully Qualified Domain Name) is
  entered in one LineEdit while the port number is
  specified in another.

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  EndPointUIChange

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

AFFECTED FILES
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardwidget.cpp

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

D18867: Remove redundant DNS field per bug 403546

[Bruce Anderson]

andersonbruce added a comment.


  Any connection created by the previous version or the basic NetworkManager 
plugin should continue to work even though the DNS server list will not be 
viewable in the new interface. 
  I did not provide an automatic conversion from a previously created 
connection to the new storage format. Neither did I create a warning (popup?) 
if an old style config is displayed by the editor. If there is a consensus to 
add either of these items, I can do that.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D18867: Remove redundant DNS field per bug 403546

[Bruce Anderson]

andersonbruce created this revision.
andersonbruce added a reviewer: jgrulich.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
Herald added 1 blocking reviewer(s): jgrulich.
andersonbruce requested review of this revision.

REVISION SUMMARY
  A DNS field was included on the VPN tab in the initial release to match the
  interface of the underlying NetworkManager plugin. This change removes this 
field and instead 
  requires the user to use the standard DNS field on the IPv4 and/or IPv6 tabs 
to enter DNS 
  servers.
  The import function was changed to insert any DNS servers specified in the 
incoming config 
  file into the new locations and the export function was changed to output DNS 
servers from 
  the IPv4 and IPv6 tabs if they are present but for compatibility with 
connections created 
  with the previous version or with the basic NetworkManager plugin, if the 
IPv4 and IPv6 tabs 
  do not contain DNS and an (undisplayed) entry on the VPN tab is present it 
will export from 
  there.  This allows a user to update a connection to the new format by simply 
exporting it, 
  deleting it, and re-importing it.

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  RemoveDNSField

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

AFFECTED FILES
  libs/editor/connectioneditorbase.cpp
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

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

D15093: Add WireGuard capability.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

[Bruce Anderson]

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


  - Fix review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15520?vs=42281=42297

BRANCH
  IP4/6validatorsUpdate

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

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
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp
  tests/simpleipv4test.cpp
  tests/simpleipv6test.cpp

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

D15093: Add WireGuard capability.

[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

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

[Bruce Anderson]

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


  - Fix a case that was being reported as Acceptable rather than Intermediate

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15520?vs=41878=42281

BRANCH
  IP4/6validatorsUpdate

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

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
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp
  tests/simpleipv4test.cpp
  tests/simpleipv6test.cpp

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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

andersonbruce abandoned this revision.
andersonbruce added a comment.


  Changes for this revision were merged into D15520 


REPOSITORY
  R116 Plasma Network Management Applet

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

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

D15093: Add WireGuard capability.

[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.

[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.

[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.

[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

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

[Bruce Anderson]

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


  - Remove unnecessary includes and member functions

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15520?vs=41875=41878

BRANCH
  IP4/6validatorsUpdate

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

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
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp
  tests/simpleipv4test.cpp
  tests/simpleipv6test.cpp

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

D15093: Add WireGuard capability.

[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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

andersonbruce added a comment.


  In D15521#327222 , @jgrulich wrote:
  
  > Maybe merge this review with D15520 . I 
think they should go together.
  
  
  I believe that I have now moved all the changes from this review into review 
D15520  so this review can be deleted or 
marked obsolete or whatever you want to do with it.  I also believe that all of 
the comments made here have been addressed in the changes updated to D15520 
.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

[Bruce Anderson]

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


  - Merge new IP list validator into this branch
  - Update from review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15520?vs=41677=41875

BRANCH
  IP4/6validatorsUpdate

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

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
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp
  tests/simpleipv4test.cpp
  tests/simpleipv6test.cpp

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

D15093: Add WireGuard capability.

[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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

andersonbruce added inline comments.

INLINE COMMENTS

> jgrulich wrote in simpleiplistvalidator.cpp:39
> In both validator constructors (mean also for IPv6 one), you can directly 
> pass "style" param from contructor, given both enums seem to be identical.

They are the same now but initially they were not (I just added the WithPort 
option to IPv6) and my thought was that they may not necessarily stay identical 
in the future. If someone goes in and changes, say, the IPv4 version without 
realizing it affects the list validator, this code still works.

> pino wrote in simpleiplistvalidator.cpp:62
> no need for a regexp, please use `QString::split()` with 
> `QString::SkipEmptyParts`

I used the regex so that it would split on commas with or without spaces around 
them and how I read SkipEmptyParts is that it will drop something between two 
adjacent commas which is not what I was trying to do. 
Now I know that I could use "trimmed()" on the resulting strings to strip off 
any remaining spaces but I don't see any advantage to either using "trimmed()" 
every time I use one of the resulting strings or doing it once and creating 
another local copy of the strings for this purpose. It seemed to me that doing 
everything in one step was a just as viable approach unless there is something 
inherently bad with splitting on a regex that I am not aware of, especially 
given that I commented it to explain what it is doing.

> pino wrote in simpleiplistvalidator.cpp:67
> set (later on), but never use

I don't understand this comment. 'result' is set here to Acceptable, it can be 
modified to Intermediate at line 102 but if all the addresses are valid it 
needs to have been set initially to Acceptable for the return at line 106 to be 
correct.

> pino wrote in simpleiplisttest.cpp:53
> `QVERIFY(foo == bar)` -> `QCOMPARE(foo, bar)`, it applies to all the checks 
> in this file

Any particular reason you think this is superior?

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

andersonbruce added a comment.


  In D15521#327222 , @jgrulich wrote:
  
  > Maybe merge this review with D15520 . I 
think they should go together.
  
  
  Can you please discuss this with Pino, he was the one who suggested separate 
reviews. 
  I will follow whatever you decide but if you decide that they should be 
merged please let me know how to do this.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

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

INLINE COMMENTS

> jgrulich wrote in CMakeLists.txt:92
> I'm sure you don't need to add all these and why did you remove PUBLIC and 
> PRIVATE keywords?

Damn, copied the wrong file. Sorry about that.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

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


  - Correct CMakefile I screwed up
  
  Updating D15521: Add validator for lists of IP addresses
  
  
  Added as separate review per comment from Pino on
  review D15093 . This code will not 
compile without
  the updated code in review D15520 . Also 
includes
  unit test.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15521?vs=41733=41794

BRANCH
  IPAddressListValidator

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

AFFECTED FILES
  libs/editor/CMakeLists.txt
  libs/editor/simpleiplistvalidator.cpp
  libs/editor/simpleiplistvalidator.h
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp

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

D15093: Add WireGuard capability.

[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.

[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.

[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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

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


  - Correct overlooked comments from review D15093 

  
  Updating D15521: Add validator for lists of IP addresses
  
  
  Added as separate review per comment from Pino on
  review D15093 . This code will not 
compile without
  the updated code in review D15520 . Also 
includes
  unit test.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15521?vs=41678=41733

BRANCH
  IPAddressListValidator

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

AFFECTED FILES
  libs/editor/CMakeLists.txt
  libs/editor/simpleiplistvalidator.cpp
  libs/editor/simpleiplistvalidator.h
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp

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

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

[Bruce Anderson]

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

REVISION SUMMARY
  CCBUG: 397572
  FIXED-IN: 5.14.0

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  IPAddressListValidator

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

AFFECTED FILES
  libs/editor/CMakeLists.txt
  libs/editor/simpleiplistvalidator.cpp
  libs/editor/simpleiplistvalidator.h
  tests/CMakeLists.txt
  tests/simpleiplisttest.cpp

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

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidatorto optionally handle a CIDR or port suffix. Added capabilitiesimplemented via defaulted parameter to maintain compatibilitywith

[Bruce Anderson]

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

REVISION SUMMARY
  ...updated
  code. Opened as a separate review per comment from Pino on review
  D15093 .
  
  Part of:
  FEATURE: 397572
  FIXED-IN: 5.14.0

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  IP4/6validatorsUpdate

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

AFFECTED FILES
  CMakeLists.txt
  libs/editor/simpleipv4addressvalidator.cpp
  libs/editor/simpleipv4addressvalidator.h
  libs/editor/simpleipv6addressvalidator.cpp
  libs/editor/simpleipv6addressvalidator.h
  tests/CMakeLists.txt
  tests/simpleipv4test.cpp
  tests/simpleipv6test.cpp

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

D15093: Add WireGuard capability.

[Bruce Anderson]

andersonbruce added a comment.


  In D15093#323095 <https://phabricator.kde.org/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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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.

[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

Linux Pro Magazine article about Wayland adoption

[Bruce Byfield]

Hi:

I'm preparing an article on distros' adoption of Wayland. I was hoping
to have answers to a few questions:

- What is the state of Wayland development in Plasma? 

- Are there any obstacles in Plasma or perhaps Qt that prevent
  Wayland from being used by average users in KDE?

- How will the user experience of Wayland differ from that of the X
  Window system?

- When Wayland is introduced, will a fallback to X be available?

- Is there a tentative schedule for Wayland's release?

- Is there anything else that you would like to say.

Note that I am not described to the list, so please copy me on any
replies.

I will need any answers by Friday, October 13.

Thanks for any answers,

-- 
Bruce Byfield (on Pacific time) 604-421-7177
Writer of "Designing with LibreOffice"
www.designingwithlibreoffice.com

how to draw a transparent / semi-transparent area in plasma-qml ?

[Bruce Ouyang]

i have tried rectrangle with opacity:0.5 and color:transparent, but this 
doesn't work.
looking forward for your reply!
thanks advance!
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: how to read and write clipboard in plasma qml?

[Bruce Ouyang]

thanks for your reply!
there is no singal handler named onCompleted in the document here:
http://qt-project.org/doc/qt-4.8/qml-textedit.html

where do you find it? where is the description?

On 2013年7月23日星期二 CST下午8时53分00秒, Sebastian Kügler wrote:

Hi,

On Friday, July 19, 2013 22:39:42 Bruce Ouyang wrote:
i have googled and found no solution. it seems i have to 
utilize LauchApp to

call a python script for accessing clipboard ps: xclip command is not my
solution because it may be absent in some KDE distributions.


You can use a QML TextEdit as helper, this allows you to access 
the clipboard.


so if

TextEdit {
  visible: false
  text: myStuff
  onCompleted: { 
text = This is going to be copied

selectAll()
copy()
  }
  [...]
}

Same mechanism with paste(), just the other way round.

Cheers,


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

does XMLHttpRequest support https access and setting request headers in qml?

[Bruce Ouyang]

1, i try to call xhr.open(GET, 
https://api.weibo.com/2/short_url/shorten.json?source=3599790545url_long=http%3A%2F%2Fopen.weibo.com%2Fwiki%2F2%2Fshort_url%2Fshorten;)
 but failed, but i can open this link in google chrome browser.
2, when i try to call xhr.setRequestHeader(Content-Type, application/json), 
an error is throwed out : Error: Invalid state

looking forward for your reply !
thanks advance !

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: does XMLHttpRequest support https access and setting request headers in qml?

[Bruce Ouyang]

On 2013年7月21日星期日 CST下午6时15分45秒, Bruce Ouyang wrote:
1, i try to call xhr.open(GET, 
https://api.weibo.com/2/short_url/shorten.json?source=3599790545url_long=http%3A%2F%2Fopen.weibo.com%2Fwiki%2F2%2Fshort_url%2Fshorten;;) 
but failed, but i can open this link in google chrome browser.


now i think xhr in qml support https access, there is some other problem in my 
little program.
2, when i try to call xhr.setRequestHeader(Content-Type, 
application/json), an error is throwed out : Error: Invalid 
state

my mistake. i should call xhr.setRequestHeader after xhr.open



___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: does HTTP extension work in plasma qml?

[Bruce Ouyang]

i found qml XmlHttpRequest is fit for my needs, thanks for your reply.

On 2013年7月16日星期二 CST下午8时44分01秒, Sebastian Kügler wrote:

Hi Bruce,

On Tuesday, July 16, 2013 19:51:09 bruce wrote:
i have wrote a little plasma-qml applet.i try to utilize HTTP 
extension from 
plasma-javascript to retreive web resources. but 
plasmoid.getUrl() returned 
undefined. in plasma-javascript works well.

i have pasted the main.qml code into paste.kde.org, link follows:
http://paste.kde.org/p1f0f7ab5/
the attachment is the whole project zip file.


i have add line X-Plasma-RequiredExtensions=LaunchApp,HTTP in this qml 
plasmoid metadata.desktop is HTTP extension available in QML?


This is a little different in QML, you can load content from a 
http:// url in 
many components without this extension. We are not able to 
sandbox QML as we 
do with JavaScript, so our QML API, while being very similar to 
the JS one, is 
less restrictive.


Wether it works or not depends a little on the actual usecase, 
some properties 
(the image source, for example) just work with http (or QUrl, really).


I can't look into your code as the zip is filtered out by a 
virus scanner, but 
I believe this answer should help you. If not, let us know.


Cheers,


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

how to read and write clipboard in plasma qml?

[Bruce Ouyang]

i have googled and found no solution. it seems i have to utilize LauchApp to 
call a python script for accessing clipboard
ps: xclip command is not my solution because it may be absent in some KDE 
distributions.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

how to monitor TextEdit's value in plasma qml?

[Bruce Ouyang]

in html, textedit element has an onChange attribute for monitoring its value 
change. plasma qml doesn't.
how can i achieve this purpose?
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: wierd problems about systemmonitor's acpi temperature

[bruce]

yes, the directory does exist, and

/cat /sys/class/thermal/thermal_zone0/temp/

returns /55000/

在 2013年5月23日 星期四 09:18:43，Giorgos Tsiapaliokas 写道：


Hello,


On 22 May 2013 07:16, bruce bruce...@gmail.com[1] wrote:
2,  when wierd problem #2 happened, i checked 
directory'/proc/acpi/thermal_zone',and found 
its gone, but i am sure it really didexists for a long time. same machine, same 
operating 
system, why does thishappens?




does this one /sys/class/thermal/ exists on your machine?


-- 


Giorgos Tsiapaliokas (terietor)


terietor.org[2] 




[1] mailto:bruce...@gmail.com
[2] http://terietor.org
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

wierd problems about systemmonitor's acpi temperature

[bruce]

dataEngine(systemmonitor).connectSource(acpi/Thermal_Zone/0/Temperature,plasmoid,500)

in my plasmoid i use the preceding statements to connect to 
acpi/Thermal_Zone/0/Temperature, and use  plasmoid.dataUpdated to receive 
temperature data.

but there is a very wierd problem:
1, at the very beginning, it works at least for 10 days. i can receive cpu 
temperature data. i extract temperature data from data[value]
2, but suddenly for unkown reason, i can not receive temperature anymore.
dataUpdated method is still be called periodly, but data[value] is 
undefined. 
3, in someone else's machine, 
dataEngine(systemmonitor).connectSource(acpi/Thermal_Zone/0/Temperature,plasmoid,500)
 
returns true, but dataUpdated is never be called!

here is my question:
1, from where  does systemmonitor dataengine 's datasource 
acpi/Thermal_Zone/0/Temperature retrieve temperature data?  is it from 
directory '/proc/acpi/thermal_zone'? or '/sys/class/thermal/thermal_zone0'?

2,  when wierd problem #2 happened, i checked directory 
'/proc/acpi/thermal_zone',and found its gone, but i am sure it really did 
exists for a long time. same machine, same operating system, why does this 
happens?

any information is welcomed, thanks advance!
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

how to capture a mouse click when developing a plasmoid using qtscript?

[bruce . oy]

i'd like to write a plasmoid-wrapper for byzanz which is a really nice desktop 
record tool especially can produce gif directly.
thanks for your help advance.




bruce.oy___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

it seems prototype inheritance is not supported in qtscript when developing a plasmoid, right? please tell me if you know, thanks advance.

[bruce]

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

if i define a function testfunc() in plasmoid /contents/main.js, when user add two instances of my plasmoid to taskbar, in these two instances, is function testfunc() a same object?

[bruce]

if i define testfunc.PROPERTY1=1, is it a same property?
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: if i define a function testfunc() in plasmoid /contents/main.js, when user add two instances of my plasmoid to taskbar, in these two instances, is function testfunc() a same object?

[bruce]

在 2013年5月13日 星期一 15:57:16，Giorgos Tsiapaliokas 写道：


Hello,


your function will be a function in all the instances
of your plasmoid, but it won't live in the same memory.


What exactly do you want to do?




On 13 May 2013 14:22, bruce bruce...@gmail.com[1] wrote:


if i define testfunc.PROPERTY1=1, is it a same 
property?___Plasma-devel mailing 
list

Plasma-devel@kde.org[2]
https://mail.kde.org/mailman/listinfo/plasma-devel[3]





-- 


Giorgos Tsiapaliokas (terietor)


terietor.org[4]



my purpose is finding a variable or anything similar which exists in different 
instances of a 
plasmoid, similar with shared memory between processes, shared object between 
java 
threads, etc.




[1] mailto:bruce...@gmail.com
[2] mailto:Plasma-devel@kde.org
[3] https://mail.kde.org/mailman/listinfo/plasma-devel
[4] http://terietor.org
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

there seems to be no dataengines or datasources can provide harddisk's temperature, right?

[bruce]

so all i have to do is install hddtemp or smartctl?
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel