[FormLayout] Use even top and bottom spacing for separator
ClosedPublic

Authored by filipf on Apr 15 2019, 10:18 PM.

Details

Summary

The Kirigami separator has spacing below it, but not above it when in a Form Layout. This patch adds a check if there is a separator present and if it is, it adds spacing above it.

BUG: 405614

Test Plan

Before:

After:

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
filipf created this revision.Apr 15 2019, 10:18 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptApr 15 2019, 10:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Apr 15 2019, 10:18 PM
filipf edited the summary of this revision. (Show Details)Apr 15 2019, 10:20 PM
filipf edited the test plan for this revision. (Show Details)
filipf added reviewers: mart, Kirigami, ngraham.
mart accepted this revision.Apr 26 2019, 11:11 AM
This revision is now accepted and ready to land.Apr 26 2019, 11:11 AM

@mart are you sure this is the right solution? From my tests the patch also adds extra spacing whenever there is an instance of isSection, but we'd only want it to do so when there is a separator present.

Before:

After:

We can see in this example when there is no separator that extra (unwanted) spacing has been added.

mart requested changes to this revision.Apr 26 2019, 1:06 PM

ok, let's delay this still for a while

This revision now requires changes to proceed.Apr 26 2019, 1:06 PM
filipf updated this revision to Diff 57065.Apr 26 2019, 7:25 PM

OK I solved it, but not sure if there isn't a better way

filipf retitled this revision from [FormLayout] RFC: Use even top and bottom spacing for separator to [FormLayout] Use even top and bottom spacing for separator.Apr 26 2019, 7:26 PM
filipf edited the summary of this revision. (Show Details)
ngraham accepted this revision as: ngraham.Apr 26 2019, 8:13 PM

This looks okay to me, but wait for @mart. :)

mart requested changes to this revision.May 16 2019, 10:30 AM

I don't like adding a new property to separator for doing duck typing...

even is really ugly and errorprone as well, but i would prefer
Layout.topMargin: item.toString().indexOf("Separator")===0 ? Kirigami.Units.smallSpacing : 0

with a comment:
// FIXME: use item instanceof Kirigami.Separator when we can depend from Qt 5.11

This revision now requires changes to proceed.May 16 2019, 10:30 AM
filipf updated this revision to Diff 58443.May 21 2019, 9:28 PM

use mart's solution

filipf updated this revision to Diff 58444.May 21 2019, 9:32 PM

minor fix

In D20585#465924, @mart wrote:

I don't like adding a new property to separator for doing duck typing...

even is really ugly and errorprone as well, but i would prefer
Layout.topMargin: item.toString().indexOf("Separator")===0 ? Kirigami.Units.smallSpacing : 0

with a comment:
// FIXME: use item instanceof Kirigami.Separator when we can depend from Qt 5.11

This doesn't work, but I will try to tinker with it.

mart accepted this revision.May 23 2019, 8:39 AM
This revision is now accepted and ready to land.May 23 2019, 8:39 AM
This revision was automatically updated to reflect the committed changes.