Re: patch #59456, #59456-2, #64977 -- menu stuff

[Owen Taylor <otaylor redhat com>]

Kristian Rietveld <kristian planet nl> writes:

> Hi all,
> 
> This patch implements some GtkMenu stuff, which makes the menu work 
> somewhat saner. ChangeLog has been appended, patch has been attached.

Looks good, with a few comments below.

> Index: gtkmenu.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkmenu.c,v
> retrieving revision 1.76
> diff -u -r1.76 gtkmenu.c
> --- gtkmenu.c	2001/11/20 23:43:02	1.76
> +++ gtkmenu.c	2001/11/27 17:43:44
> @@ -33,6 +33,7 @@
>  #include "gtkmain.h"
>  #include "gtkmenu.h"
>  #include "gtkmenuitem.h"
> +#include "gtkseparatormenuitem.h"
>  #include "gtksignal.h"
>  #include "gtkwindow.h"
>  #include "gtkhbox.h"
> @@ -1716,7 +1717,11 @@
>     * which may be different from 'widget'.
>     */
>    menu_item = gtk_get_event_widget ((GdkEvent*) event);
> -  if (!menu_item || !GTK_IS_MENU_ITEM (menu_item) || !GTK_WIDGET_IS_SENSITIVE (menu_item) ||
> +  if (!menu_item || !GTK_IS_MENU_ITEM (menu_item) ||
> +      !GTK_WIDGET_IS_SENSITIVE (menu_item) ||
> +      (!GTK_BIN (menu_item)->child &&
> +       G_OBJECT_TYPE (menu_item) == GTK_TYPE_MENU_ITEM) ||
> +      GTK_IS_SEPARATOR_MENU_ITEM (menu_item) ||
>        !GTK_IS_MENU (menu_item->parent))
>      return FALSE;

I think I'd export gtk_menu_shell_is_selectable_menu_item() 
as _gtk_menu_item_is_selectable() or something to avoid
the duplication of complex logic here ... this isn't exactly 
_readable_. :-)

>  	{
>  	  if ((menu_item->parent == widget) &&
>  	      (menu_item != menu_shell->active_menu_item))
> -	    gtk_menu_shell_select_item (menu_shell, menu_item);
> +	    {
> +	      if (menu_item &&
> +		  GTK_MENU_SHELL_GET_CLASS (menu_shell)->submenu_placement == GTK_TOP_BOTTOM)

The if (menu_item) part is probably not necessary since it would have
segfaulted aove if menu_item == NULL.

Actually, despite what I said earlier, I think the hacky
submenu_placement part probably isn't necessary either, since we do
want this behavior for _any_ time a submenu is popped up by clicking
with the mouse, which is what this code is about.

> +		gtk_object_set_data (GTK_OBJECT (menu_shell),
> +				     "gtk-menushell-just-activated",
> +				     0x1);
> +	      gtk_menu_shell_select_item (menu_shell, menu_item);
> +	    }
>  	}
>      }

This should be GUINT_TO_POINTER (1), not 0x1, if you want to avoid
compiler warnings.

> @@ -460,8 +470,24 @@
>  
>        if ((event->time - menu_shell->activate_time) > MENU_SHELL_TIMEOUT)
>  	{
> +	  if (menu_item &&
> +	      GTK_MENU_SHELL_GET_CLASS (menu_shell)->submenu_placement == GTK_TOP_BOTTOM)
> +	    {
> +	      if (gtk_object_get_data (GTK_OBJECT (menu_shell), "gtk-menushell-just-activated") == 0x1)
> +		{
> +		  gtk_object_remove_data (GTK_OBJECT (menu_shell), "gtk-menushell-just-activated");
> +		}
> +	      else
> +		{
> +		  gtk_menu_shell_deactivate (menu_shell);
> +		  /* we didn't do a selection -> don't emit a signal */
> +		  return TRUE;
> +		}
> +	    }

I don't think this code portion will always be hit; for instance, if
the mouse is released faster than MENU_SHELL_TIMEOUT. If it doesn't
get hit, then we get stuck user data, so the menu will act strangely
in the future. It's probably better to, at the top of 
gtk_menu_shell_button_release, do:

 gboolean just_activated = FALSE:
 if (g_object_get_data (G_OBJECT (menu_shell), "gtk-menushell-just-activated"))
   {
     just_activated = TRUE;
     g_object_set_data (G_OBJECT (menu_shell), "gtk-menushell-just-activated", NULL);
   }

> @@ -876,7 +907,8 @@
>  	  while (node != start_node && 
>  		 (!node ||
>  		  !GTK_WIDGET_IS_SENSITIVE (node->data) ||
> -		  !GTK_WIDGET_VISIBLE (node->data) ))
> +		  !GTK_WIDGET_VISIBLE (node->data) ||
> +		  !gtk_menu_shell_is_selectable_menu_item (node->data)))

Probably should move the IS_SENSITIVE and VISIBLE checks inside the
is_selectable_menu_item() function.

Regards,
                                        Owen