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.cpp:167
>  {
> -    KOpenWithDialog owdlg( this );
> -    if (owdlg.exec() != QDialog::Accepted)
> -        return;
> +    KOpenWithDialog* owdlg = new KOpenWithDialog(this);
> +    connect(owdlg, &QDialog::finished, this, [this, owdlg] (int result) {

Space before * not after, or just use auto

> autostart.cpp:186
>  {
> -    AddScriptDialog * addDialog = new AddScriptDialog(this);
> -    int result = addDialog->exec();
> -    if (result == QDialog::Accepted) {
> -        if (addDialog->symLink())
> -            KIO::link(addDialog->importUrl(), QUrl::fromLocalFile(m_paths[0]));
> -        else
> -            KIO::copy(addDialog->importUrl(), QUrl::fromLocalFile(m_paths[0]));
> -
> -        ScriptStartItem * item = new ScriptStartItem( m_paths[0] + addDialog->importUrl().fileName(), m_scriptItem,this );
> -        addItem( item,  addDialog->importUrl().fileName(), addDialog->importUrl().fileName(),ScriptStartItem::START );
> -    }
> -    delete addDialog;
> +    AddScriptDialog* addDialog = new AddScriptDialog(this);
> +    connect(addDialog, &QDialog::finished, this, [this, addDialog] (int result) {

ditto

> autostart.cpp:214
>  {
> +    Q_UNUSED(parent)
> +    Q_UNUSED(last)

Actually I think I'd Q_ASSERT(!parent.isValid())

> autostart.cpp:215
> +    Q_UNUSED(parent)
> +    Q_UNUSED(last)
>  

This Q_UNUSED is now used ;-)

> autostart.cpp:276
> +
> +        KPropertiesDialog* dlg = new KPropertiesDialog( kfi, this );
> +        connect(dlg, &QDialog::finished, this, [this, index, fileName, desktopItem, dlg] (int result) {

Space before * not after, or use auto
Also drop the spaces between the parentheses

> autostart.cpp:296
>  {
> -    if ( widget->listCMD->currentItem() == nullptr )
> +    if ( widget->listCMD->currentItem() == nullptr ) {
>          return;

Since you touched the line, please drop the spaces between the parentheses

> autostart.cpp:304
>  {
> -    if ( widget->listCMD->currentItem() == nullptr )
> +    if ( widget->listCMD->currentItem() == nullptr ) {
>          return;

Since you touched the line, please drop the spaces between the parentheses

> autostart.cpp:321
> +
> +QModelIndex Autostart::indexFromWidget(QTreeWidgetItem* widget) const {
> +    int index = m_programItem->indexOfChild(widget);

Since you touched the line please fix the * and { positions

> ervin wrote in autostart.h:64
> Still needs being addressed

Still needs being addressed. :-)

> autostartmodel.cpp:130
> +
> +AutostartModel::AutostartModel(QWidget* parent)
> +    : QAbstractListModel(parent)

Space before * not after

> ervin wrote in autostartmodel.cpp:177
> Makes me realize: are we sure it will always be the first entry? I'm not sure we can guarantee that over time.

Still needs being addressed.

> autostartmodel.h:49
> +public:
> +    explicit AutostartModel(QWidget* parent = nullptr);
> +

Since you touched the line, please drop the spaces between the parentheses

> autostartmodel.h:81
> +    QVector<AutostartEntry> m_entries;
> +    QWidget* m_window;
> +};

Space before * not after

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/20200408/c1cd3366/attachment-0001.html>