Differential D26934 Diff 82533 kcms/autostart/autostartmodel.h

Changeset View

Standalone View

View Options

kcms/autostart/autostartmodel.h

  • This file was added.
1​/***************************************************************************
2​ * Copyright (C) 2020 by Méven Car <meven.car@enioka.com> *
ervinUnsubmitted
Done

Copyright header missing

ervin: Copyright header missing
3​ * *
4​ * This program is free software; you can redistribute it and/or modify *
5​ * it under the terms of the GNU General Public License as published by *
6​ * the Free Software Foundation; either version 2 of the License, or *
7​ * (at your option) any later version. *
8​ * *
9​ * This program is distributed in the hope that it will be useful, *
10​ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
ervinUnsubmitted
Done

Shouldn't the underscores be dropped from those names?

ervin: Shouldn't the underscores be dropped from those names?
11​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
12​ * GNU General Public License for more details. *
13​ * *
14​ * You should have received a copy of the GNU General Public License *
15​ * along with this program; if not, write to the *
16​ * Free Software Foundation, Inc., *
ervinUnsubmitted
Done

I'd give it a less generic name or move it in a namespace this is an ambiguous call waiting to happen. :-)

Also I'd move the implementation in the cpp file and drop the static.

Finally, are you sure this is needed at all? I mean this is mostly about implicit conversion except that you provide a fallback in case of a missing value.

If that's really needed I'd simplify the implementation by checking the index is between 0 and the last value of the enum in which case you can return it right away or otherwise return Xdg_Autostart.

ervin: I'd give it a less generic name or move it in a namespace this is an ambiguous call waiting to…
mevenAuthorUnsubmitted
Done

I'd give it a less generic name or move it in a namespace this is an ambiguous call waiting to happen. :-)

I renamed it to AutostartModel::sourceFromInt

Also I'd move the implementation in the cpp file and drop the static.

I am sure of the point.

Finally, are you sure this is needed at all? I mean this is mostly about implicit conversion except that you provide a fallback in case of a missing value.
If that's really needed I'd simplify the implementation by checking the index is between 0 and the last value of the enum in which case you can return it right away or otherwise return Xdg_Autostart.

It makes the implementation quite explicit IMO, cannot return bad values, and can be enum-case checked by the compiler.
Checking against a range is less maintainable IMO.

(This is inspired by idiomatic rust, I am afraid)

meven: > I'd give it a less generic name or move it in a namespace this is an ambiguous call waiting…
17​ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA *
18​ ***************************************************************************/
19​#ifndef AUTOSTARTMODEL_H
20​#define AUTOSTARTMODEL_H
21​
22​#include <QAbstractListModel>
23​
24​#include <KService>
25​
26​enum AutostartEntrySource {
ervinUnsubmitted
Done

Break the line before the opening brace, also you could turn it into a struct and drop the "public:"

ervin: Break the line before the opening brace, also you could turn it into a struct and drop the…
27​ XdgAutoStart = 0,
28​ XdgScripts = 1,
29​ PlasmaShutdown = 2,
ervinUnsubmitted
Done

Missing space before =

ervin: Missing space before =
30​ PlasmaStart = 3,
31​};
32​
33​struct AutostartEntry
34​{
35​ QString name; // Human readable name or local script name
36​ QString command; // exec or original .sh file
37​ AutostartEntrySource source;
38​ bool enabled;
39​ QString fileName; // the file backing the entry
40​ bool onlyInPlasma;
41​};
42​Q_DECLARE_TYPEINFO(AutostartEntry, Q_MOVABLE_TYPE);
ervinUnsubmitted
Done

Shouldn't that take an optional parent? (and thus be made explicit)

