Re: Patch to use normal focus system for menu keynav

[Owen Taylor <otaylor redhat com>]

"Padraig O'Briain" <Padraig Obriain Sun COM> writes:

> This patch attempts to address one of the issues Owen raised in 
> http://mail.gnome.org/archives/gtk-devel-list/2001-October/msg00224.html.
> 
> A grab is not done when F10 is used to focus the menubar or a item in the 
> menubar is clicked by the mouse. Instead the focus is changed to the menubar and 
> is changed back to the previously focussed widget when leaving the menu bar.
> 
> Can this patch be committed?

Hmmm, I guess I'd really have to see it in the context of the full
change to be able to give a real answer.

> Index: gtk/gtkmenushell.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkmenushell.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 gtkmenushell.c
> --- gtk/gtkmenushell.c	2002/01/16 08:53:15	1.51
> +++ gtk/gtkmenushell.c	2002/01/16 09:25:33
> @@ -147,6 +147,9 @@ static void gtk_real_menu_shell_activate
>  						  gboolean           
> force_hide);
>  static void gtk_real_menu_shell_cancel           (GtkMenuShell      
> *menu_shell);
>  
> +static void focus_widget_destroyed           (gpointer          data,
> +                                              GtkWindow         *window);
> +
>  static GtkContainerClass *parent_class = NULL;
>  static guint menu_shell_signals[LAST_SIGNAL] = { 0 };
>  
> @@ -402,7 +405,7 @@ gtk_menu_shell_button_press (GtkWidget  
>      {
>        if (!menu_shell->active)
>  	{
> -	  gtk_grab_add (GTK_WIDGET (widget));
> +	  _gtk_menu_shell_activate (menu_shell);
>  	  menu_shell->have_grab = TRUE;
>  	  menu_shell->active = TRUE;

 * Setting the have_grab flag but not grabbing strikes me as (at the least)
   pretty confusing. If we are reusing this flag to mean 'have focus' we
   should rename it as well. 

 * If you are encapsulating the "grabbing"into _menu_shell_activate, you
   should probably move the setting of have_grab and active as well.

>  	}
> @@ -736,8 +739,42 @@ gtk_real_menu_shell_deactivate (GtkMenuS
>  
>        if (menu_shell->have_grab)
>  	{
> +          GtkWidget *toplevel;
> +
> +          toplevel = gtk_widget_get_toplevel (GTK_WIDGET (menu_shell));
> +          if (toplevel && GTK_WIDGET_TOPLEVEL (toplevel))
> +            {
> +              GtkWindow *window;
> +              GtkWidget *previous_focus;
> +
> +              window = GTK_WINDOW (toplevel);
> +              previous_focus = g_object_get_data (G_OBJECT (window), 
> +                                                  "gtk-previous-focus-widget");
> +
> +              if (previous_focus)
> +                {
> +                  g_object_weak_unref (G_OBJECT (previous_focus), 
> +                                        (GWeakNotify) focus_widget_destroyed,
> +                                       window);
> +                  g_object_set_data (G_OBJECT (window), 
> +                                     "gtk-previous-focus-widget",
> +                                     NULL);
> +                  if (gtk_widget_is_ancestor (previous_focus, toplevel))
> +                    {
> +	              gtk_window_set_focus (window, previous_focus);
> +                    }

 * Style nitpick, we don't include braces for single line blocks like this

> +                  else
> +                    {
> +	              gtk_window_set_focus (window, NULL);
> +                    }
> +
> +                }
> +              else
> +                {
> +	          gtk_window_set_focus (window, previous_focus);
> +                }
> +            }
>  	  menu_shell->have_grab = FALSE;
> -	  gtk_grab_remove (GTK_WIDGET (menu_shell));


> +_gtk_menu_shell_activate (GtkMenuShell *menu_shell)
> +{
> +  GtkWidget *toplevel;
> +
> +  toplevel = gtk_widget_get_toplevel (GTK_WIDGET (menu_shell));
> +  if (toplevel && GTK_WIDGET_TOPLEVEL (toplevel))
> +    {
> +      GtkWindow *window;
> +      GtkWidget *focus_widget;
> +
> +      window = GTK_WINDOW (toplevel);
> +      focus_widget = window->focus_widget;
> +
> +      if (focus_widget && !GTK_IS_MENU_SHELL (focus_widget))
> +        {
> +          GtkWidget *previous_focus;
> +
> +          previous_focus = g_object_get_data (G_OBJECT (window), 
> +                                              "gtk-previous-focus-widget");
> +          if (!previous_focus)
> +            {
> +              g_object_set_data (G_OBJECT (window), 
> +                                 "gtk-previous-focus-widget",
> +                                 focus_widget);
> +              g_object_weak_ref (G_OBJECT (focus_widget),
> +                                 (GWeakNotify) focus_widget_destroyed, 
> +                                 window);

I'd probably want to take into account here the possibility that the toplevel
might be destroyed before the menu shell is deactivated. I'm not sure
that this can happen, but it is easier to worry about the reference counting
separately from the overall code flow.
> +
> +            }
> +        }
> +      gtk_widget_grab_focus (GTK_WIDGET (menu_shell));

I don't think this is actually doing anything. The menu shell doesn't have
the CAN_FOCUS flag set on it, so this silently does nothing. (Try focusing
another widget then popping up the menu... the other widgets remains
displayed as focused)

Regards,
                                        Owen