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