reserve space for checkable widgets in menu items
ClosedPublic

Authored by zzag on Feb 11 2018, 8:52 AM.

Details

Summary

At a given moment, menu items do not have empty space on the left side.
This feels not really comfortable or "natural"(I don't know how to
express this feeling).

This revision tries to solve the problem above by reserving space for checkable
widgets. Also, it achieves consistency with macOS/Windows/Unity/etc.

Before

After

Context menus with checkable widgets(e.g. radio buttons or checkboxes) look the same

Test Plan
  • open context menu

Diff Detail

Repository
R31 Breeze
Branch
menuitem-margins
Lint
No Linters Available
Unit
No Unit Test Coverage
zzag created this revision.Feb 11 2018, 8:52 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 11 2018, 8:52 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
zzag requested review of this revision.Feb 11 2018, 8:52 AM
zzag edited the summary of this revision. (Show Details)Feb 11 2018, 8:53 AM
zzag edited the summary of this revision. (Show Details)Feb 11 2018, 8:56 AM

How does it look when a menu has a checkbox on it, for example the 'View' menu on Konsole


What other DE or OS that you used has such a wide menu that it feels natural to you?

I could maybe see this working for the left edge, but having such a lot of padding between the right edge and the arrows seems a little weird to me.

abetts added a subscriber: abetts.Feb 12 2018, 12:39 AM

This could work, just the left padding is too much in this patch. Maybe reduce by a 3rd? or Half?

zzag updated this revision to Diff 26980.Feb 12 2018, 7:33 AM

allow changing individual margins(e.g. left/right/top/bottom margin)

zzag added a comment.Feb 12 2018, 7:37 AM

@ngraham, @abetts

Margins:

  • left: 20
  • right: 6
  • top: 3
  • bottom: 3

What do you think of this?

This could work, just the left padding is too much in this patch. Maybe reduce by a 3rd? or Half?

In my opinion, it should be even bigger. (regarding 16px(?) left padding)


@anemeth see screenshots above

zzag added a comment.EditedFeb 12 2018, 7:41 AM

What other DE or OS that you used has such a wide menu that it feels natural to you?

WIndows and macOS.

Also, context menus are a little wide in google chrome and firefox.

I like the margins how they are at the moment and personally would increase it maximum by 5 px or so.
The new margins shown in the screenshot are just too huge for my taste.

zzag added a comment.Feb 12 2018, 8:44 AM

@mmustac what about this?

margins:

  • left: 10
  • right: 4
  • top,bottom: 3

The new margins shown in the screenshot are just too huge for my taste.

Well, it's hard to target each personal taste. For me, bigger left margin feels more natural. Typical the "for my taste" problem.

In D10438#204757, @zzag wrote:

@mmustac what about this?

margins:

  • left: 10
  • right: 4
  • top,bottom: 3

    ---

The new margins shown in the screenshot are just too huge for my taste.

Well, it's hard to target each personal taste. For me, bigger left margin feels more natural. Typical the "for my taste" problem.

Sorry to say but to my taste current margins are fine and new one two large
Now, to address the taste issue:
1 What is the use case?
2 are there bug reports about margins being too narrow? (From our users?)
3 is this consistent with other margins used elsewhere in breeze ?

For 2 at leasthe, bug reports are generally theother way around, from people complaining that breeze wastes too much space

So -1 from my side sorry.

According to the HIG units.smallSpacing (4px) or units.largeSpacing (18px) or multiples of these should be used when ever possible, maybe it would make sense to uses these?

januz added a subscriber: januz.Feb 12 2018, 1:03 PM

IMO, the last version looks better than the current menu. That said, I think the top/bottom paddings are still too tight, I would try adding 2-3px for each.
It's true that there's a question of taste but more whitespace is generally a good thing (unless you go overboard and start making huge widgets). A couple more pixels in the menus will help focus the elements better (by framing them in negative space), it will make the ui look less "full of stuff" and less tense.

For reference:

Material design manual: https://material.io/guidelines/components/menus.html#menus-usage
Gnome: http://i.imgur.com/er2odvE.png
Mac: https://www.intego.com/mac-security-blog/wp-content/uploads/2016/12/Mac-menu-bar-extras-sound.png
Windows: https://docs.microsoft.com/en-us/windows/uwp/design/controls-and-patterns/menus

IMO, the last version looks better than the current menu. That said, I think the top/bottom paddings are still too tight, I would try adding 2-3px for each.
It's true that there's a question of taste but more whitespace is generally a good thing (unless you go overboard and start making huge widgets). A couple more pixels in the menus will help focus the elements better (by framing them in negative space), it will make the ui look less "full of stuff" and less tense.

