Differential D25999

Add optional subtitle to grid delegate
ClosedPublic
Actions

Authored by ngraham on Dec 14 2019, 5:39 PM.

Details

Reviewers
davidedmundson
ndavis
Group Reviewers
VDG
Plasma
Commits
R296:0dda5f9566cf: Add optional subtitle to grid delegate
Summary

In various cases, we may want some or all grid delegates to have a subtitle. This patch
adds such a capability. In order to make room, the vertical spacing between rows is
slightly increased.

Test Plan

Apply D25785 and add subtitle: model.followsSystemColors ? i18n("Follows color scheme") : undef to the gridDelegate:

Diff Detail

Repository
R296 KDeclarative
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Dec 14 2019, 5:39 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 14 2019, 5:39 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Dec 14 2019, 5:39 PM
ngraham edited the test plan for this revision. (Show Details)Dec 14 2019, 5:42 PM
filipf added a subscriber: filipf.Dec 14 2019, 5:52 PM

Thanks, I'll tweak my patch to use this.

Does the implementation increase the horizontal spacing between delegates even if there are no subtitles present?

ngraham added a comment.Dec 14 2019, 5:53 PM

It does, because we can't know in advance whether or not any delegates will set a subtitle. Actually... can we know this?

davidedmundson added inline comments.Dec 14 2019, 5:56 PM
src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml
193

what is the first clause for?

ngraham updated this revision to Diff 71540.Dec 14 2019, 5:58 PM
ngraham marked an inline comment as done.

Remove pointless check

filipf added a comment.Dec 14 2019, 6:08 PM
In D25999#577780, @ngraham wrote:

It does, because we can't know in advance whether or not any delegates will set a subtitle. Actually... can we know this?

Yeah if we could it wouldn't be bad to add a condition.

Although honestly after testing this live I don't really mind the added padding from a visual POV.

There's a content overflow problem in the colors KCM now though:

It's only related to the change in GridViewInternal.qml.

filipf added a comment.Dec 14 2019, 6:09 PM

^ No, it seems to be GridDelegate actually.

ndavis accepted this revision.Dec 15 2019, 1:48 AM
ndavis added a subscriber: ndavis.

+1 visually

This revision is now accepted and ready to land.Dec 15 2019, 1:48 AM
ndavis resigned from this revision.Dec 15 2019, 1:51 AM

Whoops, didn't mean to give my official approval

This revision now requires review to proceed.Dec 15 2019, 1:51 AM
ngraham added a comment.Dec 15 2019, 1:29 PM

Got a shipit from @davidedmundson, shippingit!

This revision was not accepted when it landed; it landed in state Needs Review.Dec 15 2019, 1:29 PM
Closed by commit R296:0dda5f9566cf: Add optional subtitle to grid delegate (authored by ngraham). · Explain Why
This revision was automatically updated to reflect the committed changes.
broulik added a subscriber: broulik.Dec 18 2019, 9:26 AM
broulik added inline comments.
src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml
81

This change breaks colors KCM thumbnail size

Revision Contents
Changeset List

Diff 71598

View Options

src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml

Loading...
View Options

src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

Loading...