[Plasma Style KCM] Add search filter
ClosedPublic

Authored by davidre on Dec 16 2019, 8:42 AM.

Details

Summary

Addsm the ability to filter themes by name by typing in the search line or
whether they use the current color scheme or if they are light or dark if they
do not. To enable this the model is extracted out into its own class and a
QSortFilterProxyModel is added for the Filtering. This is mostly based on the
same functionality of the Colors KCM and uses the same heuristic to decide if a
theme is light or dark.

Test Plan

Diff Detail

Repository
R119 Plasma Desktop
Branch
filtermodel
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19970
Build 19988: arc lint + arc unit
davidre created this revision.Dec 16 2019, 8:42 AM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 16 2019, 8:42 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Dec 16 2019, 8:42 AM
davidre edited the test plan for this revision. (Show Details)Dec 16 2019, 8:42 AM
crossi added a subscriber: crossi.Dec 16 2019, 8:58 AM
ndavis added a subscriber: ndavis.Dec 17 2019, 1:50 AM
ndavis added inline comments.
kcms/desktoptheme/package/contents/ui/main.qml
106

"Color scheme compatible" is a bit better because it won't get shortened and even if it does, it'll be easier to see what the filter does.

Some concern here as I tested the patch. The filter is working but the implementation breaks some functionality.

  • At initialization, the current theme is not selected in the grid view.
  • When settings a theme for deletion, it does not activate the apply button and the theme is no longer greyed out in the grid view.
kcms/desktoptheme/kcm.cpp
72

Should connect to KCMDesktopTheme::settingsChanged slot to request ManagedConfigModule to reassess isSaveNeeded().

309

Why removing this ?

It is necessary for ManagedConfigModule to enable the apply button when items are marked for deletion.

crossi added a subscriber: ervin.Dec 17 2019, 12:58 PM
davidre added a comment.EditedDec 17 2019, 1:07 PM

Some concern here as I tested the patch. The filter is working but the implementation breaks some functionality.

  • At initialization, the current theme is not selected in the grid view.
  • When settings a theme for deletion, it does not activate the apply button and the theme is no longer greyed out in the grid view.

Curious every issue you describe works for me. It would surprise me because the relevant code should be copied as is from the colors kcm.
I don't know what I just tested but you are right it doesn't work.

davidre updated this revision to Diff 71720.Dec 17 2019, 1:50 PM
davidre marked 2 inline comments as done.
  • Reinstate isSaveNeeded
  • Fix initial index and pendingDeletion
davidre updated this revision to Diff 71721.Dec 17 2019, 1:52 PM

Diff against master

davidre updated this revision to Diff 71722.Dec 17 2019, 1:54 PM
davidre marked an inline comment as done.
  • better string
davidre updated this revision to Diff 71723.Dec 17 2019, 1:59 PM

Don't have a overly long line

davidre updated this revision to Diff 71724.Dec 17 2019, 2:03 PM

I think this signal is not needed

ndavis accepted this revision.Dec 17 2019, 4:22 PM

Visually LGTM

This revision is now accepted and ready to land.Dec 17 2019, 4:22 PM
  • Reinstate isSaveNeeded
  • Fix initial index and pendingDeletion

I can confirm these issue are fixed.

Still one concern, when setting an item for pending deletion, it moves the current index to the next item after the one marked for deletion. See the attachement below.

Current style is Breeze Dark, I want to delete Nordic style.

Once marked for deletion, Oxygen style is now selected

davidre updated this revision to Diff 71760.Dec 18 2019, 8:40 AM
  • Only move to the next theme if the current selected one is marked for deletion

FYI I could spot this same bug in the colors kcm. Will fix after this

  • Only move to the next theme if the current selected one is marked for deletion

    FYI I could spot this same bug in the colors kcm. Will fix after this

Looks good to me.

ngraham accepted this revision.Dec 18 2019, 1:34 PM
ervin requested changes to this revision.Dec 23 2019, 12:43 PM
ervin added inline comments.
kcms/desktoptheme/filterproxymodel.cpp
111

Ditto

kcms/desktoptheme/filterproxymodel.h
62

s/source_row/sourceRow/ same for source_parent

kcms/desktoptheme/package/contents/ui/ThemePreview.qml
178–186

Better use === with this bloody javascript

kcms/desktoptheme/themesmodel.cpp
162

A ref on a temporary sounds like a bad idea to me.

166

Don't use Q_FOREACH but a proper for in new code

174

qAsConst(themes)

238

qAsConst(m_data)

kcms/desktoptheme/themesmodel.h
66

This one and the two following are missing the default values for their parameters (which parents have, I wish override would check for that too).

This revision now requires changes to proceed.Dec 23 2019, 12:43 PM
davidre updated this revision to Diff 72087.Dec 23 2019, 3:05 PM
davidre marked 7 inline comments as done.
  • ervin's comments
  • remove unused method declaration
davidre updated this revision to Diff 72088.Dec 23 2019, 3:06 PM

Diff against master, somehow this branch is screwed up when using arc

davidre added inline comments.Dec 23 2019, 3:06 PM
kcms/desktoptheme/themesmodel.cpp
162

Yeah that doesn't look right. The load method is a straight copy from the old load method.

238

Is it also needed in a const member method?

davidre marked an inline comment as done.Dec 23 2019, 3:07 PM
ervin added inline comments.Dec 23 2019, 3:20 PM
kcms/desktoptheme/themesmodel.cpp
238

Not strictly speaking indeed.

Now C++ being what it is you're never safe of someone introducing a member as mutable though. ;-)

ervin accepted this revision.Dec 23 2019, 3:23 PM
This revision is now accepted and ready to land.Dec 23 2019, 3:23 PM
This revision was automatically updated to reflect the committed changes.