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

[Kevin Ottens]

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