pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.
Much better now!
General notes:
- there are various checks on lengths of strings like `str.length() > 0` or
`str.length() != 0`: if all you need is check whether a string is empty or not,
just use `str.isEmpty()`
- 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
INLINE COMMENTS
> nm-wireguard-service.h:2
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> +/* nm-openvpn-service - openvpn integration with NetworkManager
> + *
this comment needs to be fixed
> wireguard.cpp:25
> +#include <QStringBuilder>
> +#include <QFile>
> +#include <QFileInfo>
unused
> wireguard.cpp:29-30
> +#include <KLocalizedString>
> +#include <KMessageBox>
> +#include <KStandardDirs>
> +#include <KConfig>
both unused
> wireguard.cpp:41
> +
> +#include <arpa/inet.h>
> +
unused
> wireguard.cpp:60
> +#define NMV_WG_TAG_FWMARK "FwMark"
> +#define NMV_WG_ASSIGN "="
> +
unused
> wireguard.cpp:151-153
> + KConfig importFile(fileName, KConfig::NoGlobals);
> + KConfigGroup interfaceGroup = importFile.group(NMV_WG_TAG_INTERFACE);
> + KConfigGroup peerGroup = importFile.group(NMV_WG_TAG_PEER);;
make all 3 as `const`, so it is clear they are read-only (and attempts to use
writeEntry() will result in build failures)
> wireguard.cpp:153
> + KConfigGroup interfaceGroup = importFile.group(NMV_WG_TAG_INTERFACE);
> + KConfigGroup peerGroup = importFile.group(NMV_WG_TAG_PEER);;
> +
extra ';'
> wireguard.cpp:166-172
> + // The config file must have both [Interface] and [Peer] sections
> + if (!importFile.groupList().contains("Interface")
> + || !importFile.groupList().contains("Peer")) {
> + mError = VpnUiPlugin::Error;
> + mErrorMessage = i18n("Could not open file");
> + return result;
> + }
These checks can be moved right after reading the config file and getting the
KConfigGroup objects.
> wireguard.cpp:167-168
> + // The config file must have both [Interface] and [Peer] sections
> + if (!importFile.groupList().contains("Interface")
> + || !importFile.groupList().contains("Peer")) {
> + mError = VpnUiPlugin::Error;
You do not need to get the list of groups in the config file (twice): earlier
you get the KConfigGroup objects for the groups, so checking eg
`interfaceGroup.exists()` should do the job.
> wireguard.cpp:175-178
> + value = interfaceGroup.readEntry(NMV_WG_TAG_ADDRESS);
> + {
> + QStringList addressList;
> + addressList << value.split(QRegExp("\\s*,\\s*"));
KConfig already supports comma-separated lists -- just pass QStringList() as
`default` value to readEntry(), so KConfigGroup knows the value is a list.
> wireguard.cpp:195
> + // Listen Port
> + value = interfaceGroup.readEntry(NMV_WG_TAG_LISTEN_PORT);
> + if (value.length() > 0) {
If you specify `0` as default parameter, KConfigGroup will try to decode the
value as integer automatically. If `0` is a valid value for the port, then use
`-1`.
After doing that, you do not need a validator anymore, just a manual range
check will do the job.
> wireguard.cpp:209
> + QRegExp validatorRegex(*regexStrings.keySpec);
> + QRegExpValidator validator(validatorRegex);
> + int pos = 0;
this validator is not needed, just use `validatorRegex` directly
> wireguard.cpp:231
> + // MTU
> + value = interfaceGroup.readEntry(NMV_WG_TAG_MTU);
> + if (value.length() > 0) {
same as for the listen port: please read the value directly as integer.
> wireguard.cpp:271
> + QRegExp validatorRegex(*regexStrings.keySpec);
> + QRegExpValidator validator(validatorRegex);
> + int pos = 0;
as above, no need for a validator, just use the QRegExp directly
> wireguard.cpp:288
> + + ", *)*" + *regexStrings.ip4Orip6Address);
> + QRegExpValidator *validator = new QRegExpValidator(allowedIPsRegex,
> this);
> + if (validator->validate(value, pos) != QValidator::State::Invalid) {
as above, no need for a validator, just use the QRegExp directly
> wireguard.cpp:306
> + QRegExp validatorRegex(*regexStrings.keySpec);
> + QRegExpValidator validator(validatorRegex);
> + int pos = 0;
as above, no need for a validator, just use the QRegExp directly
> wireguard.cpp:350-353
> + value = dataMap[NM_WG_KEY_ADDR_IP4];
> + if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP6))) {
> + value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> + }
as mentioned above: KConfigGroup supports lists
> wireguard.cpp:361-365
> + // Do Private Key
> + if (dataMap.contains(QLatin1String(NM_WG_KEY_PRIVATE_KEY)))
> + interfaceGroup.writeEntry(NMV_WG_TAG_PRIVATE_KEY,
> dataMap[NM_WG_KEY_PRIVATE_KEY]);
> + else
> + return false;
this, just like all the other required keys, should be checked before even
opening the output file
> wireguard.ui:23-25
> + <property name="fieldGrowthPolicy">
> + <enum>QFormLayout::ExpandingFieldsGrow</enum>
> + </property>
please leave the default behaviour, which is set by the style and/or the
platform plugin
> wireguard.ui:26-28
> + <property name="verticalSpacing">
> + <number>6</number>
> + </property>
no hardcoded margins/spacings please
> wireguard.ui:39
> + <property name="toolTip">
> + <string><html><head/><body><p>IPv4 intrnrt
> address assigned to the local interface. IPv4 or IPv6 address (or both)
> required</p></body></html></string>
> + </property>
typo "Internet" (proper noun), also in other places in this .ui file
> wireguardadvanced.ui:39
> + <property name="toolTip">
> + <string><html><head/><body><p>A 16-bit port
> for listening. Optional; if not specified, chosen
> randomly.</p></body></html></string>
> + </property>
IMHO specifying that is "16-bit" is out of scope, and of no value
> wireguardadvanced.ui:51-55
> + <widget class="QLineEdit" name="mTULineEdit">
> + <property name="toolTip">
> + <string><html><head/><body><p>If not
> specified, the MTU is automatically determined from the endpoint addresses or
> the system default route, which is usually a sane choice. However, to
> manually specify an MTU to override this automatic discovery, this value may
> be specified explicitly.</p></body></html></string>
> + </property>
> + </widget>
IMHO this is better as QSpinBox, with `0` as default value (see also the
`specialTextValue` property of QAbstractSpinBox).
This is also what the MTU configuration for wired connections uses, btw.
> wireguardadvancedwidget.h:59
> + void loadConfig();
> + void setOrClear(NMStringMap &data, QLatin1String key, QString value)
> const;
> + Ui::WireGuardAdvancedWidget *m_ui;
const & for both the key and value parameters
> wireguardauth.cpp:49-50
> +
> +
> +
> +QVariantMap WireGuardAuthWidget::setting() const
extra empty lines
> wireguardwidget.cpp:31-32
> +
> +#include <KProcess>
> +#include <KUrlRequester>
> +
undeeded
> wireguardwidget.h:26
> +
> +#include <QProcess>
> +
unneeded
> wireguardwidget.h:58
> + Private *d;
> + void setOrClear(NMStringMap &data, QLatin1String key, QString value)
> const;
> +
const & for both the key and value parameters
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
