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