Re: Spacing in our apps

2023-12-18 Thread Nate Graham
This is an important topic, so I appreciate you bringing it up, Carl. 
It's been on my mind recently as well.


For some background, the reason why we have multiple spacing values in 
Kirigami and Plasma (e.g. SmallSpacing, LargeSpacing, GridUnit, etc) is 
because in the past we didn't want developers multiplying standard 
spacing values by "random numbers." The idea was that developers would 
choose the perfect spacing value for each situation and use it directly, 
without having to multiply it by anything.


Unfortunately, we failed to make it obvious what situation merited what 
spacing value. So in practice individual developers ended up using 
whichever spacing values felt best to them, and as we know, opinions 
differ on the topic of padding vs density in UI layouts. The result is 
inconsistent spacings in many places.


In addition, people end up multiplying the standard spacings by random 
numbers anyway, because most of them are quite small and sometimes you 
do need big spacing.


Finally, until Noah added MediumSpacing fairly recently, none of the 
standard spacing values matched the default spacing value in Breeze of 
6px. Even then the spacings may not remain a perfect match because the 
Kirigami/Plasma spacing values adjust with the font size, while the 
Breeze one does not.


I think we can unfortunately conclude that this design system was not a 
success, and in fact worsened the consistency of our spacings in 
QtQuick-based software compared to QtWidgets-based software while 
confusing developers.


As such, I support *proposal A*, which I feel is the simplest and most 
comprehensible one.


I could accept "A bis", but I think having two standard values that get 
defined to the same underlying value in Breeze is recipe for continuing 
the confusion, just in a different form. What's the difference between a 
spacing and a margin, anyway? We might want to move away from continuing 
this distinction in our QStyle too, seeing as we define both values to 
the same thing.



Nate




On 12/17/23 05:21, Carl Schwan wrote:

Hi,

I'm been trying to unify a bit the usage of spacing in our apps and I'm
noticing a difference between how we do it in QWidgets apps and QML apps.

In QtWidgets apps, we use

- pixelMetric(QStyle::PM_Layout{Left,Right,Top,Bottom}Margin) for the margins
- pixelMetric(QStyle::PM_Layout{Vertical,Horizontal}Margin) for the spacing
between items in layout

In practice all these pixel metrics are equal to 6 pixels with Fusion, Oxygen
and Breeze. These means that in some cases, some apps are even hardcoding
these values in their .ui files, which is bad and we should try to avoid this.

In Kirigami apps, we use:

- Kirigami.Units.smallSpacing = 4 pixels
- Kirigami.Units.mediumSpacing = 6 pixels
- Kirigami.Units.largeSpacing = 8 pixels

In most cases, smallSpacing and largeSpacing are used as mediumSpacing was
introduced later on, so it doesn't match the values from QtWidgets apps. Worse
we don't really have clear guidelines when to use small or large spacing, so
it's mostly done arbitrarely and not consistantly :(

Also having 3 different Kirigami.Units.*Spacing values that are each only 2
pixels appart doesn't sounds like a great idea as it's hard to see a
difference between these values taken side by side.

I see three ways to move forward with this issue:

a) Remove smallSpacing and largeSpacing from Kirigami, and rename
mediumSpacing to just spacing. This unified spacing value would be defined in
qqc2-desktop-style to use whatever value is defined in the current QStyle.

a bis) Instead of creating only a generic "spacing" property, we create a
"Kirigami.Units.margins" or "Kirigami.Units.paddings" property to use for
paddings of QtQuick Controls and mapped to the Layout*Margin pixel metrics and
a "Kirigami.Units.spacing" property mapped to the Layout*Spacing pixel
metrics. For Breeze and Oxygen, both value would map to 6 pixels anyway, but
it might make it easier to switch to other values in the future as well as
make the usage of Units value more explit.

b) Use 4 pixels as standard spacing in our QtWidgets apps and add a "margins"
and "largeMargin" helper methods in KWidgetsAddons or QStyle similar to this

QMargins QStyle::largeMargins() const
{
 return QMargins{
 pixelMetric(QStyle::PM_LayoutLeftMargin),
 pixelMetric(QStyle::PM_LayoutTopMargin),
 pixelMetric(QStyle::PM_LayoutRightMargin),
 pixelMetric(QStyle::PM_LayoutBottomMargin)
 }
}

QMargins QStyle::largeMargins() const
{
 return QMargins{
 pixelMetric(QStyle::PM_LayoutLeftMargin) * 2,
 pixelMetric(QStyle::PM_LayoutTopMargin) * 2,
 pixelMetric(QStyle::PM_LayoutRightMargin) * 2,
 pixelMetric(QStyle::PM_LayoutBottomMargin) * 2
 }
}

then we can remove mediumSpacing from Kirigami and ensure that in both our
Kirigami and QtWidgets apps, we use small or large only no

