D26401: KCM Baloo: Migrate to KConfigXT and add immutability

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> filteredfoldermodel.cpp:5
>   * Copyright (C) 2019 Tomaz Canabrava <tcanabrava at kde.org>
> - *
> +   Copyright (c) 2020 Benjamin Port <benjamin.port at enioka.com>
>   * This library is free software; you can redistribute it and/or

Missing the * and an empty line after that one.

> filteredfoldermodel.cpp:59
> +        : QAbstractListModel(parent)
> +        , m_settings(settings)
>  {

Indentation of that line and the previous one looks wrong... but surprisingly the previous line is not showing up as changed, I wonder if that's the review tool acting up.

> filteredfoldermodel.cpp:159
>      }
> -    beginResetModel();
> -    m_excludeList.append(QUrl(url).toLocalFile());
> -    std::sort(std::begin(m_excludeList), std::end(m_excludeList));
> -    endResetModel();
> -    Q_EMIT folderAdded();
> +    auto excluded = m_settings->excludedFolders();
> +    excluded.append(QUrl(url).toLocalFile());

Could have been the first line of that method, would avoid retrieving that list twice and make the if condition more readable.

> filteredfoldermodel.h:35
>  
> -    void setDirectoryList(const QStringList& includeDirs, const QStringList& exclude);
> +    void updateDirectoryList();
>      QStringList includeFolders() const;

Declare it as a slot for good measure (although it's technically not really necessary).

> kcm.cpp:66
> +    connect(m_settings, &BalooSettings::excludedFoldersChanged, this, [this]{ m_filteredFolderModel->updateDirectoryList(); });
> +    connect(m_settings, &BalooSettings::foldersChanged, this, [this]{ m_filteredFolderModel->updateDirectoryList(); });
>  }

Why not connect directly to updateDirectoryList in m_filteredFolderModel? Those lambdas look unnecessary (besides you'd want m_filteredFolderModel and not this as third parameter anyway).

Also you probably want to call updateDirectoryList() once just after those connects.

> kcm.cpp:77
> +    m_previouslyEnabled = m_settings->indexingEnabled();
> +    m_filteredFolderModel->updateDirectoryList();
>  }

I don't think this call is necessary (it's probably because of the missing call in the ctor).

> kcm.cpp:87
> +
> +    m_previouslyEnabled = m_settings->indexingEnabled();
>  

Why are you updating m_previouslyEnabled again? The old code wasn't doing this AFAICT.

> kcm.cpp:116
> +    ManagedConfigModule::defaults();
> +    m_filteredFolderModel->updateDirectoryList();
>  }

Same thing, I don't think this is necessary. defaults() can likely completely go away.

> kcm.cpp:133
> +
> +BalooSettings *ServerConfigModule::balooSettings() const {
> +    return m_settings;

The opening curly brace should be on its own line.

> main.qml:49
>              }
> +            enabled: !kcm.balooSettings.isImmutable("indexingEnabled")
>          }

nitpick: I'd put enabled before checked

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26401

To: bport, ervin, crossi, meven, #plasma
Cc: meven, crossi, ervin, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 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/20200106/4e2db2af/attachment-0001.html>