Re: action menu demo (again)

[Owen Taylor <otaylor redhat com>]

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

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.

Regards,
                                        Owen

> Index: gtkmenu.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkmenu.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 gtkmenu.c
> --- gtkmenu.c    2001/11/29 20:36:12    1.78
> +++ gtkmenu.c    2001/12/07 06:42:03
> @@ -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)
>      {
>        /* can't change accelerators on menu_items without paths
>         * (basically, those items are accelerator-locked).