Re: Spacing in our apps

2023-12-17 Thread Ingo Klöcker
On Sonntag, 17. Dezember 2023 13:21:32 CET Carl Schwan wrote:
> a) Remove smallSpacing and largeSpacing from Kirigami, and rename
> mediumSpacing to just spacing. This unified spacing value would be defined
> in qqc2-desktop-style to use whatever value is defined in the current
> QStyle.
> 
> a bis) Instead of creating only a generic "spacing" property, we create a
> "Kirigami.Units.margins" or "Kirigami.Units.paddings" property to use for
> paddings of QtQuick Controls and mapped to the Layout*Margin pixel metrics
> and a "Kirigami.Units.spacing" property mapped to the Layout*Spacing pixel
> metrics. For Breeze and Oxygen, both value would map to 6 pixels anyway,
> but it might make it easier to switch to other values in the future as well
> as make the usage of Units value more explit.

+1 for getting rid of choices because it makes my life easier if I don't have 
to think about which value is the correct one in some situation. I don't have 
an opinion on a bis), but I guess it makes sense to mirror the QWidget pixel 
metrics in QML.

Regards,
Ingo


signature.asc
Description: This is a digitally signed message part.


Spacing in our apps

2023-12-17 Thread Carl Schwan
Hi,

I'm been trying to unify a bit the usage of spacing in our apps and I'm 
noticing a difference between how we do it in QWidgets apps and QML apps.

In QtWidgets apps, we use

- pixelMetric(QStyle::PM_Layout{Left,Right,Top,Bottom}Margin) for the margins
- pixelMetric(QStyle::PM_Layout{Vertical,Horizontal}Margin) for the spacing 
between items in layout

In practice all these pixel metrics are equal to 6 pixels with Fusion, Oxygen 
and Breeze. These means that in some cases, some apps are even hardcoding 
these values in their .ui files, which is bad and we should try to avoid this.

In Kirigami apps, we use:

- Kirigami.Units.smallSpacing = 4 pixels
- Kirigami.Units.mediumSpacing = 6 pixels
- Kirigami.Units.largeSpacing = 8 pixels

In most cases, smallSpacing and largeSpacing are used as mediumSpacing was 
introduced later on, so it doesn't match the values from QtWidgets apps. Worse 
we don't really have clear guidelines when to use small or large spacing, so 
it's mostly done arbitrarely and not consistantly :(

Also having 3 different Kirigami.Units.*Spacing values that are each only 2 
pixels appart doesn't sounds like a great idea as it's hard to see a 
difference between these values taken side by side.

I see three ways to move forward with this issue:

a) Remove smallSpacing and largeSpacing from Kirigami, and rename 
mediumSpacing to just spacing. This unified spacing value would be defined in 
qqc2-desktop-style to use whatever value is defined in the current QStyle.

a bis) Instead of creating only a generic "spacing" property, we create a 
"Kirigami.Units.margins" or "Kirigami.Units.paddings" property to use for 
paddings of QtQuick Controls and mapped to the Layout*Margin pixel metrics and 
a "Kirigami.Units.spacing" property mapped to the Layout*Spacing pixel 
metrics. For Breeze and Oxygen, both value would map to 6 pixels anyway, but 
it might make it easier to switch to other values in the future as well as 
make the usage of Units value more explit.

b) Use 4 pixels as standard spacing in our QtWidgets apps and add a "margins" 
and "largeMargin" helper methods in KWidgetsAddons or QStyle similar to this

QMargins QStyle::largeMargins() const
{
return QMargins{
pixelMetric(QStyle::PM_LayoutLeftMargin),
pixelMetric(QStyle::PM_LayoutTopMargin),
pixelMetric(QStyle::PM_LayoutRightMargin),
pixelMetric(QStyle::PM_LayoutBottomMargin)
}
}

QMargins QStyle::largeMargins() const
{
return QMargins{
pixelMetric(QStyle::PM_LayoutLeftMargin) * 2,
pixelMetric(QStyle::PM_LayoutTopMargin) * 2,
pixelMetric(QStyle::PM_LayoutRightMargin) * 2,
pixelMetric(QStyle::PM_LayoutBottomMargin) * 2
}
}

then we can remove mediumSpacing from Kirigami and ensure that in both our 
Kirigami and QtWidgets apps, we use small or large only no spacing at all. We 
should still try to define some guidelines when to use large or small spacing.

c) Do nothing and accept that spacing in Kirigami apps and QtWidgets are 
different. We might still want to define in our doc/hig the usecase for 
largeSpacing and smallSpacing

Personally I see advantages and disadvantages for all these solutions. I had a 
preference on b) but while writing this mail I'm slowly liking the a bis) idea 
more and more as we might not need a small spacing and large spacing and could 
get away with just one unified spacing.

Cheers,
Carl