Correctly compute bottom margin for grid delegates with subtitles
ClosedPublic

Authored by ngraham on Mon, Jan 13, 5:26 PM.

Details

Summary

When I added the subtitle feature, I didn't notice that the bottom margin calculation
was only including the original single label. This patch fixes that to include the
subtitle, if present.

Test Plan

With D26531 applied, before:

After:

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.Mon, Jan 13, 5:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Jan 13, 5:26 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Mon, Jan 13, 5:26 PM

Oh wow, I just noticed that putting these labels in a ColumnLayout triggered the infamous QML text kerning bug! https://bugreports.qt.io/browse/QTBUG-49646

Could that be a clue to solving it maybe?

Seeing this now, I can revert the change to use a ColumnLayout. But it's interesting to see it reproducing like this. Can anyone else confirm?

gvgeo added a subscriber: gvgeo.Mon, Jan 13, 5:47 PM
gvgeo added inline comments.
src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml
81

Maybe need to add caption.visible check, instead of always removing 3 gridUnits?
Was changed from 2 in the previous commit.

ngraham edited the test plan for this revision. (Show Details)Mon, Jan 13, 5:50 PM
ngraham marked an inline comment as done.Mon, Jan 13, 6:06 PM
ngraham added inline comments.
src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml
81

If I do that, then grids where some delegates have captions and others don't display delegates of multiple sizes, which looks bad:

ngraham updated this revision to Diff 73446.Mon, Jan 13, 6:06 PM
ngraham marked 2 inline comments as done.

Fix comment whitespace

gvgeo added a comment.EditedMon, Jan 13, 8:27 PM

Then, is it an option to create GridDelegateSubtitles.qml instead?
As is now, without subtitles, I see everywhere squash shapes and bigger than necessary empty space.

now vs before


Image is wrong. Made an error.
In this examples is not a big problem, but it must be used elsewhere that will have bigger visual effect.

Then, is it an option to create GridDelegateSubtitles.qml instead?
As is now, without subtitles, I see everywhere squash shapes and bigger than necessary empty space.

now vs before


In this examples is not a big problem, but it must be used elsewhere that will have bigger visual effect.

That's what you see with this patch? That's not what I see, weird. For me, with the patch, I see the following for grids without subtitles:

gvgeo added a comment.Tue, Jan 14, 1:09 AM

That's what you see with this patch?....

My apologies, I've made an error.

No worries!

davidedmundson accepted this revision.Fri, Jan 17, 2:37 PM
This revision is now accepted and ready to land.Fri, Jan 17, 2:37 PM
This revision was automatically updated to reflect the committed changes.