ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed.
See inline comments. Also functionality-wise there are some problems: 1. When a new entry is added, if the KCM is closed and re-opened, the new entry has been forgotten 2. When you add a path, the ugly URL schema bit doesn't get stripped off like it should: F7556828: Screenshot_20191009_090737.png <https://phabricator.kde.org/F7556828> INLINE COMMENTS > filteredfoldermodel.cpp:147 > + > + Q_FOREACH (const QString& mount, m_mountPoints) { > + if (url.startsWith(mount) && mount.size() > mountPoint.size()) Don't use Q_FOREACH in new or ported code > main.qml:23 > +import QtQuick.Layouts 1.1 > +import QtQuick.Controls 2.12 as QQC2 > +import QtQuick.Dialogs 1.2 as QtDialogs IIRC plasma isn't depending on Qt 5.12 yet, right? > main.qml:39 > + QQC2.Label { > + text: i18n("File Search helps you quickly locate all your files > based on their content") > + } This is a sentence, so it needs to end with a period. > main.qml:47 > + onCheckStateChanged: { > + if (!checkState === Qt.Unchecked) { > + indexFileContents.checked = false; This can be simplified to just `if (!checked)` > main.qml:50 > + } > + kcm.indexing = fileSearchEnabled.checked > + } Don't need to reference it with `fileSearchEnabled`, just do `kcm.indexing = checked` > main.qml:53 > + > + Component.onCompleted : { > + fileSearchEnabled.checked = kcm.indexing ? Qt.Checked : > Qt.Unchecked Don't do it this way, just use a binding: `checked: kcm.indexing` > main.qml:69 > + indexFileContents.checked = kcm.fileContents ? Qt.Checked : > Qt.Unchecked > + } > + } Same comments as with the first checkbox > main.qml:71 > + } > + > + QQC2.Label { I would add some spacing between the last checkbox and the list view's explanatory label: Item { Layout.preferredHeight: Kirigami.Units.gridUnit } > GB_2 wrote in main.qml:2 > Shouldn't this be you copyright? Not done yet > nicolasfella wrote in metadata.desktop:3 > That comment sounds wrong :) Not done yet REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D23718 To: tcanabrava, #plasma, mart, ngraham Cc: GB_2, nicolasfella, mart, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
