Differential D28025

[Baloo KCM] Complete overhaul of the include/exclude folder list
ClosedPublic
Actions

Authored by bruns on Mar 13 2020, 2:08 PM.

Details

Reviewers
ngraham
mart
Group Reviewers
Baloo
VDG
Commits
R119:7498b41a1979: [Baloo KCM] Complete overhaul of the include/exclude folder list
Summary

The current "Excluded folders" list in the KCM is quite awkward:

  1. It tries to mimic baloos automatic exclusion of external drives, but fails doing so and adds almost any external drive even when not mounted below an indexable path.
  1. Deleting an autogenerated entry actually adds it to the included folder list, and then hides it.
  1. There is no way to show the included folder list, or add any entries to it.

Remove the custom "excluded mounts" heuristic from the KCM and retrieve
the additional (not explicitly configured) excluded ones from baloo.

Replace the "excluded list" with a common list for included and excluded
folders, and flag its state. This also makes it easy to add additional
properties later.

Create a new UI delegate for each config list item, allowing to enable
and disable indexing for each entry. Make the "delete" actually always
delete a config entry, and make the control inline. Move the "Add" button
to the *right* bottom of the list (in accordance with UI guidelines) and
add some text to it.

Depends on D28024

Solves the following parts of T9879:

  • Allow adding of includeFolders in the KCM
  • Correctly show excludeFolders below explicit (non-$HOME) includeFolders in KCM

CCBUG: 417170
CCBUG: 366521
BUG: 417763
BUG: 417762

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bruns created this revision.Mar 13 2020, 2:08 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 13 2020, 2:08 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bruns requested review of this revision.Mar 13 2020, 2:08 PM
bruns added inline comments.Mar 13 2020, 3:46 PM
kcms/baloo/package/contents/ui/main.qml
176

Spurious tab

bruns marked an inline comment as done.Mar 13 2020, 3:46 PM
ngraham added a comment.Mar 13 2020, 4:16 PM

Looks interesting, will take a look soon.

ngraham requested changes to this revision.Mar 19 2020, 6:38 PM
ngraham added a reviewer: mart.

I have some UI suggestions:

  • Have a button to add an exclusion path as well as a button to add an inclusion path, rather than a single Add Setting button, which is a rather jargony, programmer-centric way of presenting the feature.
  • For consistency, use the typical way of assigning actions to Kirigami SwipeListItems, rather than implementing custom button appearance and behavior. If you did this because the Kirigami SwipeListItem has no provision to display an inline action with text as well as an icon, let's change the component to support that.
  • Instead of having the list item expand when clicked to reveal whether it's included or excluded, display that information in textual form in the same line, and no need to repeat the same path. This would optionally allow you to remove the magnifying glass and minus sign icons.
This revision now requires changes to proceed.Mar 19 2020, 6:38 PM
bruns added a comment.Mar 19 2020, 8:58 PM
In D28025#630729, @ngraham wrote:

I have some UI suggestions:

  • Have a button to add an exclusion path as well as a button to add an inclusion path, rather than a single Add Setting button, which is a rather jargony, programmer-centric way of presenting the feature.

Having only inclusion/exclusion is a temporary state. I plan to add more settings for each path, so having an "add" button for each possible state will no longer be feasible.

Also, "removing" a path from the "included" list is not the same as excluding it - the state depends on the state of its next ancestor. This exact type of mixup has led to the messy state the current KCM is in.

  • For consistency, use the typical way of assigning actions to Kirigami SwipeListItems, rather than implementing custom button appearance and behavior. If you did this because the Kirigami SwipeListItem has no provision to display an inline action with text as well as an icon, let's change the component to support that.

The Items are inspired by the Desktop Effects KCM. I have searched through the HIG for considerable time, unfortunately it lacks any specific information what to do, or any usable examples. If you can provide any examples where you think it is done the "right way (TM)", please go for it.

  • Instead of having the list item expand when clicked to reveal whether it's included or excluded, display that information in textual form in the same line, and no need to repeat the same path. This would optionally allow you to remove the magnifying glass and minus sign icons.

The search/excluded icons are just the first implemented state column. There will be more columns. Having the full state in textual form for each entry will look awkward, and having it in textual form only will make it much harder to get the current state for a given path.

