D28025: [Baloo KCM] Complete overhaul of the include/exclude folder list

[Nathaniel Graham]

ngraham added a comment.


  Ok, will do. I just have a few comments about the backend bits, just minor stuff. Overall this is really good! I plan to polish the UI after this lands, but left comments on the front-end component anyway so you can sharpen your QML skills (which are quite good already). :) Feel free to consider them more educational than actionable. :)

INLINE COMMENTS

> filteredfoldermodel.cpp:36
>  namespace {
> +    QString normalizeTrailingSlashes(QString&& input) {
> +        if (!input.endsWith('/'))

this handy little function feels like it wants to be in KCoreAddons or something

> filteredfoldermodel.cpp:47
> +            if (!str.endsWith('/'))
> +                str += QLatin1Char('/');
>          }

Could probably just call `normalizeTrailingSlashes()` here

> filteredfoldermodel.cpp:83
> +        if (displayName.startsWith(homePath))
> +            displayName.replace(0, homePath.length(), QStringLiteral("~/"));
>  

use braces for single-line if statements here, as done immediately below

> filteredfoldermodel.cpp:208
> +    auto excluded = addTrailingSlashes(m_settings->excludedFolders());
> +    auto included = addTrailingSlashes(m_settings->folders());
> +    if (entry.enableIndex) {

There's a lot of use of `auto`  in this file for simple `QStringList` objects where I think it would be better to just declare the type

> main.qml:100
>  
>          RowLayout {
> +            Layout.alignment: Qt.AlignRight

Since this layout now only has one item in it, there's no longer a need for a layout at all

> main.qml:112
> +
> +    Component {
> +        id: directoryConfigDelegate

Since this is only used once, it could simply be declared inline

  delegate: Kirigami.SwipeListItem {
      id: listItem
      onClicked: {
      [blabla]
  }

> main.qml:122
> +
> +            RowLayout {
> +                Kirigami.Icon {

set spacing: units.smallSpacing on the layout instead of adding an empty item between objects for spacing

> main.qml:133
> +                ColumnLayout {
> +                    RowLayout {
> +                        Kirigami.Icon {

ditto

REPOSITORY
  R119 Plasma Desktop

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

To: bruns, #baloo, #vdg, ngraham, mart
Cc: 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/20200323/f34fc590/attachment-0001.html>