After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 322934 - Replace menu's proxy icons with empty space hiding icons
Replace menu's proxy icons with empty space hiding icons
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkMenu
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
: 561521 (view as bug list)
Depends on:
Blocks: 557469 585421
 
 
Reported: 2005-12-01 14:35 UTC by Luca Ferretti
Modified: 2009-06-22 13:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Example of menus: current status and suggested (14.63 KB, image/png)
2005-12-01 14:36 UTC, Luca Ferretti
  Details
An example of Apple menus (OSX 10.3, found on the Web) (11.44 KB, image/jpeg)
2005-12-01 14:39 UTC, Luca Ferretti
  Details
Another example for Apple Mac OSX showing simple menu entries and check menu entries (8.17 KB, image/jpeg)
2005-12-01 14:41 UTC, Luca Ferretti
  Details
New example of menus: current status and suggested (15.13 KB, image/png)
2005-12-01 21:03 UTC, Luca Ferretti
  Details
patch (1.09 KB, patch)
2008-10-21 00:13 UTC, William Jon McCann
none Details | Review
new patch (4.69 KB, patch)
2008-10-22 07:49 UTC, William Jon McCann
needs-work Details | Review
new patch (3.09 KB, patch)
2008-10-31 06:29 UTC, Matthias Clasen
none Details | Review

Description Luca Ferretti 2005-12-01 14:35:34 UTC
When you choose to hide proxy icons in menus (for example setting to false the
GConf key /desktop/gnome/interface/menus_have_icons in GNOME), menus with
GtkCheckMenuItem and GtkRadioMenuItem and menus with only GtkMenuItem have a
different layout (see attached screenshot).

It could be interesting (and maybe useful) add empty space on GtkMenuItem
entries to fill this gap.

Moreover, if I'm right, both MS Windows and Apple OSX do something like this in
their toolkits.
Comment 1 Luca Ferretti 2005-12-01 14:36:39 UTC
Created attachment 55473 [details]
Example of menus: current status and suggested
Comment 2 Luca Ferretti 2005-12-01 14:39:12 UTC
Created attachment 55474 [details]
An example of Apple menus (OSX 10.3, found on the Web)
Comment 3 Luca Ferretti 2005-12-01 14:41:14 UTC
Created attachment 55475 [details]
Another example for Apple Mac OSX showing simple menu entries and check menu entries

PS IMHO should are radio entries....
Comment 4 Luca Ferretti 2005-12-01 21:03:16 UTC
Created attachment 55497 [details]
New example of menus: current status and suggested

Forgot to mention: good theme engines should provide squares and circles to
suggest Radio and Check menu entries
Comment 5 William Jon McCann 2008-10-21 00:13:16 UTC
Created attachment 120973 [details] [review]
patch

Not sure if there is a better way to inspect the style properties without creating the dummy instance.  Looks good though.
Comment 6 Matthias Clasen 2008-10-21 02:40:32 UTC
bug 412134 has an api proposal to avoid some of the ugliness
Comment 7 Matthias Clasen 2008-10-21 03:26:30 UTC
I think it would be more elegant to implement 
gtkmenuitem.c:gtk_real_menu_item_toggle_size_request
to return indicator_size + toggle_spacing. 

If you do that, you can remove gtk_check_menu_item_toggle_size_request
at the same time. 

Short of adding the gtk_style_get api referred to above, you can
perhaps use _gtk_style_peek_property_value to avoid the dummy widget.
Comment 8 William Jon McCann 2008-10-21 03:53:43 UTC
I thought that it may be better to only perform the lookup once per menu (if necessary) rather than per item.  It is probably more elegant to use the baseclass though.  If we do that then we certainly don't want to be creating a dummy instance to get the property from.

I guess what we really would like here is to have indicator-size be a style property for all menu items and not just check menu items.

It isn't clear to me that _gtk_style_peek_property_value will be that useful if we move the functionality to the baseclass since the property we're interested in is in a subclass.
Comment 9 Matthias Clasen 2008-10-21 03:59:28 UTC
I meant to pass the subclass to _gtk_style_peek_property_value
Comment 10 Matthias Clasen 2008-10-21 13:58:46 UTC
Some things we noticed while comparing alternative menu systems:

- If we do this, we probably need to have some padding on the right too, to avoid
  unbalanced appearance (this only seems to affect menus without accels)

- The minimal space between label and accel should probably be adjusted too
Comment 11 William Jon McCann 2008-10-22 07:49:08 UTC
Created attachment 121093 [details] [review]
new patch

