Re: Preliminary submenu patch

[Nils Barth <nils_barth post harvard edu>]

Thus spake Owen Taylor:
> 
> 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.

Okay, I thought this might happen but forgot...
This patch relies on my previous patch that makes submenu indicator
arrows point in the right direction (I think), which I've attached.
Basically that patch determines the size of the submenu before drawing
the menu item, so the submenu size can be used in navigation.

Note: I haven't synced with CVS lately b/c of the Pango and GObject
committing -- is it back to being stable? (I suppose...)
I'll fix that.

When I get home, I'll update from CVS and get the patch working again;
hopefully it was just the menuitem bit I'm now attaching, else I have
more work.

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

Thanks!

> > Questions:
> > (1) What's the proper way of dereferencing a GdkRegion?
> 
>  gdk_region_destroy()

okay, done. I suppose the correct way to dereference/destroy most
everything in GTK is with _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.

Okay -- the implementation should be similar, only with a popdown
call instead of a popup.

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

Okay, got it.

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

Sorry, my remarks were really unclear.

Here's how I implemented the navigation (er, that's what I call the
special handling of navigating to submenus -- I should be more
verbose/correct in naming functions):

When the pointer leaves a submenu, I set a GdkRegion (which I call
navigation region, Kim calls keep-up region, etc.).
Then menu motion events and menu_shell enter/leave events check to see
if the pointer is in the region (if it exists), in which case they
ignore the event; otherwise, they destroy the region and send the
event as normal (which pops down the menu).

Two problems:
(1) This is the wrong way to do it.
    Better would prolly be to make it so that when a menu_item with a
    submenu receives a leave event, it sets up such a region and grabs
    the pointer, preventing it from sending events.
    (and then the menu_item motion_notify checks when the region
    has been left, etc.)
    I couldn't figure out the correct way to do this, as the gdk grab
    pointer doesn't seem to do this, and
    gtk_widget_grab_[focus,default] looks good, but the gtk_widget API
    isn't documented (er, I volunteer to try documenting this, though my
    knowledge of GTK is limited...)

(2) Handling enter/leave/motion events for menus is complicated in
    that gtkmenu handles motion events and gtkmenushell handles
    enter/leave events.

So I was suggested fixing (2) (and moving all changes to gtkmenu, not
gtkmenushell) by making gtk_menu_[enter,leave]_notify () which would
do the navigation stuff and then call gtk_menu_shell_&_notify ()

However, it seems better to use a grab somehow.
* Could you (or someone) suggest a reference on how to grab the
  pointer/how this affects signal sending/handling?

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

oops -- sorry.

> > +/* 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.

Right -- I left in lots of debugging cruft that'll have to go.

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

Okay -- GdkEventMotion->x_root/y_root are doubles, while
gdk_region_point_in takes ints. What's the proper way of dealing with
this?

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

Sorry. When the patch is finalized/fixed, these will be removed
(what I've done in the past to fix such lint is:
make a patch, release/delete the modified files, then clean up the
patch and apply it (er, locally -- submitting it to ftp.gtk.org)
-- is this reasonable?)

In future, I'll try to make my patches less crufty before posting
them.

> > +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?

Er, it's applicable anytime you have a vertical menu popup from a LEFT_RIGHT
menuitem in any menushell. Since currently this only happens in
submenus of existing vertical menus, it could be moved into GtkMenu.

The code's in gtkmenushell b/c of gtkmenushell dealing with
enter/leave events.

The GdkRegion pointer is stored as a static global region partly by
analog with
last_submenu_deselect_time
from gtkmenuitem.c (and I'd really prefer that all this handling
happened in gtkmenuitem, I just don't understand grabs)
As I'm currently doing it, if I set a region up local to the GtkMenu,
and then whipped the pointer out of the menu, say into the menubar,
then I first had to traverse the menutree (following active_items) to
find the region, and then check that it's still in there.
Basically, the region is a transient variable of which only on
instance is ever running at once (for any given user/X display), and
which is destroyed before any other action is taken. You have to leave
the region/destroy the pointer before any menuitem/menu is
activated/deactived.

I originally put it in the GtkMenuShell structure, but decided to move
it to a static to clarify its function and simplify the code.

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

oops, sorry. vim default formatting. I'll use the emacs function Havoc
posted a while back.

> > +    /* Set navigation region */
> > +    point[0].x = (gint16) event->x_root;
> 
> GdkPoint is gint, not gint16 now.

okay (behind the times...)

Sorry about the broken-ness/incompleteness of the patch and the hasty
formatting -- I'll postpone future patch postings until I've tested
exactly the patch against current CVS.
I'm just busy/moving now, so I was hoping the patch would work and I
could get feedback so I could continue working pronto when I arrive/settle
in, but this isn't an excuse for stuff not working.

-- 
  -nils
Public key: http://www.fas.harvard.edu/~nbarth/pub-key.txt
Index: gtkmenuitem.c
===================================================================
RCS file: /cvs/gnome/gtk+/gtk/gtkmenuitem.c,v
retrieving revision 1.39
diff -u -r1.39 gtkmenuitem.c
--- gtkmenuitem.c	2000/05/12 15:25:45	1.39
+++ gtkmenuitem.c	2000/06/08 20:47:34
@@ -35,6 +35,14 @@
 
 #define BORDER_SPACING  3
 #define SELECT_TIMEOUT  75
+/* SUBMENU_OFFSET specifies by what percent of a menuitem height
+ * a submenu is vertically offset. Should vary between 0 and 1.0.
+ * (I guess -1.0 to 0 range is okay).
+ * GTK currently defaults to 0.25 -- other windowing systems default
+ * to 0.
+ */
+#define GTK_SUBMENU_OFFSET 0.25
+#define GTK_DIRECTION_DEFAULT  GTK_DIRECTION_RIGHT;
 
 #define MENU_ITEM_CLASS(w)  GTK_MENU_ITEM_CLASS (GTK_OBJECT (w)->klass)
 
@@ -171,8 +179,8 @@
   menu_item->accelerator_width = 0;
   menu_item->show_toggle_indicator = FALSE;
   menu_item->show_submenu_indicator = FALSE;
-  menu_item->submenu_direction = GTK_DIRECTION_RIGHT;
-  menu_item->submenu_placement = GTK_TOP_BOTTOM;
+  menu_item->submenu_direction = GTK_DIRECTION_DEFAULT;
+  menu_item->submenu_placement = GTK_LEFT_RIGHT;
   menu_item->right_justify = FALSE;
 
   menu_item->timer = 0;
@@ -423,6 +431,7 @@
   GtkMenuItem *menu_item;
   GtkStateType state_type;
   GtkShadowType shadow_type;
+  GtkArrowType arrow_direction;
   gint width, height;
   gint x, y;
 
@@ -456,14 +465,38 @@
 
       if (menu_item->submenu && menu_item->show_submenu_indicator)
 	{
+	  gint dummy_a=0;
+	  gint dummy_b=0;
+	  GtkRequisition requisition;
+
 	  shadow_type = GTK_SHADOW_OUT;
 	  if (state_type == GTK_STATE_PRELIGHT)
 	    shadow_type = GTK_SHADOW_IN;
 
+	  /* Similarly to in gtk_menu_position(), we need to figure out
+	   * where the menu will pop up to figure out the arrow direction
+	   */
+	  gtk_widget_size_request (GTK_WIDGET (menu_item->submenu),
+	                          &requisition);
+	  /* The temporary requisition var is so that gtk_widget_size_request
+	   * doesn't whine; otherwise we'd just have
+	   * &(GTK_WIDGET (menu_item->submenu)->requisition) above.
+	   */
+	  GTK_WIDGET (menu_item->submenu)->requisition.width  = requisition.width;
+	  GTK_WIDGET (menu_item->submenu)->requisition.height = requisition.height;
+	  gtk_menu_item_position_menu (GTK_MENU (menu_item->submenu),
+	                               &dummy_a, &dummy_b, menu_item);
+	  /* FIXME: This should be a general GTK_DIRECTION->GTK_ARROW
+	   * macro.
+	   */
+	  arrow_direction = (menu_item->submenu_direction ==
+	      GTK_DIRECTION_RIGHT)
+	    ? GTK_ARROW_RIGHT : GTK_ARROW_LEFT;
+	  
 	  gtk_paint_arrow (widget->style, widget->window,
 			   state_type, shadow_type, 
 			   area, widget, "menuitem", 
-			   GTK_ARROW_RIGHT, TRUE,
+			   arrow_direction, TRUE,
 			   x + width - 15, y + height / 2 - 5, 10, 10);
 	}
       else if (!GTK_BIN (menu_item)->child)
@@ -708,10 +741,10 @@
       break;
 
     case GTK_LEFT_RIGHT:
-      menu_item->submenu_direction = GTK_DIRECTION_RIGHT;
       parent_menu_item = GTK_MENU (GTK_WIDGET (menu_item)->parent)->parent_menu_item;
       if (parent_menu_item)
 	menu_item->submenu_direction = GTK_MENU_ITEM (parent_menu_item)->submenu_direction;
+      else menu_item->submenu_direction = GTK_DIRECTION_DEFAULT;
 
       switch (menu_item->submenu_direction)
 	{
@@ -736,7 +769,8 @@
 	  break;
 	}
 
-      ty += GTK_WIDGET (menu_item)->allocation.height / 4;
+      if (GTK_SUBMENU_OFFSET)
+	ty += (GTK_WIDGET (menu_item)->allocation.height * GTK_SUBMENU_OFFSET);
 
       break;
     }