jgrulich added a comment.
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.
INLINE COMMENTS
> wireguard.cpp:189
> + } else { // Error condition
> + return result;
> + }
You multiple times return just result, you should also set error message with
the reason so users know why the import failed.
> wireguardadvancedwidget.cpp:104
> +
> + setOrClear(data, QLatin1String(NM_WG_KEY_LISTEN_PORT),
> m_ui->listenPortLineEdit->displayText());
> + setOrClear(data, QLatin1String(NM_WG_KEY_MTU),
> m_ui->mTULineEdit->displayText());
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 &data,
const QString &key, const QString &value).
> wireguardwidget.cpp:31
> +
> +#include <KProcess>
> +#include <KUrlRequester>
Not needed.
> wireguardwidget.cpp:129
> +{
> + // Currently WireGuard does not have any secrets
> +}
Add Q_UNUSED(setting);
> wireguardwidget.h:33
> +
> +class QUrl;
> +class QLineEdit;
QUrl and QLineEdit not needed.
> wireguardwidget.h:41
> + explicit WireGuardSettingWidget(const NetworkManager::VpnSetting::Ptr
> &setting,
> + const struct WireGuardRegexStrings
> &strings,
> + QWidget *parent = 0);
This paramater can be removed if you switch to using our IPv4 and IPv6
validators, also you can completely remove wireguardstrings structure you
defined. Even if anything like this would be needed, you basically need it in
one class, why not defining it there? If our validators are not enought, then
they should be improved so every class using them can benefit from that.
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
