D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:a1fbeb96dc6c: [Colors KCM] Add search and filter 
(authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18646?vs=53304=53422#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18646?vs=53304=53422

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/colors.cpp
  kcms/colors/colors.h
  kcms/colors/colorsmodel.cpp
  kcms/colors/colorsmodel.h
  kcms/colors/filterproxymodel.cpp
  kcms/colors/filterproxymodel.h
  kcms/colors/package/contents/ui/main.qml

To: broulik, #plasma, #vdg, hein, mart
Cc: hein, davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik updated this revision to Diff 53304.
broulik added a comment.
This revision is now accepted and ready to land.


  - Refactor to use proper `QAbstractListModel` for colors
  - Fix current scheme index following as you filter

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18646?vs=50838=53304

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/colors.cpp
  kcms/colors/colors.h
  kcms/colors/colorsmodel.cpp
  kcms/colors/colorsmodel.h
  kcms/colors/filterproxymodel.cpp
  kcms/colors/filterproxymodel.h
  kcms/colors/package/contents/ui/main.qml

To: broulik, #plasma, #vdg, hein
Cc: hein, davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik planned changes to this revision.
broulik added a comment.


  The `selectedSchemeIndex` stuff doesn't take into account the filtering, so 
the highlight is wrong when filtering and also when marking a theme for 
deletion the index changes to the next one but might end up applying a 
currently invisible theme..

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg, hein
Cc: hein, davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18646: [Colors KCM] Add search and filter

[Eike Hein]

hein accepted this revision.
hein added a comment.
This revision is now accepted and ready to land.


  LGTM aside from the minor naming nitpick.

INLINE COMMENTS

> filterproxymodel.cpp:31
> +
> +void FilterProxyModel::setSourceModelProxy(QAbstractItemModel *sourceModel)
> +{

Why did you call this `setSourceModelProxy`? `QSortFilterModel::setSourceModel` 
is virtual, you could do the emit in a reimpl?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg, hein
Cc: hein, davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik added a comment.


  > It does kind of beg the question of why this is the only grid view that has 
a search, of course.
  
  Dunno, colors has significantly more entries than the other KCMs.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Nathaniel Graham]

ngraham added a comment.


  UI-wise, I think this can go in.
  
  It does kind of beg the question of why this is the only grid view that has a 
search, of course.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik added a comment.


  Is this good now?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> davidedmundson wrote in main.qml:129
> why do we need a different approach from the way you've done the clear button?

If I could change the left padding of the `ComboBox` button I would have done 
that and placed an icon ontop. However, since it's all painted as a monolith by 
`KQuickStyleItem` I cannot.

It doesn't follow the `padding` properties because it's all derived from the 
`QStyle`, and changing that opens a can of worm imho.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> main.qml:129
> +
> +// HACK QQC2 doesn't support icons, so we just tamper with 
> the desktop style ComboBox's background
> +// and inject a nice little filter icon.

why do we need a different approach from the way you've done the clear button?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Nathaniel Graham]

ngraham accepted this revision as: VDG.
ngraham added a comment.


  UI and interactivity are great. Very nice addition.
  
  Would be nice to have the search field as a re-usable component, yes. As in 
D18716: Add a SearchField component .

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik updated this revision to Diff 50838.
broulik edited the test plan for this revision.
broulik added a comment.


  - Add filter icon to ComboBox, needs D18718 


REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18646?vs=50667=50838

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/colors.cpp
  kcms/colors/colors.h
  kcms/colors/filterproxymodel.cpp
  kcms/colors/filterproxymodel.h
  kcms/colors/package/contents/ui/main.qml

To: broulik, #plasma, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik updated this revision to Diff 50667.
broulik edited the test plan for this revision.
broulik added a comment.


  - Change filter into a simple `ComboBox`, simplifies code a lot
  - Change placeholder to "Search..." to be consistent with other KCMs
  - Fixed left-to-right layout issues

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18646?vs=50654=50667

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/colors.cpp
  kcms/colors/colors.h
  kcms/colors/filterproxymodel.cpp
  kcms/colors/filterproxymodel.h
  kcms/colors/package/contents/ui/main.qml

To: broulik, #plasma, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik planned changes to this revision.
broulik added a comment.


  Alright, will change it to a `ComboBox`. We might lose the filter icon, which 
I quite liked, in the process, though.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Nathaniel Graham]

ngraham added a comment.


  In D18646#403270 , @broulik wrote:
  
  > An alternative would be
  >  F6581375: Screenshot_20190201_162719.png 

  
  
  I would prefer this, but with the filter being a real combobox. In other 
words, I'd like the following changes:
  
  - Button should not be flat; should draw the button border (we only put flat 
buttons in toolbars)
  - The button should have a downward-pointing arrow to indicate that it's a 
combobox
  - The pop-up doesn't need radio buttons bu the menu items

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik added a comment.


  An alternative would be
  F6581375: Screenshot_20190201_162719.png 

  which is a lot more obvious than a tiny filter icon. Your decision, also 
whether it should highlight when filtered or not (it does not in Widget 
Explorer where I took this from)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18646: [Colors KCM] Add search and filter

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, VDG.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  This adds a search bar to search for schemes in the list as well as a filter 
to show only light or dark themes using a heuristic on the theme's window color.

TEST PLAN
  F6581348: Screenshot_20190201_161444.png 

  
  Should the filter be cleared when you install a new theme to avoid it being 
hidden by a filter?
  When a filter is active, the filter icon turns blue.
  
  There's some minor right-to-left layout issues

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/colors.cpp
  kcms/colors/colors.h
  kcms/colors/filterproxymodel.cpp
  kcms/colors/filterproxymodel.h
  kcms/colors/package/contents/ui/TextFieldButton.qml
  kcms/colors/package/contents/ui/main.qml

To: broulik, #plasma, #vdg
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart