Re: Preliminary submenu patch

[Owen Taylor <otaylor redhat com>]

Nils Barth <nils_barth@post.harvard.edu> writes:

> Attached find a preliminary patch to improve submenu navigation, a la
> Mac.
> 
> It deals properly with mouse movement and with keystrokes:
> when the pointer leaves a menu item, it creates a GdkRegion (the
> triangular region that I've been going on about), and then every
> menu_motion event and menu_shell enter/leave event checks the pointer
> to see that it's in the region. If so, the event is ignored. If not
> (or if the pointer is NULL) it kills the region and the event is
> procesed as usual.

I hate to say it, but this patch doesn't work at all for me. 
I see no effect at all. (Using current CVS GTK+). I'm not sure
what is going on - I haven't tried to debug.

But I've made some stylistic comments on the patch as well
and tried to answer your questions.

Regards,
                                        Owen

> Any keyboard action kills the region.
> The region is stored as a static pointer in gtkmenushell.c
> 
> Note that testgtk was very helpful in bug hunting and in code design:
> it showed bugs in early attempts I had never thought of.
> 
> Questions:
> (1) What's the proper way of dereferencing a GdkRegion?

 gdk_region_destroy()

> (2) I haven't implemented a timeout -- I was thinking of copying the
>     code from gtk_real_menu_item_select -- is this right? (David?)

 that's a timeout, yes.

> (3) Currently, it's ugly in that to share a function between
>     gtkmenushell and gtkmenu, I make public functions available.

There always have been "private" functions in GTK+ header files.
They should be marked as such with a comment. With GTK+-1.4, we
have the added convention of beginning such functions with a 
underscore:

 _gtk_menu_shell_motion_notify()

One reason for this doing is that we can eventually control exports
using a regexp on function name.

>     Would an acceptable better way be to add a line like:
> 
> if (GTK_WIDGET_CLASS (parent_class)->motion_notify_event (widget, event))
>   return TRUE;
> 
>     to gtkmenu.c:gtk_menu_motion_notify_event ()
>     and then make gtk_menu_shell_motion_notify_event () which just
>     called
> if (gtk_menu_shell_navigating ((gint) event->x_root, (gint)
>     event->y_root))
> return TRUE;
>      ? (I didn't do this b/c it involved adding new signal handlers, and I wanted
>      to make sure this was kosher).

New signal handlers? I don't quite follow. But, no, I don't like this
approach, because I don't see this as a natural behavior for a motion
event for a generic MenuShell descendent.

> Index: gtkmenu.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkmenu.c,v
> retrieving revision 1.45
> diff -u -r1.45 gtkmenu.c
> --- gtkmenu.c	2000/05/21 06:13:34	1.45
> +++ gtkmenu.c	2000/06/07 13:05:32
> @@ -30,11 +30,11 @@
>  #include "gtklabel.h"
>  #include "gtkmain.h"
>  #include "gtkmenu.h"
> +#include "gtkmenushell.h"

This is not needed, gtkmenu.h includes gtkmenushell.h.

>  #include "gtkmenuitem.h"
>  #include "gtksignal.h"
>  #include "gtkwindow.h"
>  
> -
>  #define MENU_ITEM_CLASS(w)   GTK_MENU_ITEM_GET_CLASS (w)
>  #define	MENU_NEEDS_RESIZE(m) GTK_MENU_SHELL (m)->menu_flag
>  
> @@ -1103,14 +1103,24 @@
>  }
>  
>  static gint 
> -gtk_menu_motion_notify  (GtkWidget	   *widget,
> -			 GdkEventMotion    *event)
> +gtk_menu_motion_notify (GtkWidget	  *widget,
> +			GdkEventMotion    *event)
>  {
> +  GtkMenu *menu;
> +  GtkMenuShell *menu_shell;
> +  
>    g_return_val_if_fail (widget != NULL, FALSE);
>    g_return_val_if_fail (GTK_IS_MENU (widget), FALSE);
>    
> -  if (GTK_MENU_SHELL (widget)->ignore_enter)
> -    GTK_MENU_SHELL (widget)->ignore_enter = FALSE;
> +  menu = GTK_MENU (widget);
> +
> +/* start */
> +  if (gtk_menu_shell_navigating ((gint) event->x_root, (gint) event->y_root))
> +    return TRUE;
> +  menu_shell = GTK_MENU_SHELL (widget);
> +/* end */

Obviously these comments would have to be removed before the final
result.  Stylistically I don't particularly like the explicit casts in
here. Explicit casts prevent the compiler from using its intelligence
to decide if the cast is reasonably or not.

> +  if (menu_shell->ignore_enter)
> +    menu_shell->ignore_enter = FALSE;
>    else 
>      {
>        gint width, height;
> @@ -1125,11 +1135,10 @@
>  	  send_event.crossing.window = event->window;
>  	  send_event.crossing.time = event->time;
>  	  send_event.crossing.send_event = TRUE;
> -	  
> +
>  	  gtk_widget_event (widget, &send_event);
>  	}
>      }
> -

It's easier if you don't make random line insertions/deletions in a line
like this.

[...]

> +static GdkRegion *navigation_region = NULL;

I don't like the general approach of having a global region like this.
It seems to me that the region should be stored in the GtkMenu
structure. The "triangular region" only applies to navigation within
a single menu, right? Actually, I'm not sure why the handling of the
navigation is in GtkMenuShell at all - not GtkMenu. The behavior
is specialized to vertical menus, right?

There are quite a few stylistic problems in the following.

The basic GTK+ style is the GNU style, with a few additions. Indentation
follow the GNU style. We don't yet have a coding-style document,
but:

 pango/docs/TEXT/coding-style.txt

is reasonably close to the GTK+ style.

> +void
> +gtk_menu_shell_stop_navigating ()
> +{
> +  /* FIXME: Should I deallocate the region? */
> +  navigation_region = NULL;
> +}
> +
> +gboolean
> +gtk_menu_shell_navigating (gint event_x, gint event_y)
> +{
> +  if (navigation_region)
> +  {
> +    if (gdk_region_point_in (navigation_region, event_x, event_y))
> +      return TRUE;
> +    else
> +    {
> +      gtk_menu_shell_stop_navigating ();
> +      return FALSE;
> +    }
> +  }
> +  return FALSE;
> +}
> +
> +static void
> +gtk_menu_shell_set_navigation_region (GtkMenuItem      *menu_item,
> +				      GdkEventCrossing *event)
> +{
> +  gint submenu_left = 0, submenu_right  = 0;
> +  gint submenu_top  = 0, submenu_bottom = 0;
> +  gint width        = 0, height         = 0;
> +  GdkPoint point[3];
> +  GtkWidget *event_widget;
> +
> +  g_return_if_fail (menu_item->submenu != NULL);
> +  g_return_if_fail (event != NULL);
> +  
> +  event_widget = gtk_get_event_widget ((GdkEvent*) event);
> +  
> +  gdk_window_get_root_origin (menu_item->submenu->window,
> +      &submenu_left, &submenu_top);
> +  gdk_window_get_size (menu_item->submenu->window, &width, &height);
> +  submenu_right = submenu_left + width;
> +  submenu_bottom = submenu_top + height;
> +  gdk_window_get_size (event_widget->window, &width, &height);
> +  /* FIXME: 6 is a magic number -- why does it work? */
> +  if ((event->x >= 0) && (event->x <= width - 6)
> +      && !((event->y < 0) && (event->y_root <= submenu_top))
> +      && !((event->y >= 0) && (event->y_root >= submenu_bottom)))
> +  {
> +    /* Set navigation region */
> +    point[0].x = (gint16) event->x_root;

GdkPoint is gint, not gint16 now.

[...]

>  }
>  
>  static void
> Index: gtkmenushell.h
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkmenushell.h,v
> retrieving revision 1.8
> diff -u -r1.8 gtkmenushell.h
> --- gtkmenushell.h	2000/02/13 08:16:47	1.8
> +++ gtkmenushell.h	2000/06/07 13:05:33
> @@ -55,7 +55,8 @@
>    GList *children;
>    GtkWidget *active_menu_item;
>    GtkWidget *parent_menu_shell;
> -  
> +
> +/*  GdkRegion *navigation_region; */
>    guint active : 1;
>    guint have_grab : 1;
>    guint have_xgrab : 1;
> @@ -99,8 +100,9 @@
>  void    gtk_menu_shell_activate_item  (GtkMenuShell      *menu_shell,
>  				       GtkWidget         *menu_item,
>  				       gboolean           force_deactivate);
> -
> -
> +void    gtk_menu_shell_stop_navigating ();
> +gboolean gtk_menu_shell_navigating        (gint              event_x,
> +					   gint              event_y);
>  #ifdef __cplusplus
>  }
>  #endif /* __cplusplus */