Maybe moving the style property up the widget hierarchy wouldn't be an API break since it is only used through property accessors?  If so, this patch seems to work.  It should also fix the spacing problem on the other (usually right) side of the menu item label.
Comment 12 Matthias Clasen 2008-10-26 03:25:12 UTC
Some observations:

- _gtk_style_peek_property_value tries to find style properties for parent types,
  so the gtk_widget_style_get (checkmenuitem, "indicator-size",...) will find
  a value set with GtkMenuItem::indicator-size = x in an rc file.

- The rc file parser doesn't complain about SomeGtkClass::some-property = x even
  if SomeGtkClass doesn't have a style property ::some-property. And the property
  value will be stored in the style and available for lookups.

- Most themes only set GtkCheckMenuItem::indicator-size in styles for check menu items. Those themes will be broken, since the effective padding of a menu will depend on whether it contains check menu items or not.

- Your patch doesn't work right for menus containing only image menu items without images, I think. At least I see some examples of those where the indicator size is added when menu-images is FALSE, but not when it is TRUE.
Comment 13 Matthias Clasen 2008-10-31 06:29:04 UTC
Created attachment 121715 [details] [review]
new patch

Here is a version of your original patch that uses gtk_style_get().
If we decide to go with this, gtk_style_get should of course be exported and move to gtkstyle.[hc].

The patch doesn't have the deficiencies I mentioned above for the other patch, so I tend towards this one.
Comment 14 Christian Dywan 2008-10-31 10:54:08 UTC
(In reply to comment #13)
> Created an attachment (id=121715) [edit]
> new patch
> 
> Here is a version of your original patch that uses gtk_style_get().
> If we decide to go with this, gtk_style_get should of course be exported and
> move to gtkstyle.[hc].

I didn't really look this trough in detail, but I suggest to be careful with enhancing the current GtkStyle interface. There are people working heavily on finding out if replacing GtkStyle is a good idea. Granted it won't just disappear, but maybe _gtk_style_get should be an internal, underscore prefixed function, at least for the moment.
Comment 15 Matthias Clasen 2008-10-31 13:14:53 UTC
You've said as much in the other bug. 

However, I disagree with you assessment that this adds any new functionality that would be a burden to maintain for HotNewStyle. You can see that by looking at 
Jons original patch: You can already get the style properties, at the cost of instantiating widgets all the time.

Comment 16 Christian Dywan 2008-10-31 17:30:08 UTC
(In reply to comment #15)
> You've said as much in the other bug. 
> 
> However, I disagree with you assessment that this adds any new functionality
> that would be a burden to maintain for HotNewStyle. You can see that by looking
> at Jons original patch: You can already get the style properties, at the cost of
> instantiating widgets all the time.

My primary point of concern is actually not maintenance. But in the case that we actually end up with a new style interface it would be a shame to have introduced an enhancement only to deprecate it. It would hardly be worth porting code if it's deprecated already in the next version. So from the perspetive of a developer using Gtk it would feel awkward.

That said, it may be of more use than I realize now, so my opinion is not totally carved in stone :)
Comment 17 Matthias Clasen 2008-11-01 04:33:49 UTC
Bug 322934 – Replace menu's proxy icons with empty space hiding icons

* gtk/gtkmenu.c (gtk_menu_size_request): Use consistent padding
regardless of imagees or checks being in the menu. Also add
padding on the right edge.
Proposal by Luca Ferretti, patch by Jon McCann
Comment 18 Cody Russell 2009-03-16 15:29:34 UTC
*** Bug 561521 has been marked as a duplicate of this bug. ***
Comment 19 Yeti 2009-06-21 18:20:02 UTC
Doing this unconditionally makes small multicolumn menus really ugly.  See attachment 137130 [details] in bug 585421.  Please add a way to request the original smart behaviour.
Comment 20 Yeti 2009-06-22 11:30:29 UTC
(In reply to comment #19)
> Please add a way to request the original smart behaviour.

This is easy if one really has a GtkMenu.  However, the ugly screenshot shows a GtkComboBox which does not provide any means to access the GtkMenu widget.

But maybe the new behaviour makes little sense for menus created from a GtkComboBox anyway.  Combo boxes with icons are relatively rare and those that have icons tend to have them in each item.  So simply reverting the behaviour for all combo box menus could be acceptable.
Comment 21 Matthias Clasen 2009-06-22 13:12:52 UTC
> So simply reverting the behaviour
> for all combo box menus could be acceptable.

I've done that yesterday