ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> autostart.h:52
> + void updateDesktopStartItem(DesktopStartItem *item, const QString &name,
> const QString &command, bool disabled, const QString &fileName);
> + void updateScriptStartItem(ScriptStartItem *item, const QString &name,
> const QString& command, AutostartEntrySource type, const QString &fileName);
>
& is misplaced for command
> autostart.h:67
> private:
> - QTreeWidgetItem *m_programItem, *m_scriptItem;
> - QString m_desktopPath;
> - QStringList m_paths;
> - QStringList m_pathName;
> + QModelIndex indexFromWidget(QTreeWidgetItem* widget);
> +
- is misplaced
> autostartitem.cpp:57
> + for (int i = 0; i < AutostartModel::listPathName().size(); i++) {
> + m_comboBoxStartup->addItem( AutostartModel::listPathName()[i], i + 1
> /* +1 to skip first path that is not selectable for scripts */);
> + }
This is not obvious at all even with the comment... I guess you expect
listPathName indices to almost line up with the enum values? That sounds very
fragile as well then. Looks like it needs to be reworked a bit. Maybe listing
the path names isn't enough you need to provide some extra context from the
model.
> autostartmodel.cpp:56
> + {
> +
> this->append(QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation)
> + QStringLiteral("/autostart/"));
> +
> this->append(QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation)
> + QStringLiteral("/autostart-scripts/"));
We don't do "this->"
Beside I find using Q_GLOBAL_STATIC_WITH_ARGS and a factory function more
readable and less boilerplaty than inheritance like you did.
It'd give something like:
QStringList autostartPaths()
{
return {
QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) +
QStringLiteral("/autostart/"),
...
};
}
Q_GLOBAL_STATIC_WITH_ARGS(QStringList, s_paths, autostartPaths())
> autostartmodel.cpp:69
> + {
> + this->append(i18n("Startup"));
> + this->append(i18n("Logout"));
ditto
> autostartmodel.cpp:101
> + const bool enabled = !(hidden ||
> + notShowList.contains(QLatin1String("KDE")) ||
> + (!onlyShowList.isEmpty() &&
> !onlyShowList.contains(QLatin1String("KDE"))));
is it me or indentation is wrong here? I'd expect one less space (same on the
line below)
> autostartmodel.cpp:114
> + onlyInPlasma
> + });
> +}
Indentation seems off here as well, Note that the AutostartEntry() here is
merely redundant. I think I'd write it like this:
return {
name,
command,
....
};
> autostartmodel.cpp:343
> + if (role == Qt::EditRole) {
> + emit dataChanged(index, index, {DisplayRole});
> + }
I think I'd avoid emitting twice but have one or two roles as third parameter.
Also, since I don't know how anyone else being that careful of specifying the
dirty roles in dataChanged there's still the valid option of just emitting
dataChanged with only two parameters.
> autostartmodel.cpp:421
> +
> + AutostartEntry entry = AutostartEntry{
> + destinationScript.fileName(),
I'd drop the redundant AutostartEntry (or use auto). Also could be const'd.
> autostartmodel.h:28
> + XdgAutoStart = 0,
> + XdgScripts= 1,
> + PlasmaShutdown = 2,
Missing space before =
> autostartmodel.h:60
> +
> + static AutostartEntrySource sourceFromInt(int index)
> + {
I'd still move the implementation in the cpp file. Makes sense to keep static
now though.
> autostartmodel.h:68
> + }
> + return XdgAutoStart;
> + }
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).
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D26934
To: meven, mlaurent, ervin, #plasma, broulik, bport, crossi
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh,
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
