Re: action menu demo (again)

[James Henstridge <james daa com au>]

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.

James.

--
Email: james daa com au
WWW:   http://www.daa.com.au/~james/