Re: action menu demo (again)

Owen Taylor wrote:

James Henstridge <james daa com au> writes:

I didn't get any response about the problems I was having with dynamic
accel changes with my action menu demo, so I investigated it a bit
further.  It looks like _gtk_widget_get_accel_path() was returning
NULL for my menu items even though I had called
gtk_menu_item_set_accel_path() on them.

As a quick fix, I applied the following patch, which checks
menu_item->accel_path if _gtk_widget_get_accel_path() returns
NULL. With this patch applied, the code works perfectly.

To some extent, this change could be considered to break the current
behavior, which is:

 A menu item has a changeable accelerator if:
a) GTK+ has or can create a path for the menu item
   b) There is an accelerator group set for the menu

Since your change ends up removing b). What the condition _actually_
should be is:

This probably indicates a bug in gtk then. I initially tried adding the gtk_menu_set_accel_group() calls before doing the gtkmenu.c patch, but it still resulted in _gtk_widget_get_accel_path() returning NULL. I guess it has something to do with the ordering of the calls, as GtkItemFactory and GnomeUIInfo (which I recently converted to use the new APIs) both work as expected. My code did:
   menu = gtk_menu_new()
   gtk_menu_set_accel_group(menu, accel_gorup)

   menu_item = gtk_menu_item_new_with_mnemonic(...);
   gtk_menu_item_set_accel_path(menu_item, ...);
   gtk_container_add(menu, menu_item);

This didn't work at all. It looks like GtkItemFactory effectively has the last two calls reversed. However, in my code I don't actually want to set an accel on the menu item anyway.

 A menu item has a changeable accelerator if:
a) GTK+ has or can create a path for the menu item
   b) There is an accelerator closure that the menu item is watching

But because, having menu items where the closure is not from the menu item wasn't thought about enough when the API was designed, we don't really
have a good way to do b). We could probably hunt through the menu
heirarchy for an accelerator label with an accelerator closure set,
but I doubt it's worth it. I think it's not very likely we'll have b)
without a).

So, I think you change is fine, However, I'd suggest that there really
isn't a point in checking both _gtk_widget_get_accel_path() and
GTK_MENU_ITEM (menu_item)->accel_path.

@@ -1661,6 +1661,8 @@ gtk_menu_key_press (GtkWidget    *widget,
      path = _gtk_widget_get_accel_path (menu_item);
      if (!path)
+    path = GTK_MENU_ITEM (menu_item)->accel_path;
+      if (!path)

I'd just do the second check immediately. Why don't you go ahead and
make this change, and if Tim thinks that not catching the missing
accelerator group is a problem we can implement the search for
an accelerator label.

okay.  I will check in a modified version with the checks reversed.


Email: james daa com au

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]