More fine granular settings have been requested several times. Having a content indexer run on ~/Downloads poses a security risk. Running full-text indexing on ~/Documents/MyCppProjects/ is definitely subject to a users preference, while most users would expect ~/Pictures/, ~/Videos/ and ~/Music/ to be scanned for metadata.

The current model and visual representation are complete nonsense, from a programmers as well as a users view. This definitely gets the model in a usable and extensible state, and shows the real state to the user (instead of showing some invented entries, and leaving out the other real half), and also makes it configurable. The important part here is the model. The view/delegate can be extended even by some person who is not familiar with baloo internals.

ngraham added a comment.EditedMar 19 2020, 10:20 PM
In D28025#630849, @bruns wrote:

Having only inclusion/exclusion is a temporary state. I plan to add more settings for each path, so having an "add" button for each possible state will no longer be feasible.

Also, "removing" a path from the "included" list is not the same as excluding it - the state depends on the state of its next ancestor. This exact type of mixup has led to the messy state the current KCM is in.

Then maybe we should rethink the UI, because that's what it currently suggests. What are the other states you're planning to add?

The Items are inspired by the Desktop Effects KCM. I have searched through the HIG for considerable time, unfortunately it lacks any specific information what to do, or any usable examples. If you can provide any examples where you think it is done the "right way (TM)", please go for it.

Yeah, we need to add more examples and better guidance to the HIG for sure. However you must be looking at an old version of the Desktop Effects KCM because the git master version shows what I'm talking about:

You can also look at the Desktop effects KCM, the Activities KCM, or Discover's Settings page.

The search/excluded icons are just the first implemented state column. There will be more columns. Having the full state in textual form for each entry will look awkward, and having it in textual form only will make it much harder to get the current state for a given path.

More fine granular settings have been requested several times. Having a content indexer run on ~/Downloads poses a security risk. Running full-text indexing on ~/Documents/MyCppProjects/ is definitely subject to a users preference, while most users would expect ~/Pictures/, ~/Videos/ and ~/Music/ to be scanned for metadata.

The current model and visual representation are complete nonsense, from a programmers as well as a users view. This definitely gets the model in a usable and extensible state, and shows the real state to the user (instead of showing some invented entries, and leaving out the other real half), and also makes it configurable. The important part here is the model. The view/delegate can be extended even by some person who is not familiar with baloo internals.

In general I'm not a fan of patches that change both the backend and UI and say, "well, we can make a better UI later." Let's do it now, or we might forget to do it later, or do the backend bits in a way that make it impossible to do the UI in a user-friendly way. It sounds like what we really need here is a true multi-column table, like the one in the git master version of the System Tray applet.

bruns added a comment.Mar 20 2020, 1:20 AM
In D28025#630882, @ngraham wrote:
In D28025#630849, @bruns wrote:

Having only inclusion/exclusion is a temporary state. I plan to add more settings for each path, so having an "add" button for each possible state will no longer be feasible.

Also, "removing" a path from the "included" list is not the same as excluding it - the state depends on the state of its next ancestor. This exact type of mixup has led to the messy state the current KCM is in.

Then maybe we should rethink the UI, because that's what it currently suggests. What are the other states you're planning to add?

Honestly, no, thats not what it suggests. It clearly tells if a folder should be searched (magnifying glass) or not (minus sign). Currently a checkbox would do, but thats a) not scalable b) non-obvious (does checked equal included or excluded)?

I have already mentioned meta-data vs full-text. Also file-name only is often wanted.

The Items are inspired by the Desktop Effects KCM. I have searched through the HIG for considerable time, unfortunately it lacks any specific information what to do, or any usable examples. If you can provide any examples where you think it is done the "right way (TM)", please go for it.

Yeah, we need to add more examples and better guidance to the HIG for sure. However you must be looking at an old version of the Desktop Effects KCM because the git master version shows what I'm talking about:

You can also look at the Desktop effects KCM, the Activities KCM, or Discover's Settings page.

Lets see:

  • Desktop Effects KCM: It has 3 state checkboxes where this clearly violates HIG. The "Video" button is a toggle button, but to get its meaning you have to hover it for the tooltip. It has a "Get new Desktop Effects" button on the top button, which not only installs new effects, but must also be used to uninstall effects.
  • Activities: "Create New..." on the bottom right (HIG violation).

So much for your poster childs ...