ervin: Shouldn't that take an optional parent? (and thus be made explicit)
43​
44​class AutostartModel : public QAbstractListModel
45​{
ervinUnsubmitted
Done

Huh? Is it meant to be equivalent to Qt::DisplayRole? Shouldn't it have the corresponding value then?

ervin: Huh? Is it meant to be equivalent to Qt::DisplayRole? Shouldn't it have the corresponding value…
46​ Q_OBJECT
47​
48​public:
49​ explicit AutostartModel(QWidget *parent = nullptr);
50​
ervinUnsubmitted
Done

Since you touched the line, please drop the spaces between the parentheses

ervin: Since you touched the line, please drop the spaces between the parentheses
51​ enum Roles {
52​ Command = Qt::UserRole + 1,
53​ Enabled,
broulikUnsubmitted
Done

Why not just use Qt::DisplayRole?

broulik: Why not just use `Qt::DisplayRole`?
mevenAuthorUnsubmitted
Done

Not sure what you mean.

meven: Not sure what you mean.
broulikUnsubmitted
Done

Why define DisplayRole as dedicated enum entry, rather than just using Qt::DisplayRole in the code everywhere

broulik: Why define `DisplayRole` as dedicated enum entry, rather than just using `Qt::DisplayRole` in…
mevenAuthorUnsubmitted
Done

I see, I meant my code to have all Roles used explicitly declared here rather than relying on the developer knowing about Qt::DisplayRole.

meven: I see, I meant my code to have all Roles used explicitly declared here rather than relying on…
ervinUnsubmitted
Done

Weeell... knowing about Qt::DisplayRole is kind of prerequisite to making your own model. :-)

ervin: Weeell... knowing about Qt::DisplayRole is kind of prerequisite to making your own model. :-)
mevenAuthorUnsubmitted
Not Done

As you want.
IMO this is not a good pattern to expect reviewers to have any previous knowledge about enum values, especially for new contributors (I was concerned by this when I started reading QAbstractModel implementations)
Better to teach them those values by making things explicit. It also helps when using an IDE for discoverability.

meven: As you want. IMO this is not a good pattern to expect reviewers to have any previous knowledge…
54​ Source,
55​ FileName,
56​ OnlyInPlasma
57​ };
58​
ervinUnsubmitted
Done

To really fulfill the contract: role = Qt::DisplayRole

And yes you wish override would check for that but it doesn't...

ervin: To really fulfill the contract: `role = Qt::DisplayRole` And yes you wish override would check…
59​ static AutostartEntrySource sourceFromInt(int index);
ervinUnsubmitted
Done

Likewise: role = Qt::EditRole

ervin: Likewise: `role = Qt::EditRole`
60​ static QString XdgAutoStartPath();
61​ static QStringList listPath();
ervinUnsubmitted
Done

I'd still move the implementation in the cpp file. Makes sense to keep static now though.

ervin: I'd still move the implementation in the cpp file. Makes sense to keep static now though.
62​ static QStringList listPathName();
63​
64​ int rowCount(const QModelIndex &parent = QModelIndex()) const override;
65​ QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override;
66​ bool setData(const QModelIndex &index, const QVariant &value, int role = Qt::EditRole) override;
67​ QHash<int, QByteArray> roleNames() const override;
68​
69​ bool addEntry(const KService::Ptr &service);
ervinUnsubmitted
Done

I still don't buy the safety argument for not changing this implementation which is very not "DRY".

index is an int so the argument of the enum-checked by the compiler is completely moot, it will never be checked (some compilers might even give a warning because of the lack of default in the switch which means it doesn't exhaust all possible values for index).
The cannot return "bad value" argument is debatable it means than an infinity of values (from int perspective which is obviously not really all natural numbers) will fallback to XdgAutostart

So it's really a conversion with no real check, whatever the way you paint it. I'd advocate making it look like what it is and with less repetition.

From your description I think I'd even go for the implementation I advocated earlier + Q_UNREACHABLE before the fallback return (or a Q_ASSERT about index being in the expected range, if I understood you correctly we're not supposed to hit that return ever).

ervin: I still don't buy the safety argument for not changing this implementation which is very not…
70​
71​ bool addEntry(const QUrl &path, const bool symlink);
72​
73​ bool reloadEntry(const QModelIndex &index, const QString &fileName);
74​ bool removeEntry(const QModelIndex &index);
75​
76​ void load();
77​
78​private:
79​ QVector<AutostartEntry> m_entries;
80​ QWidget *m_window;
81​};
82​
ervinUnsubmitted
Done

Space before * not after

ervin: Space before * not after
83​#endif // AUTOSTARTMODEL_H