Differential D15093 Diff 42118 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 "ui_wireguardadvanced.h"
23​#include "nm-wireguard-service.h"
24​#include "settingwidget.h"
25​
26​class WireGuardAdvancedWidget::Private {
27​public:
pinoUnsubmitted
Done

unused

pino: unused
28​ NetworkManager::VpnSetting::Ptr setting;
29​ Ui::WireGuardAdvancedWidget ui;
30​};
31​
32​WireGuardAdvancedWidget::WireGuardAdvancedWidget(const NetworkManager::VpnSetting::Ptr &setting, QWidget *parent)
pinoUnsubmitted
Done

unused

pino: unused
33​ : QDialog(parent)
34​ , d(new Private)
35​{
36​ d->ui.setupUi(this);
37​
38​ setWindowTitle(i18nc("@title: window advanced wireguard properties", "Advanced WireGuard properties"));
39​
40​ d->setting = setting;
41​
42​ connect(d->ui.buttonBox, &QDialogButtonBox::accepted, this, &WireGuardAdvancedWidget::accept);
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
43​ connect(d->ui.buttonBox, &QDialogButtonBox::rejected, this, &WireGuardAdvancedWidget::reject);
44​
45​ d->ui.presharedKeyLineEdit->setPasswordModeEnabled(true);
46​
47​ QIntValidator *fwMarkValidator = new QIntValidator(nullptr);
48​ fwMarkValidator->setBottom(0);
49​ d->ui.fwMarkLineEdit->setValidator(fwMarkValidator);
jgrulichUnsubmitted
Not Done

Coding style, but can be fixed afterwards.

jgrulich: Coding style, but can be fixed afterwards.
50​
51​ KAcceleratorManager::manage(this);
52​
53​ if (d->setting) {
54​ loadConfig();
55​ }
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​
58​WireGuardAdvancedWidget::~WireGuardAdvancedWidget()
59​{
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
60​ delete d;
61​}
62​
63​void WireGuardAdvancedWidget::loadConfig()
64​{
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
65​ const NMStringMap dataMap = d->setting->data();
66​
67​ if (dataMap.contains(QLatin1String(NM_WG_KEY_LISTEN_PORT)))
68​ d->ui.listenPortSpinBox->setValue(dataMap[NM_WG_KEY_LISTEN_PORT].toUInt());
69​ else
70​ d->ui.listenPortSpinBox->setValue(0);
71​
72​ if (dataMap.contains(QLatin1String(NM_WG_KEY_MTU)))
73​ d->ui.mtuSpinBox->setValue(dataMap[NM_WG_KEY_MTU].toUInt());
74​ else
75​ d->ui.mtuSpinBox->setValue(0);
76​
77​ if (dataMap.contains(QLatin1String(NM_WG_KEY_TABLE)))
78​ d->ui.tableLineEdit->setText(dataMap[NM_WG_KEY_TABLE]);
79​ else
80​ d->ui.tableLineEdit->clear();
81​
82​ if (dataMap.contains(QLatin1String(NM_WG_KEY_FWMARK)))
83​ d->ui.fwMarkLineEdit->setText(dataMap[NM_WG_KEY_FWMARK]);
84​ else
85​ d->ui.fwMarkLineEdit->clear();
86​
87​ if (dataMap.contains(QLatin1String(NM_WG_KEY_PRESHARED_KEY)))
88​ d->ui.presharedKeyLineEdit->setText(dataMap[NM_WG_KEY_PRESHARED_KEY]);
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​ else
90​ d->ui.presharedKeyLineEdit->setText(QString());
91​}
92​
93​void WireGuardAdvancedWidget::setProperty(NMStringMap &data, const QLatin1String &key, const QString &value) const
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​{
95​ if (!value.isEmpty())
96​ data.insert(key, value);
97​}
98​
pinoUnsubmitted
Done

QString(), instead of ""

pino: `QString()`, instead of `""`
99​void WireGuardAdvancedWidget::setProperty(NMStringMap &data, const QLatin1String &key, const int value) const
100​{
101​ if (value > 0)
102​ data.insert(key, QString::number(value));
103​}
104​
105​NetworkManager::VpnSetting::Ptr WireGuardAdvancedWidget::setting() const
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​{
107​ NMStringMap data;
108​ QString stringVal;
109​
110​ setProperty(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), d->ui.listenPortSpinBox->value());
111​ setProperty(data, QLatin1String(NM_WG_KEY_MTU), d->ui.mtuSpinBox->value());
112​ setProperty(data, QLatin1String(NM_WG_KEY_TABLE), d->ui.tableLineEdit->displayText());
113​ setProperty(data, QLatin1String(NM_WG_KEY_FWMARK), d->ui.fwMarkLineEdit->displayText());
114​ setProperty(data, QLatin1String(NM_WG_KEY_PRESHARED_KEY), d->ui.presharedKeyLineEdit->text());
115​
116​ d->setting->setData(data);
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​ return d->setting;
119​}
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…
pinoUnsubmitted
Done

unused

pino: unused
pinoUnsubmitted
Done

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

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