For reference:

Material design manual: https://material.io/guidelines/components/menus.html#menus-usage

This is a touch based ui.

Gnome: http://i.imgur.com/er2odvE.png
Mac: https://www.intego.com/mac-security-blog/wp-content/uploads/2016/12/Mac-menu-bar-extras-sound.png
Windows: https://docs.microsoft.com/en-us/windows/uwp/design/controls-and-patterns/menus

None of these are application's menu. (though I did not check if applications menus are narrower in these three cases)

In anycase, increasing margins should then be made consistently accross the style and not one by one if you want to keep balance.
For gnome, for instance, see how _all_ margins are larger (and thus consistent).

So far, on bugzilla there have been more complains about breeze being too space-hungry than too dense.
(I can post the links here if needed)

also see how in two the posted screenshots the checkmarks for checkable items is actually very close to the menu border.

According to the HIG units.smallSpacing (4px) or units.largeSpacing (18px) or multiples of these should be used when ever possible, maybe it would make sense to uses these?

Thats a good point. One should use update the breeze metrics (which predate the HIG), to use these whenever possible.
This is largely unrelated to this patch though.

zzag added a comment.Feb 12 2018, 3:02 PM

Just to clarify that other DE and OS have some space on the left side:

even Ubuntu(Unity):

Typical pattern:
[<checkable>] [<image>] <text> [<shortcut> or <arrow>]
in the case when a menu item doesn't have a checkbox or radiobutton, some space is reserved(more precisely, width of checkbox/radiobutton).

So, the left margin should be even bigger - 28. (CheckBox_Size + 2*MenuItem_ItemSpacing)


According to the HIG units.smallSpacing (4px) or units.largeSpacing (18px) or multiples of these should be used when ever possible, maybe it would make sense to uses these?

So, margins should be preferred to multiples of 4 or 18, right?

also see how in two the posted screenshots the checkmarks for checkable items is actually very close to the menu border

It can be changed. Currently, distance between menu's left border and checkboxes equals to MenuItem_ItemSpacing.

zzag added a comment.Feb 12 2018, 3:10 PM

In anycase, increasing margins should then be made consistently accross the style and not one by one if you want to keep balance.

No, it should not. This is a small change which doesn't require rethinking the whole design, imo.

For gnome, for instance, see how _all_ margins are larger (and thus consistent).

GNOME just loves "nom-nom-nom available display space".

In D10438#204922, @zzag wrote:

you're right
one more thing I noticed in the screenshots here that there is no keyboard shortcut hint that makes the menu double the width it should be

maybe in an another patch that could be removed too?

In D10438#204922, @zzag wrote:

you're right
one more thing I noticed in the screenshots here that there is no keyboard shortcut hint that makes the menu double the width it should be

maybe in an another patch that could be removed too?

That would be a serious regression to many users. How would you then know about the shortcuts ?

We cannot ever remove the shortcuts from the menus, for countless usability reasons. Lack of discoverable keyboard shortcuts is one of the many productivity and ergonomic flaws of GNOME; we cannot regress to that level.

in macOS, the shortcuts are more compact because symbols are used instead of letters:

I'm not sure we could use that here because PC keyboards' modifier keys don't have symbols on them the way Mac keyboards generally do.

In D10438#204922, @zzag wrote:

Just to clarify that other DE and OS have some space on the left side:

Agreed, and if others from vdg agree, I am fine with always adding the necessary space for checkboxes and radio buttons on the left.
But then that makes it a different patch, right ?

Margins are not changed. What is changed is the allocated space for the menu item content, that always accommodate for the checkbox, and draws it whenever there is one.

Personally, I think it is better that this space is not allocated when there is no crossheck to be drawn in the full menu. I think it is an _improvement_ with respect to the other design you send, and would see this change rather as a regression. (I would not see why you allocate some empty space if there is nothing to render in it).

But then, if VDG agrees, I would oblige.

So, the left margin should be even bigger - 28. (CheckBox_Size + 2*MenuItem_ItemSpacing)

How are we accounting for stacking menu items that have:

Checkbox - Item Icon - Label -- Shortcut-Arrow Icon

The first part of that tends to make the checkbox be closest to the edge. Are we thinking we should/not use icons in menus, keep the same?

anemeth added a comment.EditedFeb 12 2018, 4:22 PM

Are we thinking we should/not use icons in menus, keep the same?

