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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> autostart.cpp:182
> +    AddScriptDialog addDialog(this);
> +    if (addDialog.exec() != QDialog::Accepted) {
> +        return;

In preparation for a move to QML, can we please not `exec()`

> autostart.cpp:192
> +    QTreeWidgetItem* widgetItem = widget->listCMD->currentItem();
> +    if (!widgetItem)
>          return;

Coding style

> autostart.cpp:231
> +    Q_UNUSED(roles)
> +    Q_UNUSED(bottomRight)
> +

Don't always ignore those ranges, we want to handle this model stuff properly

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

`qobject_cast`

> autostart.cpp:261
> +        const QString fileName = m_model->data(index, AutostartModel::Roles::FileName).toString();
> +        KFileItem kfi = KFileItem(QUrl::fromLocalFile(fileName));
>          kfi.setDelayedMimeTypes(true);

`KFileItem kfi(QUrl::fromLocalFile(fileName));`

> autostart.cpp:49
>  Autostart::Autostart( QWidget* parent, const QVariantList& )
> -    : KCModule(parent )
> +    : KCModule(parent ), m_model(new AutostartModel())
>  {

Put on new line

> autostart.cpp:100
>          if ( entry ) {
> -            bool disable = ( item->checkState( col ) == Qt::Unchecked );
> -            KDesktopFile kc(entry->fileName().path());
> -            KConfigGroup grp = kc.desktopGroup();
> -            if ( grp.hasKey( "Hidden" ) && !disable) {
> -                grp.deleteEntry( "Hidden" );
> -            }
> +            bool enabled = ( item->checkState( col ) == Qt::Checked );
> +            m_model->setData(indexFromWidget(item), enabled, AutostartModel::Roles::Enabled);

`const bool`

> autostart.cpp:102
> +            m_model->setData(indexFromWidget(item), enabled, AutostartModel::Roles::Enabled);
> +            if ( enabled )
> +                item->setText( COL_STATUS, i18nc( "The program will be run", "Enabled" ) );

Coding style:

  if (enabled) {
      ...
  }

> autostart.cpp:210
> +
> +    QModelIndex idx = m_model->index(first, 0);
> +

You probably want to loop

  for (int i = first; i <= last; ++i) {

> autostart.h:64
> +    void slotRowInserted(const QModelIndex &parent, int first, int last);
> +    void slotDatachanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QVector<int> &roles);
>  

Capitalize `Changed`

> autostart.h:67
>  private:
> -    QTreeWidgetItem *m_programItem, *m_scriptItem;
> -    QString m_desktopPath;
> -    QStringList m_paths;
> -    QStringList m_pathName;
> +    QModelIndex indexFromWidget(QTreeWidgetItem *widget);
> +

`const`

> autostartmodel.cpp:176
> +
> +    //add scripts, skips first value
> +    for (int i = 1; i < listPath().length(); i++) {

but why?

> autostartmodel.cpp:261
> +
> +    int dirty = false;
> +    AutostartEntry &entry = m_entries[index.row()];

`bool`

> autostartmodel.cpp:291
> +            grp.deleteEntry( "Hidden" );
> +        }else {
> +            grp.writeEntry("Hidden", !entry.enabled);

Coding style: `} else {`

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

Why not just use `Qt::DisplayRole`?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  D26934

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/20200327/a503b980/attachment-0001.html>