D26934: KCM/Autostart Add a model to separate logic from UI

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> autostart.cpp:87
>  
> +    setMinimumHeight(300);
> +    setMinimumWidth(400);

Unrelated right? Where do this come from?

> autostart.cpp:112
>  
> -void Autostart::addItem( DesktopStartItem* item, const QString& name, const QString& run, const QString& command, bool disabled )
> +void Autostart::setDesktopStartItem( DesktopStartItem* item, const QString &name, const QString &command, bool disabled, const QString &fileName)
>  {

Please remove the space after (, and space should be before * not after

Shouldn't that be called "updateDesktopStartItem" or similar? It's not setting anything on "this" after all, but changing the state of the item.

> autostart.cpp:183
>  {
>      AddScriptDialog * addDialog = new AddScriptDialog(this);
> +    if (addDialog->exec() != QDialog::Accepted) {

It'd be better to have this dialog on the stack, or reachable via a pointer available between calls to this slot. Currently it'd instantiate a new dialog each time this slot is invoked and it'd never be freed until the KCModule itself is freed.

> autostart.cpp:243
> +        QTreeWidgetItem *item = m_scriptItem->child(topLeft.row() - m_programItem->childCount());
> +        ScriptStartItem *scriptEntry = dynamic_cast<ScriptStartItem*>( item );
> +        setScriptStartItem(scriptEntry, name, command, source, fileName);

qobject_cast is deemed faster (since it turns out those items inherit from QObject which gives me the creeps somehow)

> autostart.cpp:250
> +        QTreeWidgetItem *item = m_programItem->child(topLeft.row());
> +        DesktopStartItem *desktopItem = dynamic_cast<DesktopStartItem*>( item );
> +        setDesktopStartItem(desktopItem, name, command, !enabled, fileName);

ditto

> autostart.cpp:254
> +
> +    // const int index = widget->listCMD->indexOfTopLevelItem(widgetItem->parent()) + widgetItem->parent()->indexOfChild(widgetItem);
> +}

I guess you didn't meant to commit this

> autostart.cpp:288
>          return;
> -    slotEditCMD( (AutoStartItem*)widget->listCMD->currentItem() );
> +    slotEditCMD( dynamic_cast<AutoStartItem*>(widget->listCMD->currentItem()) );
>  }

ditto

> autostart.h:48
>  public Q_SLOTS:
> -    void slotChangeStartup( ScriptStartItem* item, int index );
> +    void slotChangeStartup( ScriptStartItem *item, int index );
>  

Since you touched the line anyway you could as well remove the spaces between the parentheses

> autostartmodel.cpp:1
> +#include "autostartmodel.h"
> +

Copyright header missing

> autostartmodel.cpp:31
> +    // must match enum AutostartEntrySource
> +    const static QStringList s_paths = QStringList()
> +            << QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + QStringLiteral("/autostart/")

Urgh, no, use Q_GLOBAL_STATIC with an initializer

> autostartmodel.cpp:37
> +
> +    const QStringList s_pathName = QStringList() << i18n("Startup")
> +         << i18n("Logout")

Ditto

> autostartmodel.cpp:66
> +{
> +    this->beginResetModel();
> +

We don't do "this->"

> autostartmodel.cpp:74
> +
> +    autostartdir.setFilter( QDir::Files );
> +

Remove the spaces between the parentheses

> autostartmodel.cpp:76
> +
> +    for (QFileInfo fi : autostartdir.entryInfoList()) {
> +        QString filename = fi.fileName();

const auto &fi, and qAsConst (have no idea if the list is cached somewhere and thus could detach)

> autostartmodel.cpp:79
> +        bool desktopFile = filename.endsWith(QLatin1String(".desktop"));
> +        if ( desktopFile ) {
> +            AutostartEntry entry = loadDesktopEntry(fi.absoluteFilePath());

Drop the spaces

> autostartmodel.cpp:100
> +
> +        dir.setFilter( QDir::Files );
> +

Drop the spaces

> autostartmodel.cpp:102
> +
> +        for (QFileInfo fi : dir.entryInfoList()) {
> +

const auto &fi, and qAsConst (have no idea if the list is cached somewhere and thus could detach)

> autostartmodel.cpp:121
> +
> +    this->endResetModel();
> +}

s/this->//

> autostartmodel.cpp:126
> +{
> +    Q_UNUSED(parent)
> +

Should return 0 if the parent is valid

> autostartmodel.cpp:131
> +
> +bool AutostartModel::checkEntry(const AutostartEntry &entry)
> +{

Shouldn't that be at least const? Actually could probably even be static or in an anonymous namespace at the top of this file

> autostartmodel.cpp:145
> +
> +AutostartEntry AutostartModel::loadDesktopEntry(const QString &fileName)
> +{

Ditto

> autostartmodel.cpp:195
> +
> +    switch (role) {
> +    case DisplayRole : return entry.name;

at least Qt::DisplayRole should be handled

> autostartmodel.cpp:196
> +    switch (role) {
> +    case DisplayRole : return entry.name;
> +    case Command : return entry.command;

No space before :

> autostartmodel.cpp:217
> +    switch (role) {
> +    case DisplayRole : {
> +        if (entry.name == value.toString()) {

I think you meant Qt::EditRole here. Also no space before :

> autostartmodel.cpp:303
> +    if (dirty) {
> +        emit dataChanged(index, index, {role});
> +    }

There might be a trick in the case of Qt::EditRole since it'd have to notify also for Qt::DisplayRole in that case.

> autostartmodel.cpp:311
> +{
> +    emit beginInsertRows(QModelIndex(), m_entries.count(), m_entries.count());
> +

s/emit// (this is not a signal)

> autostartmodel.cpp:318
> +    // https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/923360
> +    if ( service->desktopEntryName().isEmpty() || service->entryPath().isEmpty()) {
> +        // create a new desktop file in s_desktopPath

Remove the space after (

> autostartmodel.cpp:351
> +    m_entries.push_back(entry);
> +    emit endInsertRows();
> +

s/emit//

> autostartmodel.cpp:358
> +{
> +    emit beginInsertRows(QModelIndex(), m_entries.count(), m_entries.count());
> +

s/emit//

> autostartmodel.cpp:390
> +     m_entries.push_back(entry);
> +     emit endInsertRows();
> +

s/emit//

> autostartmodel.cpp:395
> +
> +bool AutostartModel::removeEntry(const QModelIndex &index) {
> +

Break the line before {

> autostartmodel.cpp:402
> +    if (job->exec()) {
> +        emit beginRemoveRows(QModelIndex(), index.row(), index.row());
> +

s/emit//

> autostartmodel.cpp:406
> +
> +        emit endRemoveRows();
> +        return true;

s/emit//

> autostartmodel.cpp:415
> +    return {
> +        {DisplayRole, QByteArrayLiteral("display")},
> +        {Command, QByteArrayLiteral("command")},

Shouldn't be necessary. Also to have the default roles handled you generally call roleNames() of the parent and add your own entries.

> autostartmodel.h:1
> +#ifndef AUTOSTARTMODEL_H
> +#define AUTOSTARTMODEL_H

Copyright header missing

> autostartmodel.h:9
> +enum AutostartEntrySource {
> +    Xdg_AutoStart = 0,
> +    Xdg_Scripts = 1,

Shouldn't the underscores be dropped from those names?

> autostartmodel.h:15
> +
> +static AutostartEntrySource fromInt(int index) {
> +    switch (index) {

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.

> autostartmodel.h:25
> +
> +class AutostartEntry {
> +public:

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

> autostartmodel.h:41
> +public:
> +    AutostartModel();
> +

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

> autostartmodel.h:44
> +    enum Roles {
> +        DisplayRole,
> +        Command = Qt::UserRole + 1,

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

> autostartmodel.h:57
> +    int rowCount(const QModelIndex &parent = QModelIndex()) const override;
> +    QVariant data(const QModelIndex &index, int role) const override;
> +    bool setData(const QModelIndex &index, const QVariant &value, int role) override;

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

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

> autostartmodel.h:58
> +    QVariant data(const QModelIndex &index, int role) const override;
> +    bool setData(const QModelIndex &index, const QVariant &value, int role) override;
> +    QHash<int, QByteArray> roleNames() const override;

Likewise: `role = Qt::EditRole`

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200225/75186c9b/attachment-0001.html>