D15093: Add WireGuard capability.

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

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

Missing wireguardkeyvalidator.cpp

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

Linking against following ones is enough:

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

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

Name could be just WireGuard I guess

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

Coding style

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

Coding style

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

Coding style

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

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

Same please do for classic wireguard widget.

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

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

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

Coding style

> wireguardwidget.cpp:30
> +#include <QPointer>
> +#include <KColorScheme>
> +

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

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

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

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

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

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

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

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

Coding style

REPOSITORY
  R116 Plasma Network Management Applet

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

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20181017/0fc1a248/attachment-0001.html>