Looks quite good in my opinion with this patch applied and no icons

I just discovered that disabling icons in menus also disabled the keyboard shortcut hint.
Really nice

Let's try to avoid radical proposals like getting rid of keyboard shortcuts and icons. For now, let's focus on the initial goal, or perhaps a slightly modified goal of adding room to put checkboxes to the left of the text (which I would support).

Let's try to avoid radical proposals like getting rid of keyboard shortcuts and icons. For now, let's focus on the initial goal, or perhaps a slightly modified goal of adding room to put checkboxes to the left of the text (which I would support).

Can you answer the question though?

zzag added a comment.Feb 12 2018, 4:27 PM

Let's try to avoid radical proposals like getting rid of keyboard shortcuts and icons. For now, let's focus on the initial goal, or perhaps a slightly modified goal of adding room to put checkboxes to the left of the text (which I would support).

I haven't got rid of keyboard shortcuts. I've encountered this bug too. With ShowIconsInMenuItems set to false, QStyleOptionMenuItem doesn't pass shortcuts.

Let's try to avoid radical proposals like getting rid of keyboard shortcuts and icons. For now, let's focus on the initial goal, or perhaps a slightly modified goal of adding room to put checkboxes to the left of the text (which I would support).

Can you answer the question though?

I would not be in favor of removing the menu item icons, no. This would introduce inconsistency issues all over the place and represents a fairly significant departure from a long-standing UI. If we ever do that, it would have to:

  • Present clear usability and visual advantages that enough folks agree on
  • Involve significant discussion of the ramifications
  • Be in in its own patch
  • Involve a great deal of testing to make sure there's no fallout
  • etc

...In short, not something worth bringing up in the discussion of this patch.

zzag added a comment.Feb 12 2018, 5:26 PM

But then that makes it a different patch, right ?

Why?

Margins are not changed. What is changed is the allocated space for the menu item content, that always accommodate for the checkbox, and draws it whenever there is one.

No, margins still have to be changed. I would like to have static margins but how to add space on the left side if there is no any menu items with a checkable widget? Maybe I'm missing something? I should have added the word reserved in quotes.

The first part of that tends to make the checkbox be closest to the edge. Are we thinking we should/not use icons in menus, keep the same?

What do you mean?

zzag added a comment.Feb 12 2018, 5:30 PM

Did you change the left margin? Could you please test the left margin with values 20/24/28 and pick the best one?

In D10438#205041, @zzag wrote:

But then that makes it a different patch, right ?

Why?

Because I know the code: I wrote it.

Margins are not changed. What is changed is the allocated space for the menu item content, that always accommodate for the checkbox, and draws it whenever there is one.

No, margins still have to be changed. I would like to have static margins but how to add space on the left side if there is no any menu items with a checkable widget? Maybe I'm missing something? I should have added the word reserved in quotes.

This is incorrect.
The only thing you need to change to accomodate for enough space for checkboxes on all menu items disregarding whether there is a checkbox or not is to comment the two calls to:

if( menuItemOption->menuHasCheckableItems )

Thats a very small patch that requires no change to the margins.

In D10438#205042, @zzag wrote:

Did you change the left margin? Could you please test the left margin with values 20/24/28 and pick the best one?

I didn't change the margins previously, only applied the patch. Here are they changed now:


20 looks better, but 28 looks centered.
Since this is with icons + shortcuts disabled it should not be used as reference.

zzag updated this revision to Diff 27019.Feb 12 2018, 6:16 PM

reserve space for checkable widgets in menu items

zzag added a comment.Feb 12 2018, 6:17 PM

How it currently looks:

@zzag Did you accidentally removed the changes in breeze.h and breezestyle.h? :)

zzag retitled this revision from increase left/right margin of menuitems to reserve space for checkable widgets in menu items.Feb 12 2018, 6:26 PM
zzag edited the summary of this revision. (Show Details)

@zzag Did you accidentally removed the changes in breeze.h and breezestyle.h? :)

Yes. There is no point to keep them.

anemeth added a comment.EditedFeb 12 2018, 6:29 PM
In D10438#205119, @zzag wrote:

@zzag Did you accidentally removed the changes in breeze.h and breezestyle.h? :)

Yes. There is no point to keep them.

Whoops, I misunderstood your prev comment.
I personally like this approach better than the previous.
Could you change the image in the summary too?

zzag added a comment.Feb 12 2018, 6:31 PM

Whoops, I misunderstood your prev comment.

I've deleted changes in breeze.h and breezestyle.h on purpose.

