pino added a comment.
Hi Enrique,
I'm not a plasma-nm developer, however I provide some tips & hints regarding
your patch.
Other than what I noted already, there are few more things that apply in
general:
- make sure to respect the indentation: each level by 4 spaces with no tabs,
space after a comma for function arguments, etc
- make sure to use curly brackets {...} also for blocks with only 1 line
- nullptr instead of NULL
INLINE COMMENTS
> openconnectauth.cpp:60-66
> +#if !OPENCONNECT_CHECK_VER(2,1)
> +#define __openconnect_set_token_mode(...) -EOPNOTSUPP
> +#elif !OPENCONNECT_CHECK_VER(2,2)
> +#define __openconnect_set_token_mode(vpninfo, mode, secret)
> openconnect_set_stoken_mode(vpninfo, 1, secret)
> +#else
> +#define __openconnect_set_token_mode openconnect_set_token_mode
> +#endif
IMHO it is better to define the macros in the same way for all the cases,
specifying their parameters
> openconnectauth.cpp:225
> + d->tokenMode = dataMap[NM_OPENCONNECT_KEY_TOKEN_MODE].toUtf8();
> +
> }
extra empty line
> openconnectauth.cpp:279
> +
> + QByteArray tokenSecret =
> d->secrets[NM_OPENCONNECT_KEY_TOKEN_SECRET].toUtf8();
> + if (!d->tokenMode.isEmpty()) {
this is needed only in the block within the if in the line below, so move it
inside that block; also, make tokenSecret const
> openconnectauth.cpp:298
> + d->token.tokenMode = OC_TOKEN_MODE_YUBIOATH;
> + if (!tokenSecret.isEmpty() && tokenSecret.length())
> + d->token.tokenSecret = tokenSecret;
if tokenSecret is not empty, then its length will always be greater than 0, so
the second condition is redundant
> openconnectauth.cpp:301
> + else
> + d->token.tokenSecret = NULL;
> + }
d->token.tokenSecret is a QByteArray, so if you want to clear it just call
clear()
> openconnectauth.cpp:305
> + if (ret) {
> + addFormInfo(QLatin1String("dialog-error"), i18n("Failed to
> initialize software token: %1\n", ret));
> + }
it looks like all the other messages for addFormInfo() do not have a trailing
newline, so please remove it
> openconnectauth.cpp:360
> addFormInfo(QLatin1String("dialog-information"), i18n("Contacting host,
> please wait..."));
> +
> d->worker->start();
extra empty line
> openconnectauth.cpp:374
> {
> +
> Q_D(const OpenconnectAuthWidget);
extra empty line
> openconnectauth.cpp:380
>
> +
> secrets.unite(d->secrets);
extra empty line
> openconnectauth.h:58
> void processAuthForm(struct oc_auth_form *);
> - void updateLog(const QString &, const int &);
> + void updateLog(const QString&, const int&);
> void logLevelChanged(int);
unrelated change
> openconnectauth.h:66
> + void initTokens();
> +
> };
extra empty line
> openconnectauthworkerthread.h:95
> + void initTokens(void);
> +
> protected:
extra empty line
> openconnectprop.ui:181-182
> + <property name="text">
> + <string>Prevent the user from manually accepting
> +invalid certificates</string>
> + </property>
this seems a bit too long as label for a checkbox
> openconnectprop.ui:208
> + <property name="text">
> + <string>Tokens</string>
> + </property>
usually buttons are actions, so IMHO "Show Tokens" is more appropriate
> openconnecttoken.ui:13-15
> + <property name="windowTitle">
> + <string>Form</string>
> + </property>
remove these lines (or invoke on this .ui file the fixuifiles script available
in kde-dev-scripts)
> openconnecttoken.ui:85
> + <property name="text">
> + <string>Yubikey OATH</string>
> + </property>
"YubiKey"; also, is "OATH" correct?
> openconnectwidget.cpp:48
> +
> +#include <QStringList>
> +
this include should be placed together with the other Qt includes some lines
above
> openconnectwidget.cpp:56
> NetworkManager::VpnSetting::Ptr setting;
> + mutable QStringList tokenModeList;
> + QDialog *tokenDlg;
are you sure it needs to be mutable?
> openconnectwidget.cpp:78
> +
> +
> + d->tokenDlg = new QDialog(this);
extra empty line
> openconnectwidget.cpp:80
> + d->tokenDlg = new QDialog(this);
> + d->tokenWid = new QWidget(this);
> + d->tokenUi.setupUi(d->tokenWid);
tokenWid is not needed, you can just setupUi on the dialog
> openconnectwidget.cpp:85
> + d->tokenDlg->setLayout(layout);
> + QDialogButtonBox* buttons = new
> QDialogButtonBox(QDialogButtonBox::Ok|QDialogButtonBox::Cancel, d->tokenDlg);
> + connect(buttons, &QDialogButtonBox::accepted, d->tokenDlg,
> &QDialog::accept);
the button box can be added directly to the ui file, and avoid the need to add
it manually
> openconnectwidget.cpp:122-136
> + if (combo->itemText(i).startsWith("RSA") &&
> !openconnect_has_stoken_support ()) {
> + combo->removeItem(i);
> + d->tokenModeList.removeAt(i);
> + }
> + else if (combo->itemText(i).startsWith("TOTP") &&
> !openconnect_has_oath_support ()) {
> + combo->removeItem(i);
> + d->tokenModeList.removeAt(i);
never ever compare to ui strings! they are translatable, so these checks break
when using a translation;
a better way is to use the itemData mechanism of a combobox to associate
arbitrary stuff (better some values of an enum) to easily get back the
information on the selected item;
also, considering that the items are added statically in the ui file and here
removed at runtime depending on the available modes, then IMHO it would be
better to directly only add the supported modes at runtime here
> openconnectwidget.cpp:150
> const NMStringMap dataMap = d->setting->data();
> +
>
extra empty line
> openconnectwidget.cpp:163
> +
> + int index =
> d->tokenModeList.indexOf(dataMap[NM_OPENCONNECT_KEY_TOKEN_MODE]);
> + if (index > 0) {
const
> openconnectwidget.cpp:241
> + d->tokenDlg->show();
> +}
> bool OpenconnectSettingWidget::isValid() const
missing empty line after this
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D18394
To: enriquem, jgrulich
Cc: pino, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