Yes, it does use custom buttons. The second one can be trivially replaced by a Kirigami.Action (or I could use "flat" style, and it would look and behave exactly like a Kirigami.Action, so no "custom button appearance" nor "behavior"). The "Enable/Disable indexing" button is custom. It can be trivially changed to a Kirigami.Action by moving the text to the tooltip, but IMHO thats much worse from a usability view.

The search/excluded icons are just the first implemented state column. There will be more columns. Having the full state in textual form for each entry will look awkward, and having it in textual form only will make it much harder to get the current state for a given path.

More fine granular settings have been requested several times. Having a content indexer run on ~/Downloads poses a security risk. Running full-text indexing on ~/Documents/MyCppProjects/ is definitely subject to a users preference, while most users would expect ~/Pictures/, ~/Videos/ and ~/Music/ to be scanned for metadata.

The current model and visual representation are complete nonsense, from a programmers as well as a users view. This definitely gets the model in a usable and extensible state, and shows the real state to the user (instead of showing some invented entries, and leaving out the other real half), and also makes it configurable. The important part here is the model. The view/delegate can be extended even by some person who is not familiar with baloo internals.

In general I'm not a fan of patches that change both the backend and UI and say, "well, we can make a better UI later." Let's do it now, or we might forget to do it later, or do the backend bits in a way that make it impossible to do the UI in a user-friendly way. It wounds like what we really need here is a true multi-column table, like the one in the git master version of the System Tray applet.

Thats not what I said. It has a sound model with this change. The model is extensible. The model represents the configuration correctly. Based on this model, everybody can polish the UI.

If you can provide a better model, a better UI, please do it. The new model and UI addresses all the complaints I am aware of.

bruns updated this revision to Diff 78064.Mar 20 2020, 1:31 AM

Use Kirigami.Action for Trash

ngraham added a comment.Mar 20 2020, 5:20 PM

Yes, it's true that our UIs are frequently inconsistent; that's why the Goal: Consistency goal exists. A big part of this goal is to improve the usability of common components like the Kirigami SwipeListItem so that people are more comfortable using it as-is and not overriding bits of or re-implementing something themselves.

If you're okay with me modifying the UI in a follow-up patch, I'll accept the UI in its current form. Is that acceptable?

bruns edited the summary of this revision. (Show Details)Mar 20 2020, 8:10 PM
bruns edited the summary of this revision. (Show Details)Mar 20 2020, 8:15 PM
bruns added a comment.Mar 20 2020, 8:37 PM
In D28025#631358, @ngraham wrote:

If you're okay with me modifying the UI in a follow-up patch, I'll accept the UI in its current form. Is that acceptable?

Thats the intention of all this - get it in a usable state, and improve it gradually.

ngraham added a comment.Mar 23 2020, 11:20 PM

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. :)

kcms/baloo/filteredfoldermodel.cpp
36

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

43

Could probably just call normalizeTrailingSlashes() here

79

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

206

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

kcms/baloo/package/contents/ui/main.qml
99–100

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

119

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

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

ditto

129

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

bruns marked 2 inline comments as done.Mar 24 2020, 12:02 AM
bruns added inline comments.
kcms/baloo/filteredfoldermodel.cpp
36

Now its likely inlined by the compiler ...

kcms/baloo/package/contents/ui/main.qml
119

IMHO it is to large to be declared inline. The toplevel layout is ~90 lines and can be viewed on a single screen now.

bruns updated this revision to Diff 78332.Mar 24 2020, 1:05 AM
bruns marked 2 inline comments as done.

update

bruns updated this revision to Diff 78333.Mar 24 2020, 1:07 AM
bruns marked 2 inline comments as done.

update2

bruns marked 3 inline comments as done.Mar 24 2020, 1:08 AM
ngraham accepted this revision.Mar 24 2020, 3:59 AM
This revision is now accepted and ready to land.Mar 24 2020, 3:59 AM
Closed by commit R119:7498b41a1979: [Baloo KCM] Complete overhaul of the include/exclude folder list (authored by bruns). · Explain WhyMar 24 2020, 8:39 PM
This revision was automatically updated to reflect the committed changes.
ngraham added a comment.Mar 25 2020, 2:43 PM

UI refinement is here: D28280

Revision Contents
Changeset List

Diff 78404

View Options

kcms/baloo/filteredfoldermodel.h

Loading...
View Options

kcms/baloo/filteredfoldermodel.cpp

Loading...
View Options

kcms/baloo/package/contents/ui/main.qml

Loading...