pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed.
note there are still few "not done" comments around (eg using QSpinBox for fwMark) INLINE COMMENTS > CMakeLists.txt:28 > Widgets > + Test > ) already added by D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator <https://phabricator.kde.org/D15520> > wireguard.cpp:139 > + > + WireGuardKeyValidator keyValidator(this); > + int keyPos = 0; no need to specify `this` as parent, since it is on stack, and thus it will be deleted automatically > wireguard.cpp:139-140 > + > + WireGuardKeyValidator keyValidator(this); > + int keyPos = 0; > + indent > wireguard.cpp:177 > + if (!value.isEmpty()) { > + int pos = 0; > + SimpleIpListValidator validator(this, since earlier there is a `keyPos` variable used for basically the same purpose (i.e. passing it to the `validate()` of validators), just move `pos` earlier in the function, and use one everywhere > wireguardadvancedwidget.cpp:26 > + > +#include <QStandardPaths> > +#include <QUrl> unused > wireguardadvancedwidget.cpp:31 > +#include <KLocalizedString> > +#include <KProcess> > +#include <KAcceleratorManager> unused > wireguardadvancedwidget.cpp:120 > + NMStringMap data; > + long intVal; > + QString stringVal; a bit confusing to call `intVal` a variable which is (not correctly) a long; OTOH, it can be removed altogether (see below) > wireguardadvancedwidget.cpp:121 > + long intVal; > + QString stringVal; > + unused > wireguardadvancedwidget.cpp:123-127 > + intVal = m_ui->listenPortSpinBox->value(); > + setOrClear(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), intVal); > + > + intVal = m_ui->mtuSpinBox->value(); > + setOrClear(data, QLatin1String(NM_WG_KEY_MTU), intVal); just inline the calls to `value()`, just like done below > wireguardadvancedwidget.h:27 > +#include <QDialog> > +#include <QProcess> > + unused > wireguardkeyvalidator.h:26 > + > +class Q_DECL_EXPORT WireGuardKeyValidator : public QValidator > +{ no need for `Q_DECL_EXPORT`, as it is in a plugin, and thus it does not need to be exported as public symbol > pino wrote in wireguardwidget.cpp:182 > no need for the (...) for the whole condition: > > return foo || bar; > > is easier than > > return (foo || bar); this is not "done", btw 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