zzag edited the summary of this revision. (Show Details)Feb 12 2018, 6:33 PM

+1 on this new approach.

Maybe you could align the checkbox better?
The numbers are pixels

zzag added a comment.Feb 12 2018, 8:07 PM

Maybe you could align the checkbox better?
The numbers are pixels

I don't really think this diff should mess with margins. I'd would like to leave it as it is right now. Maybe another diff should be created regarding checkbox spacing.


Explanation behind numbers:

  • 4: it should be 3(marginwidth) + 1(measurement error?)
  • 9: 4(itemspacing) + 1 + 2[(20 - 16) / 2] + 2(icons have padding)
  • 8: 2[(20 - 16) / 2] + 4(itemspacing) + 1 + 1(measurement error?)
zzag added a comment.Feb 13 2018, 12:12 AM

@anemeth here are menu items with centered check boxes and radio buttons:

it's still not perfectly aligned because some icons have inner padding. So, that's the best what I can do.

Also, I've increased MenuItem_MarginWidth to 4(according to @fabianr's comment).

That's all.


I'm not sure whether these changes should come with this diff or maybe I should create another diff which "fixes" alignment of check boxes. @ngraham, @hpereiradacosta, @abetts what do you think?


Also, is there a bug report about missing shortcut hints when icons in menus are disabled?

hpereiradacosta accepted this revision.Feb 13 2018, 8:50 AM
In D10438#205230, @zzag wrote:

I'm not sure whether these changes should come with this diff or maybe I should create another diff which "fixes" alignment of check boxes. @ngraham, @hpereiradacosta, @abetts what do you think?

This should go in a different patch.


Also, is there a bug report about missing shortcut hints when icons in menus are disabled?

Not that I know. Also: I cannot reproduce. Here, if I uncheck "Show Icons in Menus" in system settings->widget style->Fine tuning, icons dissapear and shortcut are still there.
Or are we talking about a different settings ?

In any case, this change is ready to land. Thanks !

This revision is now accepted and ready to land.Feb 13 2018, 8:50 AM

@zzag It would be greate if you could add a screenshot of the new context menu to https://community.kde.org/KDE_Visual_Design_Group/HIG/ContextMenu#Examples . Of cause other improvements/corrections to the page are more then welcome, too. If you hagve question you can ping me on IRC/Telegram in the vdg channel.

zzag added a comment.Feb 13 2018, 1:11 PM

Not that I know. Also: I cannot reproduce. Here, if I uncheck "Show Icons in Menus" in system settings->widget style->Fine tuning, icons dissapear and shortcut are still there.

Seems like it's only relevant to Arch Linux and Arch Linux-based distros. KDE neon doesn't have such issues.
Qt version: 5.10

It would be greate if you could add a screenshot of the new context menu to https://community.kde.org/KDE_Visual_Design_Group/HIG/ContextMenu#Examples . Of cause other improvements/corrections to the page are more then welcome, too.

Sure, I'll add later.

This revision was automatically updated to reflect the committed changes.
hpereiradacosta added subscribers: colomar, alake.EditedFeb 13 2018, 2:53 PM

So ... I applied the patch (which I approved), used it, and ... don't like it sorry.
See attached screenshots
I think the unnecessary space breaks alignment with the menubar above. and is completely unnecessary. Is this _really_ what we want ? @ngraham ? @alake ? @colomar

FWIW I like it, but since this is mostly subjective, I wouldn't fight to the death to retain if if everyone else suddenly hated it.

FWIW I like it, but since this is mostly subjective, I wouldn't fight to the death to retain if if everyone else suddenly hated it.

ok. Maybe it is just a question to get used to it.
also we'll see if we get user feedback.

zzag added a comment.Feb 13 2018, 3:12 PM

Maybe an option like "Reserve space for check boxes" should be added in fine tuning settings?

I am a user :-D
To be honest I liked Vlad`s proposal after my first comment a lot. { margin-left: 10px; }
The current implementation is nothing I would really like to have and be definitly a reason to search for a solution or another theme (which would propably be the easier part for most of the poeple, specially new ones).

zzag added a comment.Feb 13 2018, 3:51 PM
This comment was removed by zzag.
In D10438#205519, @zzag wrote:

Maybe an option like "Reserve space for check boxes" should be added in fine tuning settings?

Please no option. If we put an option to every feature we are not sure about, then we end up with unmaintainable and cluttered code, and only offer a cluttered style to the user.
Either we like this feature and it stays, or we dont and it goes.

... another screenshot: