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 673243 - GtkRadioMenuItem accelerators no longer appear
GtkRadioMenuItem accelerators no longer appear
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkMenu
3.4.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2012-03-31 16:09 UTC by Zachary Dovel
Modified: 2013-02-19 02:06 UTC
See Also:
GNOME target: ---
GNOME version: 3.3/3.4


Attachments
Use property label as in other menu items (1.92 KB, patch)
2013-02-14 21:19 UTC, Olivier Brunel (jjacky)
reviewed Details | Review
Use property label as in other menu items (2.77 KB, patch)
2013-02-14 22:46 UTC, Olivier Brunel (jjacky)
committed Details | Review

Description Zachary Dovel 2012-03-31 16:09:18 UTC
This is a regression in the 3.3.x series.  To recreate, make a program with a menu using:

GtkWidget* widget = gtk_radio_menu_item_new_with_label (di->longname);
gtk_menu_shell_append (GTK_MENU_SHELL (shell),  GTK_WIDGET (widget));
gtk_widget_add_accelerator (GTK_WIDGET (widget), "activate", accelgroup, (GDK_KEY_A), GDK_MOD1_MASK, GTK_ACCEL_VISIBLE);
gtk_widget_show (widget);

The accelerator works.  It is just not visible.  If you change gtk_radio_menu_item_new_with_label to gtk_menu_item_new_with_label, the accel will appear as it should.
Comment 1 IgnorantGuru 2013-02-14 15:50:24 UTC
Confirmed in gtk 3.4 - key shortcuts do not appear in the menu for radio items, but they do emit the activate signal.

Since this bug is almost a year old, is there any support for gtk3 or has it been abandoned?
Comment 2 Olivier Brunel (jjacky) 2013-02-14 21:19:27 UTC
Created attachment 236165 [details] [review]
Use property label as in other menu items

I'm not sure why GtkRadioMenuItem-s are still adding accel_label on their own, whereas other menu items simply use the property label; Might have been an oversight when the new API was introduced.

The patch switch to using properties, which seems to fix the problem.
Comment 3 Emmanuele Bassi (:ebassi) 2013-02-14 21:41:08 UTC
Review of attachment 236165 [details] [review]:

good patch; it may be possible to do better, tho. :-)

::: gtk/gtkradiomenuitem.c
@@ +254,2 @@
   radio_menu_item = gtk_radio_menu_item_new (group);
+  g_object_set (radio_menu_item, "label", label, NULL);

can't we do:

  return g_object_new (GTK_TYPE_RADIO_MENU_ITEM,
                       "group", group != NULL ? group->data : NULL,
                       "label", label,
                       NULL);

instead?

usually, the less a constructor function does on top of g_object_new() (ideally, nothing at all), the better.

@@ +279,2 @@
   radio_menu_item = gtk_radio_menu_item_new (group);
+  g_object_set (radio_menu_item, "label", label, "use-underline", TRUE, NULL);

same as above, we should be able to just call g_object_new().
Comment 4 Olivier Brunel (jjacky) 2013-02-14 22:46:55 UTC
Created attachment 236181 [details] [review]
Use property label as in other menu items

New version using only g_object_new() to create the widget. Also fixes a bug in set_property handler, where setting group to NULL would cause a warning.
Comment 5 Cosimo Cecchi 2013-02-18 17:11:02 UTC
Review of attachment 236181 [details] [review]:

Looks good to me.
Comment 6 Cosimo Cecchi 2013-02-19 01:49:06 UTC
Pushed to master for 3.7.10.
Comment 7 IgnorantGuru 2013-02-19 02:06:37 UTC
Thanks for addressing this.