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
