Differential D15093 Diff 45010 vpn/wireguard/wireguardadvancedwidget.cpp

Changeset View

Standalone View

View Options

vpn/wireguard/wireguardadvancedwidget.cpp

  • This file was added.
1​/*
2​ Copyright 2018 Bruce Anderson <banderson19com@san.rr.com>
3​
4​ This library is free software; you can redistribute it and/or
5​ modify it under the terms of the GNU Lesser General Public
6​ License as published by the Free Software Foundation; either
7​ version 2.1 of the License, or (at your option) version 3, or any
8​ later version accepted by the membership of KDE e.V. (or its
9​ successor approved by the membership of KDE e.V.), which shall
10​ act as a proxy defined in Section 6 of version 3 of the license.
11​
12​ This library is distributed in the hope that it will be useful,
13​ but WITHOUT ANY WARRANTY; without even the implied warranty of
14​ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15​ Lesser General Public License for more details.
16​
17​ You should have received a copy of the GNU Lesser General Public
18​ License along with this library. If not, see <http://www.gnu.org/licenses/>.
19​*/
20​
21​#include "wireguardadvancedwidget.h"
22​#include <QtWidgets/QPushButton>
23​#include <KColorScheme>
24​#include "ui_wireguardadvanced.h"
25​#include "nm-wireguard-service.h"
26​#include "settingwidget.h"
27​#include "wireguardkeyvalidator.h"
pinoUnsubmitted
Done

unused

pino: unused
28​
29​class WireGuardAdvancedWidget::Private
30​{
31​public:
32​ NetworkManager::VpnSetting::Ptr setting;
pinoUnsubmitted
Done

unused

pino: unused
33​ Ui::WireGuardAdvancedWidget ui;
34​ QPalette warningPalette;
35​ QPalette normalPalette;
36​ WireGuardKeyValidator *keyValidator;
37​ QIntValidator *listenPortValidator;
38​ QIntValidator *mtuValidator;
39​ QRegularExpressionValidator *fwMarkValidator;
40​ QIntValidator *persistentKeepaliveValidator;
41​ QRegularExpressionValidator *tableValidator;
42​ ~Private();
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
43​ Private();
44​ bool fwMarkValid;
45​ bool presharedKeyValid;
46​ bool tableValid;
47​};
48​
49​WireGuardAdvancedWidget::Private::Private() : fwMarkValid(true),
jgrulichUnsubmitted
Not Done

Coding style, but can be fixed afterwards.

