Add Kirigami ListSectionHeader component
Needs ReviewPublic

Authored by GB_2 on Fri, Aug 9, 3:28 PM.

Details

Summary

Adds a standard ListSectionHeader component to make ListView section headers look coherent.

Test Plan

Run the example with qmlscene:

Diff Detail

Repository
R169 Kirigami
Branch
arcpatch-D23049
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15049
Build 15067: arc lint + arc unit
GB_2 created this revision.Fri, Aug 9, 3:28 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptFri, Aug 9, 3:28 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
GB_2 requested review of this revision.Fri, Aug 9, 3:28 PM
GB_2 updated this revision to Diff 63425.

Remove duplicate AbstractListItem

ngraham added a subscriber: ngraham.Fri, Aug 9, 3:38 PM

Maybe call it a ListSectionHeader or just SectionHeader?

src/controls/ListSection.qml
2 ↗(On Diff #63425)

You made this, so it's your copyright

28 ↗(On Diff #63425)

coherent -> consistent

GB_2 added inline comments.Fri, Aug 9, 3:39 PM
src/controls/ListSection.qml
28 ↗(On Diff #63425)

I just copied it from the other list items :-)

GB_2 updated this revision to Diff 63426.Fri, Aug 9, 3:46 PM

Update copyright

GB_2 added a comment.Fri, Aug 9, 3:46 PM

Maybe call it a ListSectionHeader or just SectionHeader?

I think just the word section is more accurate, because that's what the property that it will be used in is called and header could mean only a heading label.

GB_2 marked 3 inline comments as done.Fri, Aug 9, 3:46 PM
GB_2 added a reviewer: mart.
davidre added a subscriber: davidre.Fri, Aug 9, 3:49 PM

Should this set sectionDelegate: true and not expose this to the outside?

GB_2 updated this revision to Diff 63428.Fri, Aug 9, 4:02 PM

Set sectionDelegate to true

GB_2 updated this revision to Diff 63429.Fri, Aug 9, 4:10 PM

Use correct background color

In D23049#509309, @GB_2 wrote:

Maybe call it a ListSectionHeader or just SectionHeader?

I think just the word section is more accurate, because that's what the property that it will be used in is called and header could mean only a heading label.

Hmm, that's not how I interpreted it. This component isn't a whole section, as its name implies; it's just the thing that goes above a section (i.e. a header)

GB_2 updated this revision to Diff 63430.Fri, Aug 9, 4:16 PM

Rename to ListSectionHeader

GB_2 edited the test plan for this revision. (Show Details)Fri, Aug 9, 4:16 PM
GB_2 retitled this revision from Add Kirigami ListSection component to Add Kirigami ListSectionHeader component.
GB_2 edited the summary of this revision. (Show Details)
ognarb added a subscriber: ognarb.Fri, Aug 9, 4:21 PM

Something is probably wrong with the version numbers.

src/kirigamiplugin.cpp
131

Why 2.0?

src/qmldir
19

Why 2.5?

GB_2 added a comment.Fri, Aug 9, 4:23 PM

Something is probably wrong with the version numbers.

Yeah, I don't know which I should choose and why, I just copied it from the other list items.

ognarb added a comment.Fri, Aug 9, 4:27 PM

It's a new version so probably 2.9 everywhere

The numbers need to match. I think you want 2.10 (the current version appears to be 2.9, but this QML versioning stuff is pretty confusing to me).

GB_2 updated this revision to Diff 63432.Fri, Aug 9, 4:34 PM

Use Kirigami 2.10

ngraham accepted this revision as: VDG, ngraham.Fri, Aug 9, 4:36 PM

Thanks! We definitely need @mart's input on the right implementation of these details. But visually it looks great to me in System Settings (working on patches for the current users of these section headers), and but +1 on the concept of course.

This revision is now accepted and ready to land.Fri, Aug 9, 4:36 PM
ngraham added 1 blocking reviewer(s): mart.Fri, Aug 9, 4:37 PM
This revision now requires review to proceed.Fri, Aug 9, 4:37 PM

Idea: add a RowLayout inside the contentItem and put the Heading in there, then expose a property called like "CustomItems" that allows the implementation to add additional items to that RowLayout. This would let us port Discover's sources page headers, which currently have custom controls in them:

GB_2 updated this revision to Diff 63442.Fri, Aug 9, 6:15 PM

Add customItems list

GB_2 edited the test plan for this revision. (Show Details)Fri, Aug 9, 6:15 PM

Idea: add a RowLayout inside the contentItem and put the Heading in there, then expose a property called like "CustomItems" that allows the implementation to add additional items to that RowLayout. This would let us port Discover's sources page headers, which currently have custom controls in them:

Done, I also updated the example :-)

GB_2 edited the test plan for this revision. (Show Details)Fri, Aug 9, 6:16 PM
GB_2 edited the summary of this revision. (Show Details)Fri, Aug 9, 6:19 PM
GB_2 updated this revision to Diff 63443.Fri, Aug 9, 6:20 PM

Update customItems description

GB_2 edited the test plan for this revision. (Show Details)Fri, Aug 9, 6:28 PM
GB_2 updated this revision to Diff 63444.Fri, Aug 9, 6:36 PM

Add example code

mart requested changes to this revision.Mon, Aug 12, 5:38 PM
This revision now requires changes to proceed.Mon, Aug 12, 5:38 PM
mart added inline comments.Tue, Aug 13, 9:28 AM
src/controls/ListSectionHeader.qml
93

This is a no-go hack:
those customItems shouldn't be reparented, but rather the property should be an alias to the data of another layout, like:

property alias customItems: innerLayout.data
contentItem: RowLayout {
    id: rowLayout
    RowLayout {
        id: innerLayout
    }

or, i would prefer even more, to just ditch that extra property completely:

default property alias _contents: rowLayout.data
contentItem: RowLayout {

​ id: rowLayout
​ Heading {

....

so one just does

ListSectionHeader {
   Button {....}
}

and just works(tm)

GB_2 updated this revision to Diff 63653.Tue, Aug 13, 10:31 AM

Ditch the extra property

GB_2 marked 3 inline comments as done.Tue, Aug 13, 10:32 AM
GB_2 edited the test plan for this revision. (Show Details)

Hmm, if software making use of this needs to manually add a label, that seems to defeat part of the point of using a standard component.

GB_2 added a comment.Tue, Aug 13, 1:47 PM

Hmm, if software making use of this needs to manually add a label, that seems to defeat part of the point of using a standard component.

The label in the example was just supposed to show that any controls can be added. Sorry for the confusion, I'll remove it.

GB_2 updated this revision to Diff 63662.Tue, Aug 13, 2:09 PM

Improve example

GB_2 edited the summary of this revision. (Show Details)Tue, Aug 13, 2:10 PM
GB_2 edited the test plan for this revision. (Show Details)
GB_2 added a comment.Sat, Aug 17, 9:27 AM

@mart is this good now?