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 <https://phabricator.kde.org/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 <https://api.kde.org/frameworks/kconfig/html/classKConfig.html> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180831/5f76d23e/attachment.html>