Differential D15093 Diff 41241 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​#include <QStandardPaths>
27​#include <QUrl>
pinoUnsubmitted
Done

unused

pino: unused
28​#include <QComboBox>
29​
30​#include <KLocalizedString>
31​#include <KProcess>
32​#include <KAcceleratorManager>
pinoUnsubmitted
Done

unused

pino: unused
33​
34​class WireGuardAdvancedWidget::Private {
35​public:
36​ NetworkManager::VpnSetting::Ptr setting;
37​};
38​
39​WireGuardAdvancedWidget::WireGuardAdvancedWidget(const NetworkManager::VpnSetting::Ptr &setting, QWidget *parent)
40​ : QDialog(parent)
41​ , m_ui(new Ui::WireGuardAdvancedWidget)
42​ , d(new Private)
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
43​{
44​ m_ui->setupUi(this);
45​
46​ setWindowTitle(i18nc("@title: window advanced wireguard properties", "Advanced WireGuard properties"));
47​
48​ d->setting = setting;
49​
jgrulichUnsubmitted
Not Done

Coding style, but can be fixed afterwards.

jgrulich: Coding style, but can be fixed afterwards.
50​ connect(m_ui->buttonBox, &QDialogButtonBox::accepted, this, &WireGuardAdvancedWidget::accept);
51​ connect(m_ui->buttonBox, &QDialogButtonBox::rejected, this, &WireGuardAdvancedWidget::reject);
52​
53​ m_ui->presharedKeyLineEdit->setPasswordModeEnabled(true);
54​
55​ QIntValidator *fwMarkValidator = new QIntValidator(0, 4294967295, nullptr);
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…
56​ m_ui->fwMarkLineEdit->setValidator(fwMarkValidator);
57​
58​ KAcceleratorManager::manage(this);
59​
jgrulichUnsubmitted
Done

Coding style

jgrulich: Coding style
60​ if (d->setting) {
61​ loadConfig();
62​ }
63​}
64​
jgrulichUnsubmitted
Done

Coding style

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

QString(), instead of ""

pino: `QString()`, instead of `""`
98​}
99​
100​void WireGuardAdvancedWidget::setOrClear(NMStringMap &data, QLatin1String key, QString value) const
101​{
102​ if (!value.isEmpty())
103​ data.insert(key, value);
104​ else
105​ data.remove(key);
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​
108​NetworkManager::VpnSetting::Ptr WireGuardAdvancedWidget::setting() const
109​{
110​ NMStringMap data;
111​ long intVal;
112​ QString stringVal;
113​
114​ intVal = m_ui->listenPortSpinBox->value();
115​ stringVal = (intVal > 0) ? QString::number(intVal) : QString("");
116​ setOrClear(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), stringVal);
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…
117​
118​ intVal = m_ui->mtuSpinBox->value();
119​ stringVal = (intVal > 0) ? QString::number(intVal) : QString("");
120​ setOrClear(data, QLatin1String(NM_WG_KEY_MTU), stringVal);
pinoUnsubmitted
Done

ditto

pino: ditto
121​
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​ setOrClear(data, QLatin1String(NM_WG_KEY_TABLE), m_ui->tableLineEdit->displayText());
pinoUnsubmitted
Done

unused

pino: unused
123​ setOrClear(data, QLatin1String(NM_WG_KEY_FWMARK), m_ui->fwMarkLineEdit->displayText());
124​ setOrClear(data, QLatin1String(NM_WG_KEY_PRESHARED_KEY), m_ui->presharedKeyLineEdit->text());
125​
126​ d->setting->setData(data);
127​
128​ return d->setting;
pinoUnsubmitted
Done

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

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