Re: action menu demo (again)

[Tim Janik <timj gtk org>]

On 10 Dec 2001, 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:
> 
>   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).

i actually had proxying for menu items in mind when designing the API,
however i wanted to wait for further comments on this before adding API.
what i had in mind were basically two options:
1) set a ::activate proxy closure or a proxy object on a menu item
2) leave the current API as it is and let the proxy code connect to
   the menu item's ::activate signal (which gmenu currently does
   anyway to catch mouse clicks). this involves the menu items to
   have accel_groups of course (as (b) above indicates)

i don't quite see why james can't provide the accel_group on the menu items
and have things working that way, james, could you provide more details
here please?

on having (b) without (a), it might be unlikely currently, but is actually
imaginable (e.g. using some third party menu item factory that always sets
menu item accel paths explicitely).
the problem i see is that with such a menu item, users can setup accelerators
(possibly replacing accelerators of other widgtes) without being actually
able to activate the item which is a highly confusing scenario.

so i'd rather not get rid of (b), but either 1) get gmenu alike proxying
working simply by providing the accel group for menu items, or 2) adding
explicit API for proxying. the current situation is more like creating
a bug explicitely and gmenu exploiting it.

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

_gtk_widget_get_accel_path() should actually be considered the canonical
accel path for a widget (whic hof course is the reason for it returning
NULL if there's no accel group on a menu item, while menu_item->accel_path
is still set) menu_item->accel_path is more of an internal temporary to
survive accel_group changes.

> 
> Regards,
>                                         Owen
> 

---
ciaoTJ