align checkable widgets in menu items
ClosedPublic

Authored by zzag on Feb 13 2018, 12:49 PM.

Details

Summary

Before

After

Test Plan
  • open a context menu with icons and check boxes

Diff Detail

Repository
R31 Breeze
Branch
center-checkbox
Lint
No Linters Available
Unit
No Unit Test Coverage
zzag created this revision.Feb 13 2018, 12:49 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 13 2018, 12:49 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
zzag requested review of this revision.Feb 13 2018, 12:49 PM
zzag edited the summary of this revision. (Show Details)Feb 13 2018, 12:52 PM

the diff appears more complex than it actually is because of unrelated changes. Please keep the changes to the minimum, this will help reviewing.

kstyle/breeze.h
70 ↗(On Diff #27051)

This change is unrelated with the centering. Should be another patch.

kstyle/breezestyle.cpp
4660 ↗(On Diff #27051)

this change is unrelated. Please revert.

4701 ↗(On Diff #27051)

why was this chunk of code moved ? This is unrelated to the change.
Please try keep the diff to the minimum

4744 ↗(On Diff #27051)

please dont rename variables just for the safe of it, and keep checkboxrect.

Also, code breaks in the following configurations:

(see the incorrect centering of the radio button)
this is because you have to rely on maxIconWidth, even if the current item has no icon, when computing the checkbox space.
This will maintain the same alignment for the full menu.

zzag updated this revision to Diff 27062.Feb 13 2018, 2:03 PM

delete changes not related to this diff

zzag marked 2 inline comments as done.Feb 13 2018, 2:20 PM
zzag added inline comments.
kstyle/breezestyle.cpp
4701 ↗(On Diff #27051)

To keep it consistent:

  1. compute required rectangles
  2. align checkbox rect
  3. paint text, arrows, etc.

Could you please let it stay here?

zzag added a comment.Feb 13 2018, 2:37 PM

this is because you have to rely on maxIconWidth, even if the current item has no icon, when computing the checkbox space.

I've came up with something like this

if( showIcon && menuItemOption->maxIconWidth > 0 )
{
    int dx = (iconRect.left() - Metrics::CheckBox_Size) / 2;
    checkBoxRect.moveLeft( dx );
} else {
    int dx = (textRect.left() - Metrics::CheckBox_Size) / 2;
    checkBoxRect.moveLeft( dx );
}

so, it centers properly checkboxes in menu items with and without icons

(it can be simplified with a ternary operator)

is it a good approach?

zzag updated this revision to Diff 27072.Feb 13 2018, 3:57 PM

fix checkbox aligning when there is no any icon

Ok. So, the patch works, but ... there is something fishy here. In principle it should not be needed. Right ?
Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, having something laied out as: margin + checkbox + spacing + icon + spacing + text would be automatically centered, no ?
Idem without icon. (and if there is double spacing, then it is a bug).
At least this is how the code is supposed to work. If not, it should be fixed, rather than circomvoluting the issue with some centering.
(note that the padding on the icons is disconnected to this: not all icons have padding, especially if you change icon theme, so you cannot do anything but rely on the icon size.

Bottom line, I would like to investigate the current code (which _should_ work), rather than accepting this patch.
Or do I miss something ?

The reason why I would rather fix the current code than introducing some centering, is because centering would automatically hide deeper problems, and because it makes the metrics (margin, icon width, spacing), not reflect what the code do. Which then introduces confusion and make the code not maintainable.

zzag added a comment.EditedFeb 13 2018, 8:52 PM

In principle it should not be needed. Right ?

Yes.

Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, having something laied out as: margin + checkbox + spacing + icon + spacing + text would be automatically centered, no ?

Well, in theory, yes, it would be centered. But, humans often perceive something to be centered when it's not actually centered, for example, in typography, letters like "O" or "S" are often put below a baseline. So, maybe, checkboxes should be even a little bit closer to icons/text.

Edit
To make things clear: I'm not telling "checkboxes should be closer to icons.

At a given moment, here's how things laid out:

margin + checkbox_size + spacing + 1 + [2 + PM_SmallIconSize + 2] + spacing + 1 + text

Also, why is there the number 1?

The reason why I would rather fix the current code than introducing some centering, is because centering would automatically hide deeper problems, and because it makes the metrics (margin, icon width, spacing), not reflect what the code do. Which then introduces confusion and make the code not maintainable.

Agreed.

In D10480#205772, @zzag wrote:

In principle it should not be needed. Right ?

Yes.

Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, having something laied out as: margin + checkbox + spacing + icon + spacing + text would be automatically centered, no ?

Well, in theory, yes, it would be centered. But, humans often perceive something to be centered when it's not actually centered, for example, in typography, letters like "O" or "S" are often put below a baseline. So, maybe, checkboxes should be even a little bit closer to icons/text.

Edit
To make things clear: I'm not telling "checkboxes should be closer to icons.

At a given moment, here's how things laid out:

margin + checkbox_size + spacing + 1 + [2 + PM_SmallIconSize + 2] + spacing + 1 + text

Also, why is there the number 1?

Yes. That's what needs fixing. I know there are subtleties with rect.left + rect.width() vs rect.right(). You can easily loose or gain a +1 there.
Also, fonts might not start exactly at the rect depending on hinting, which letter is drawn etc.
But anyway. This can be investigated, if only by actually drawing the relevant rect and see where space is lost, misaligned, etc.

zzag added a comment.Feb 13 2018, 10:33 PM

Side note: This patch tries to center every checkbox between the left border and blue rect(and it also ignores margins.....)

Why are there only 2 pixels?

After playing a bit with margin values I've found 5 to be reasonable.

zzag updated this revision to Diff 27116.Feb 13 2018, 11:08 PM
  • try to align check boxes by increasing MenuItem_MarginWidth
  • fix another double space problem

Why are there only 2 pixels?

Because the darker pixel, around the menu is also part of the menu, not the shadow. (meaning, there is a one pixel frame that is rendered in the margin).

After playing a bit with margin values I've found 5 to be reasonable.

So 5 does makes sense: there is 1 for the frame, and then 4, that matches the spacing. Right ?

zzag added a comment.Feb 14 2018, 12:21 PM

So 5 does makes sense: there is 1 for the frame, and then 4, that matches the spacing. Right ?

Yes, I guess so.

zzag abandoned this revision.Feb 15 2018, 9:41 AM

Hello,
I do not think this revision should be abbandonned just yet.
The part that takes care of removing double spaces when icon is not present (when no icon is present in the menu), should go in.
The part about increasing the side margins to make the checkboxes centered, should still be discussed: It is not clear to me that margins and spacing between items should be identical, and for instance see how the positioning appears off with respect to the bottom of the menu, when checkboxes is present on the last item of the menu.
One solution to fix this would be to also increase the bottom margins, but then in turn that makes the item selection funny. (asymetric top bottom). Unless of course one either

  • make the selection not extends to the end of the menu any more
  • increase the spacing between items to restore the symmetry.

Bottom line: this is a can of worms.

At the minimum, bugs (double spacing) should be fixed.

zzag added a comment.EditedFeb 15 2018, 11:15 AM

Well, I could rework this patch to fix double spacing.

Centering of check boxes would involve some changes in margins, which should be another diff, right?

positioning appears off with respect to the bottom of the menu, when checkboxes is present on the last item of the menu.

Hmm, I don't see it. Could you please show where positioning is off?

In D10480#206782, @zzag wrote:

Well, I could rework this patch to fix double spacing.

Centering of check boxes would involve some changes in margins, which should be another diff, right?

positioning appears off with respect to the bottom of the menu, when checkboxes is present on the last item of the menu.

Hmm, I don't see it. Could you please show where positioning is off?

Here: if you look at the last checkbox (folders first). The distance to the left edge is larger to that of the bottom edge, while before they were identical.
This is because margin width and margin height are different.
which in turn is needed for centering.
So this really depends on what we want to align with what ...

zzag added a comment.Feb 16 2018, 4:37 AM

Here: if you look at the last checkbox (folders first). The distance to the left edge is larger to that of the bottom edge, while before they were identical.

Oh, I haven't noticed that before. To me, that's still fine.

So this really depends on what we want to align with what ...

I'd propose to align check boxes between the left edge and icon/text. (or leave it as it is right now)

In D10480#207511, @zzag wrote:

Here: if you look at the last checkbox (folders first). The distance to the left edge is larger to that of the bottom edge, while before they were identical.

Oh, I haven't noticed that before. To me, that's still fine.

So this really depends on what we want to align with what ...

I'd propose to align check boxes between the left edge and icon/text.

ok. I agree.

See comments below: if you can split this patch in two (first bug fix, then margin width increase, then both can go.

Thanks for the good work !

kstyle/breezestyle.cpp
4771 ↗(On Diff #27051)

If I understand right, this is the double spacing bug fix.
Correct ? Very nice.
In principle, it would be better to have it in a separate Review, and a separate commit.
This way, if in the future someone wants to revert the margin change, she/he does not revert the bug fix at the same time. Can you do that ?
Note that thinking about it, I also like the other fix (the margin width increase), so this will go too.

zzag reclaimed this revision.Feb 16 2018, 11:18 AM
zzag added inline comments.
kstyle/breezestyle.cpp
4771 ↗(On Diff #27051)

This way, if in the future someone wants to revert the margin change, she/he does not revert the bug fix at the same time. Can you do that ?

Yes.

zzag updated this revision to Diff 27344.Feb 16 2018, 2:34 PM

delete double space bug fix

zzag edited the summary of this revision. (Show Details)Feb 16 2018, 2:35 PM
zzag edited the summary of this revision. (Show Details)
hpereiradacosta accepted this revision.Feb 16 2018, 4:22 PM

Ship it !
Thanks !

This revision is now accepted and ready to land.Feb 16 2018, 4:22 PM
This revision was automatically updated to reflect the committed changes.