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 <tcanabr...@kde.org>
> - *
> + Copyright (c) 2020 Benjamin Port <benjamin.p...@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
