Re: action menu demo (again)

[Owen Taylor <otaylor redhat com>]

James Henstridge <james daa com au> writes:

> 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,

Even if things worked fully, if you set the accel group, then I think
the menu item will set up a closure to "activate" on the menu item,
rather than the one you want that activates action object, so
accelerators will appear to stop working altogether.

> 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.

Yeah, looks like there a bug here ... looks like gtkmenuitem needs
a parent_set handler that calls _gtk_menu_item_refresh_accel_path().
(Filed as #66729)

> >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.

Is the _gtk_widget_get_accel_path() check needed at all?

                                        Owen