WIP: Beginning of the Baloo/Search KCM Rewrite
ClosedPublic

Authored by tcanabrava on Sep 4 2019, 2:39 PM.

Details

Diff Detail

Repository
R119 Plasma Desktop
Branch
kcm_baloo_qml
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16079
Build 16097: arc lint + arc unit
tcanabrava created this revision.Sep 4 2019, 2:39 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 4 2019, 2:39 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
tcanabrava requested review of this revision.Sep 4 2019, 2:39 PM
ngraham added a subscriber: ngraham.Sep 4 2019, 2:41 PM
ngraham added inline comments.
kcms/baloo/package/ui/main.qml
23

import QtQuick.Controls 2.5 as QQC2 (See T10862)

mart added a subscriber: mart.Sep 4 2019, 2:42 PM
mart added inline comments.
kcms/baloo/package/ui/main.qml
33

should use a Kirigami.FormLayout to have checkboxes centered?

mart added a comment.Sep 4 2019, 2:42 PM

please, include screenshots

ngraham added inline comments.Sep 4 2019, 3:29 PM
kcms/baloo/package/ui/main.qml
31

While we're at it, let's improve this string:

"This module lets you configure the system-wide file search features"

tcanabrava updated this revision to Diff 65375.Sep 4 2019, 4:18 PM
  • Fix metadata installation
  • Fix model loading on Qml
  • Base work on the Qml
tcanabrava updated this revision to Diff 65378.Sep 4 2019, 4:35 PM
  • Name fixes
ngraham edited the summary of this revision. (Show Details)Sep 5 2019, 4:27 PM
ngraham added a reviewer: Plasma.
tcanabrava updated this revision to Diff 66258.Sep 16 2019, 7:14 PM
  • Fix metadata installation
  • Fix model loading on Qml
  • Base work on the Qml
  • Name fixes
  • Move ui/main to contents
  • Fix wrong types in Qml
  • Plug the controllers
  • Link to the save button

Getting there!

Use an actual framed list view for the list of folders to not index, like the current one has.

mart accepted this revision.Sep 17 2019, 10:14 AM
This revision is now accepted and ready to land.Sep 17 2019, 10:14 AM
ngraham requested changes to this revision.Sep 17 2019, 12:46 PM

This isn't ready yet without, at a minimum, the UI change I requested for the list view.

This revision now requires changes to proceed.Sep 17 2019, 12:46 PM
tcanabrava updated this revision to Diff 66643.Sep 23 2019, 7:45 AM
  • Fix metadata installation
  • Fix model loading on Qml
  • Base work on the Qml
  • Name fixes
  • Move ui/main to contents
  • Fix wrong types in Qml
  • Plug the controllers
  • Link to the save button
  • Fix borders and background for the Baloo KCM
nicolasfella added inline comments.
kcms/baloo/package/metadata.desktop
4

That comment sounds wrong :)

The frame's alignment and padding seem off:

Could we use a list more like the one used in the Virtual Desktops KCM and the slideshow wallpaper folder list? Then we would have a unified appearance (Goal: Consistency :) ).

GB_2 added a subscriber: GB_2.Sep 25 2019, 2:49 PM

The frame's alignment and padding seem off:

Could we use a list more like the one used in the Virtual Desktops KCM and the slideshow wallpaper folder list? Then we would have a unified appearance (Goal: Consistency :) ).

Yeah, use a Kirigami BasicListItem.

kcms/baloo/package/contents/ui/main.qml
2 ↗(On Diff #66643)

Shouldn't this be you copyright?

got it, updating.

tcanabrava updated this revision to Diff 67563.Oct 9 2019, 2:57 PM
  • Fix metadata installation
  • Fix model loading on Qml
  • Base work on the Qml
  • Name fixes
  • Move ui/main to contents
  • Fix wrong types in Qml
  • Plug the controllers
  • Link to the save button
  • Fix borders and background for the Baloo KCM
  • Use kirigami BasicListItem
  • Spacing
  • Fix mapping of roles
  • Fix delegate / Mouse integration
ngraham requested changes to this revision.Oct 9 2019, 3:23 PM

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:
kcms/baloo/filteredfoldermodel.cpp
148

Don't use Q_FOREACH in new or ported code

kcms/baloo/package/contents/ui/main.qml
2 ↗(On Diff #66643)

Not done yet

23 ↗(On Diff #67563)

IIRC plasma isn't depending on Qt 5.12 yet, right?

39 ↗(On Diff #67563)

This is a sentence, so it needs to end with a period.

47 ↗(On Diff #67563)

This can be simplified to just if (!checked)

50 ↗(On Diff #67563)

Don't need to reference it with fileSearchEnabled, just do kcm.indexing = checked

53 ↗(On Diff #67563)

Don't do it this way, just use a binding:

checked: kcm.indexing

69 ↗(On Diff #67563)

Same comments as with the first checkbox

71 ↗(On Diff #67563)

I would add some spacing between the last checkbox and the list view's explanatory label:

Item {
    Layout.preferredHeight: Kirigami.Units.gridUnit
}
kcms/baloo/package/metadata.desktop
4

Not done yet

This revision now requires changes to proceed.Oct 9 2019, 3:23 PM
tcanabrava updated this revision to Diff 67567.Oct 9 2019, 4:29 PM
tcanabrava marked 7 inline comments as done.
  • Simplify Checkboxes
  • Fix previouslyEnabled logic
  • Tell Qml that we have the values
  • Move code around
  • Fix QSet math
  • Qml gives us a Url, we want the Local file
  • use folderAdded and folderRemoved
  • Remove debug information
  • Add spacing
tcanabrava updated this revision to Diff 67568.Oct 9 2019, 4:30 PM
  • fix Comment
tcanabrava marked 2 inline comments as done.Oct 9 2019, 4:31 PM

I still need to play a bit with the folder save / load. the code from the old kcm is quite strange. it saves "included folders", but there's nothing using that. perhaps it's safer if we just remove everything that's not used.

Almost done, just a tiny bit of polish is needed! See inline comments:

kcms/baloo/kcm.cpp
56

Make this match the name shown in the list ("File Search")

kcms/baloo/package/contents/ui/main.qml
32 ↗(On Diff #67568)

"This module lets you configure the file indexer and search functionality."

36 ↗(On Diff #67568)

This item isn't inside a Layout, so Layout.<anything> has no effect. You want anchors.<whatever> instead.

Also the margins should be Kirigami.Units.largeSpacing.

tcanabrava updated this revision to Diff 67572.Oct 9 2019, 6:12 PM
  • Fix Strings
  • Fix margins
tcanabrava marked 3 inline comments as done.Oct 9 2019, 6:12 PM
This revision is now accepted and ready to land.Oct 9 2019, 7:10 PM
This revision was automatically updated to reflect the committed changes.
GB_2 added inline comments.Oct 12 2019, 12:37 PM
kcms/baloo/package/contents/ui/main.qml
27 ↗(On Diff #67701)

Is this actually used? Also, because it's not installed by default it shows an error.

ngraham added inline comments.Nov 12 2019, 7:59 PM
kcms/baloo/package/contents/ui/main.qml
27 ↗(On Diff #67701)