[RFC] Unify style of new Kirigami.ListSectionHeader and CategoryDrawer
ClosedPublic

Authored by davidre on Oct 7 2019, 9:35 AM.

Details

Summary

Before the default style looked quite oxygenish. Now that we have settled on a style with Kirigami.ListSectionHeader component we can use the same style here to improve consistency between QWidget and QML lists that are displayed next to each other.

Test Plan

Before:
kcategorizedviewtest{F7546011}
Global Shortcuts KCM (without scrollbar)


KRunner KCM (with scrollbar)

After:
kcategorizedviewtest


Global Shortcuts KCM

KRunner KCM

Diff Detail

Repository
R276 KItemViews
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidre created this revision.Oct 7 2019, 9:35 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 7 2019, 9:35 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
davidre requested review of this revision.Oct 7 2019, 9:35 AM
davidre edited the summary of this revision. (Show Details)Oct 7 2019, 9:47 AM
davidre edited the test plan for this revision. (Show Details)
davidre added reviewers: Frameworks, VDG.
ngraham added a subscriber: ngraham.Oct 7 2019, 2:13 PM

Nice. Is there any way to make it touch the edges of its parent view rather than having those small margins?

Might also be nice to add a comment somewhere in here reminding people to adjust the Kirigami BasicListHeader accordingly when this is changed (and vice versa).

I could set categorySpacing in KCategorizedView::Private::Private to 0.


But the difference is that in widgets the scrollbar doesn't overlap the contents.

I could set categorySpacing in KCategorizedView::Private::Private to 0.


But the difference is that in widgets the scrollbar doesn't overlap the contents.

That's fine.

Frankly I would really like a vertical line separating the scrollbar from the content for just this reason. IIRC Breeze used to have that but it lost it for some reason. When the scrollbar isn't an overlay, we should try to make it look like one IMO.

davidre updated this revision to Diff 67480.Oct 8 2019, 8:36 AM
davidre edited the summary of this revision. (Show Details)

Set categoryspacing to 0

davidre edited the test plan for this revision. (Show Details)Oct 8 2019, 8:40 AM

Looking at lxr other users include (not exhaustive, I may have missed some)
Konversation


Kexi (don't have it installed),
Amarok,

KPluginSelector (example where it is used?),
KTouch
,
some pimExamples,
nepomuk,
and public transport (optionally).

LGTM except for the abrupt end on the right side where the scrollbar's area begins, but this is a pre-existing problem with all things that butt up against a scrollbar in a scrollview, and I think we should add a vertical separator line to fix it universally.

ognarb added a subscriber: ognarb.Oct 8 2019, 3:14 PM
ognarb added inline comments.
src/kcategorydrawer.cpp
93

Instead of using magic number, I would create a constant named padding.

davidre updated this revision to Diff 67529.Oct 9 2019, 8:30 AM

Make code more expressive and add comments

davidre marked an inline comment as done.Oct 9 2019, 8:31 AM

+1. Can you do the same thing for the similar header that's seen in System Settings' icon view?

+1. Can you do the same thing for the similar header that's seen in System Settings' icon view?

Of course. This has to be done in systemsettings directly then because it uses a custom CategoryDrawer.
https://cgit.kde.org/systemsettings.git/tree/icons/CategoryDrawer.cpp

ngraham accepted this revision.Oct 9 2019, 5:15 PM

Oh I see.

This revision is now accepted and ready to land.Oct 9 2019, 5:15 PM
ndavis added a subscriber: ndavis.Oct 10 2019, 7:18 AM
This comment was removed by ndavis.
This comment was removed by ndavis.
staniek removed a reviewer: KEXI.Oct 10 2019, 10:24 AM
This revision was automatically updated to reflect the committed changes.