jgrulich: Coding style, but can be fixed afterwards.
50​ presharedKeyValid(true),
51​ tableValid(true)
52​{
53​}
54​
55​WireGuardAdvancedWidget::Private::~Private()
56​{
pinoUnsubmitted
Done
  • 4294967295, other than being a magic value (using std::numeric_limits is a better idea), will even overflow the maximum value for the validator, since it is int (i.e. 32bit signed)
  • instead of passing minimum & maximum to constructor, just set the minimum to 0 with setMinimum, and do not touch the maximum (which is already the maximum int value)
  • fwMarkValidator is leaked: use e.g. the lineedit where it will be used as parent

... OTOH, using QSpinBox for this will basically solve all the issues above

pino: - 4294967295, other than being a magic value (using std::numeric_limits is a better idea), will…
57​ delete keyValidator;
58​ delete listenPortValidator;
59​ delete mtuValidator;
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
60​ delete fwMarkValidator;
61​ delete persistentKeepaliveValidator;
62​ delete tableValidator;
63​}
64​
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
65​WireGuardAdvancedWidget::WireGuardAdvancedWidget(const NetworkManager::VpnSetting::Ptr &setting,
66​ QWidget *parent)
67​ : QDialog(parent), d(new Private)
68​{
69​ KSharedConfigPtr config = KSharedConfig::openConfig();
70​ d->warningPalette = KColorScheme::createApplicationPalette(config);
71​ d->normalPalette = KColorScheme::createApplicationPalette(config);
72​ KColorScheme::adjustBackground(d->warningPalette,
73​ KColorScheme::NegativeBackground,
74​ QPalette::Base,
75​ KColorScheme::ColorSet::View,
76​ config);
77​
78​ KColorScheme::adjustBackground(d->normalPalette,
79​ KColorScheme::NormalBackground,
80​ QPalette::Base,
81​ KColorScheme::ColorSet::View,
82​ config);
83​
84​ d->keyValidator = new WireGuardKeyValidator(this);
85​ d->ui.setupUi(this);
86​
87​ setWindowTitle(i18nc("@title: window advanced wireguard properties",
88​ "Advanced WireGuard properties"));
jgrulichUnsubmitted
Done

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.

jgrulich: This validation doesn't seem to work, I can click on "ok" button even when the advanced widget…
andersonbruceAuthorUnsubmitted
Not Done

I did this slightly different from the suggestion but it only does the checks once per change and I think it is correctly enabling/disabling the "Ok" button. By the way, the "Ok" button is enabled when all entries on the Advanced widget are blank because all of the entries are optional and therefore all blank is perfectly valid.

andersonbruce: I did this slightly different from the suggestion but it only does the checks once per change…
89​
90​ d->setting = setting;
91​
92​ connect(d->ui.buttonBox, &QDialogButtonBox::accepted, this, &WireGuardAdvancedWidget::accept);
93​ connect(d->ui.buttonBox, &QDialogButtonBox::rejected, this, &WireGuardAdvancedWidget::reject);
jgrulichUnsubmitted
Done

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

jgrulich: Perhaps you should write a destructor for the private widget and here just delete the d-pointer.
94​ connect(d->ui.presharedKeyLineEdit, &PasswordField::textChanged,
95​ this, &WireGuardAdvancedWidget::checkPresharedKey);
96​ connect(d->ui.tableLineEdit, &QLineEdit::textChanged,
97​ this, &WireGuardAdvancedWidget::checkTable);
98​ connect(d->ui.fwMarkLineEdit, &QLineEdit::textChanged,
pinoUnsubmitted
Done

QString(), instead of ""

pino: `QString()`, instead of `""`
99​ this, &WireGuardAdvancedWidget::checkFwMark);
100​ d->ui.presharedKeyLineEdit->setPasswordModeEnabled(true);
101​
102​ d->listenPortValidator = new QIntValidator(0, 65535, nullptr);
103​ d->ui.listenPortLineEdit->setValidator(d->listenPortValidator);
104​
105​ d->persistentKeepaliveValidator = new QIntValidator(0, 65535, nullptr);
jgrulichUnsubmitted
Done

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).

jgrulich: I would prefer having just an empty map with data where you just set everything the user…
andersonbruceAuthorUnsubmitted
Not Done

Functionally I think that the current implementation does this (although I can change the name of the function if you want). It starts with a blank NMStringMap and uses setOrClear on it. Are you possibly referring to the same function name used in wireguardwidget.cpp rather than here in wireguardadvancedwidget.cpp?

andersonbruce: Functionally I think that the current implementation does this (although I can change the name…
106​ d->ui.persistentKeepaliveLineEdit->setValidator(d->persistentKeepaliveValidator);
107​
108​ d->mtuValidator = new QIntValidator(nullptr);
109​ d->mtuValidator->setBottom(0);
110​ d->ui.mtuLineEdit->setValidator(d->mtuValidator);
111​
112​ d->fwMarkValidator = new QRegularExpressionValidator(QRegularExpression("(off)|([0-9]{0,10})|(0x[0-9a-fA-F]{1,8})"));
113​ d->ui.fwMarkLineEdit->setValidator(d->fwMarkValidator);
114​
115​ d->tableValidator = new QRegularExpressionValidator(QRegularExpression("(off)|(auto)|([0-9]*)"));
116​ d->ui.tableLineEdit->setValidator(d->tableValidator);
117​
pinoUnsubmitted
Done

this is basically the positive-or-null int version of setOrClear:

void WireGuardAdvancedWidget::setOrClear(NMStringMap &data, const QLatin1String &key, int value) const
​{
  if (value > 0)
    data.insert(key, QString::number(value));
  else
    data.remove(key);
}
pino: this is basically the positive-or-null int version of setOrClear: lang=c++ void…
118​ KAcceleratorManager::manage(this);
119​
120​ if (d->setting) {
121​ loadConfig();
pinoUnsubmitted
Done

ditto

pino: ditto
pinoUnsubmitted
Done

a bit confusing to call intVal a variable which is (not correctly) a long; OTOH, it can be removed altogether (see below)

pino: a bit confusing to call `intVal` a variable which is (not correctly) a long; OTOH, it can be…
122​ }
pinoUnsubmitted
Done

unused

pino: unused
123​ checkPresharedKey();
124​ checkFwMark();
125​ checkTable();
126​}
127​
128​WireGuardAdvancedWidget::~WireGuardAdvancedWidget()
pinoUnsubmitted
Done

just inline the calls to value(), just like done below

pino: just inline the calls to `value()`, just like done below
129​{
130​ delete d;
131​}
132​
133​void WireGuardAdvancedWidget::loadConfig()
134​{
135​ const NMStringMap dataMap = d->setting->data();
136​
137​ if (dataMap.contains(QLatin1String(NM_WG_KEY_LISTEN_PORT)))
138​ d->ui.listenPortLineEdit->setText(dataMap[NM_WG_KEY_LISTEN_PORT]);
139​ else
140​ d->ui.listenPortLineEdit->clear();
141​
142​ if (dataMap.contains(QLatin1String(NM_WG_KEY_PERSISTENT_KEEPALIVE)))
143​ d->ui.persistentKeepaliveLineEdit->setText(dataMap[NM_WG_KEY_PERSISTENT_KEEPALIVE]);
144​ else
145​ d->ui.persistentKeepaliveLineEdit->clear();
146​
147​ if (dataMap.contains(QLatin1String(NM_WG_KEY_MTU)))
148​ d->ui.mtuLineEdit->setText(dataMap[NM_WG_KEY_MTU]);
149​ else
150​ d->ui.mtuLineEdit->clear();
151​
152​ if (dataMap.contains(QLatin1String(NM_WG_KEY_TABLE)))
153​ d->ui.tableLineEdit->setText(dataMap[NM_WG_KEY_TABLE]);
154​ else
155​ d->ui.tableLineEdit->clear();
156​
157​ if (dataMap.contains(QLatin1String(NM_WG_KEY_FWMARK)))
158​ d->ui.fwMarkLineEdit->setText(dataMap[NM_WG_KEY_FWMARK]);
159​ else
160​ d->ui.fwMarkLineEdit->clear();
161​
162​ if (dataMap.contains(QLatin1String(NM_WG_KEY_PRESHARED_KEY)))
163​ d->ui.presharedKeyLineEdit->setText(dataMap[NM_WG_KEY_PRESHARED_KEY]);
164​ else
165​ d->ui.presharedKeyLineEdit->setText(QString());
166​
167​ if (dataMap.contains(QLatin1String(NM_WG_KEY_PRE_UP)))
168​ d->ui.preUpLineEdit->setText(dataMap[NM_WG_KEY_PRE_UP]);
169​ else
170​ d->ui.preUpLineEdit->setText(QString());
171​
172​ if (dataMap.contains(QLatin1String(NM_WG_KEY_POST_UP)))
173​ d->ui.postUpLineEdit->setText(dataMap[NM_WG_KEY_POST_UP]);
174​ else
175​ d->ui.postUpLineEdit->setText(QString());
176​
177​ if (dataMap.contains(QLatin1String(NM_WG_KEY_PRE_DOWN)))
178​ d->ui.preDownLineEdit->setText(dataMap[NM_WG_KEY_PRE_DOWN]);
179​ else
180​ d->ui.preDownLineEdit->setText(QString());
181​
182​ if (dataMap.contains(QLatin1String(NM_WG_KEY_POST_DOWN)))
183​ d->ui.postDownLineEdit->setText(dataMap[NM_WG_KEY_POST_DOWN]);
184​ else
185​ d->ui.postDownLineEdit->setText(QString());
186​}
187​
188​void WireGuardAdvancedWidget::setProperty(NMStringMap &data, const QLatin1String &key,
189​ const QString &value) const
190​{
191​ if (!value.isEmpty())
192​ data.insert(key, value);
193​}
194​
195​NetworkManager::VpnSetting::Ptr WireGuardAdvancedWidget::setting() const
196​{
197​ NMStringMap data;
198​ QString stringVal;
199​
200​ setProperty(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), d->ui.listenPortLineEdit->displayText());
201​ setProperty(data, QLatin1String(NM_WG_KEY_PERSISTENT_KEEPALIVE),
202​ d->ui.persistentKeepaliveLineEdit->displayText());
203​ setProperty(data, QLatin1String(NM_WG_KEY_MTU), d->ui.mtuLineEdit->displayText());
204​ setProperty(data, QLatin1String(NM_WG_KEY_TABLE), d->ui.tableLineEdit->displayText());
205​ setProperty(data, QLatin1String(NM_WG_KEY_FWMARK), d->ui.fwMarkLineEdit->displayText());
206​ setProperty(data, QLatin1String(NM_WG_KEY_PRESHARED_KEY), d->ui.presharedKeyLineEdit->text());
207​ setProperty(data, QLatin1String(NM_WG_KEY_PRE_UP), d->ui.preUpLineEdit->text());
208​ setProperty(data, QLatin1String(NM_WG_KEY_POST_UP), d->ui.postUpLineEdit->text());
209​ setProperty(data, QLatin1String(NM_WG_KEY_PRE_DOWN), d->ui.preDownLineEdit->text());
210​ setProperty(data, QLatin1String(NM_WG_KEY_POST_DOWN), d->ui.postDownLineEdit->text());
211​
212​ d->setting->setData(data);
213​
214​ return d->setting;
215​}
216​
217​void WireGuardAdvancedWidget::checkPresharedKey()
218​{
219​ int pos = 0;
220​ PasswordField *widget = d->ui.presharedKeyLineEdit;
221​ QString value = widget->text();
222​ d->presharedKeyValid = QValidator::Acceptable == d->keyValidator->validate(value, pos)
223​ || value.isEmpty();
224​ setBackground(widget, d->presharedKeyValid);
225​ slotWidgetChanged();
226​}
227​
228​void WireGuardAdvancedWidget::checkTable()
229​{
230​ int pos = 0;
231​ QLineEdit *widget = d->ui.tableLineEdit;
232​ QString value = widget->displayText();
233​ d->tableValid = QValidator::Acceptable == widget->validator()->validate(value, pos)
234​ || value.isEmpty();
235​ setBackground(widget, d->tableValid);
236​ slotWidgetChanged();
237​}
238​
239​void WireGuardAdvancedWidget::checkFwMark()
240​{
241​ int pos = 0;
242​ QLineEdit *widget = d->ui.fwMarkLineEdit;
243​ QString value = widget->displayText();
244​ d->fwMarkValid = QValidator::Acceptable == widget->validator()->validate(value, pos)
245​ || value.isEmpty();
246​ setBackground(widget, d->fwMarkValid);
247​ slotWidgetChanged();
248​}
249​
250​void WireGuardAdvancedWidget::setBackground(QWidget *w, bool result) const
251​{
252​ if (result)
253​ w->setPalette(d->normalPalette);
254​ else
255​ w->setPalette(d->warningPalette);
256​}
257​
258​void WireGuardAdvancedWidget::slotWidgetChanged()
259​{
260​ if (d->tableValid && d->fwMarkValid && d->presharedKeyValid)
261​ d->ui.buttonBox->button(QDialogButtonBox::Ok)->setEnabled(true);
262​ else
263​ d->ui.buttonBox->button(QDialogButtonBox::Ok)->setEnabled(false);
264​}