Re: Submenu navigation status?

[Owen Taylor <otaylor redhat com>]

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

> Final submenu hysteresis patch! (hopefully)
> Attached find a slightly polished version of the already buffed patch.
> This gives GTK proper submenu navigation, and solves one TODO list
> item.
> If no-one has any objections to this, could a maintainer apply the
> patch? Or I could upload to the (backlogged?) FTP?

[...]

> So gtk_grab_add () was not the right way of going at this (sorry
> for the blind alley).

The new approach looks good. It isn't the only way to do it,
but its quite compact and clear.
 
> > Are there any other issues to address?
> 
> There was some question as to whether the nav_region should be a
> static (to gtkmenu.c) or part of the GtkMenu struct.
> The nav_region is currently a static, basically to be shared between
> a few functions that create/check/destroy it. It's basically a temp
> variable, which begs for static. It also has no place in GtkMenu, nor
> need to be there, since only one nav_region exists at once. If we had
> multiple pointers accessing multiple menus in a given GTK app at the
> same time...we'd want to make nav_region part of GtkMenu, but short of
> that (radical) development, I see no need nor desire to change it from
> being a static.

Well, mostly on the side of code cleanliness, I definitely see
a reason - the navigation region is basically state for a
particular menu. Motion events in other menus should not
be interpreted with respect to the navigation region.

To 'prove' that the current approach leads to problems, I've
identified at least 2 bugs prominent with it:

 - If another menu is popped up before the navigation region
   is removed in the timeout, then the navigation region
   will be leaked. (OK, this has nothing much to do with
   the location of the navigatio region...)

 - if the menu that currently has the navigation region is 
   destroyed before the navigation region is removed, your
   code will segfault. The easiest way to solve this is 
   to put the timeout ID in the GtkMenu structure, and once
   you do that, you might as well put the reion in there
   as well.
 
Anyways, to save you the task of rewriting it in such a 
repugnant way ;-), I've done so myself. My revised version
of the patch follows.

Doing this turned up some oddities in the event handling
for menus that made it a little less straightforward than
one might expect. But the comments I've added should help
people understand what is going on in the future.

> There was some discussion (between David and Nils Philippsen) about
> making the hysteresis timeout be user-definable, which could be
> done in an ad-hoc way (patches welcome, I'm sure), but...I agree with
> David that it's better to work on a general gtk config system, rather
> than causing API bloat with 200 functions of the form:
> gtk_..._[get,set]_minor_feature

As you say, this should be done (if at all) as part of a
general customization framework.

I'm not completley sure that the current timeout approach
is perfect - if I move slowly on the diagonal, sometimes
the timeout will trigger when it shouldn't. It might
be good to use a slightly shorter timeout and then 
reset it on motion events in a way such that it is dependent
on the speed of (vertical?) motion.

> If anyone sees anything wrong with the code, please say so/fix it,
> else can it be commited?

I've noted a few comments about other fixes I've made below.
If you're OK with my new version, I'll commit it.

====
+  if (gtk_menu_navigating_submenu (event->x_root, event->y_root))
+	return TRUE; 
+
====

This was about the only formatting problem I found.

====
+  send_event.crossing.window = gdk_window_at_pointer (NULL, NULL);
+  send_event.crossing.type = GDK_ENTER_NOTIFY;
+  send_event.crossing.time = GDK_CURRENT_TIME;
+  send_event.crossing.send_event = TRUE;
+
+  gtk_widget_event (GTK_WIDGET (user_data), &send_event);
====

I don't think this was working at all - the widget that 
needs to receive the enter event is the menu that is the
parent of the menu item (user_data), and gtk_widget_event()
doesn't do event propagation up the widget heirarchy.

I didn't spend a lot of time tracing this down since I did
things a bit differently, but I think that this just appeared
to work because gtk_menu_motion_notify() was (because of
an old bug) continually sending enter events.

====
+  if ((event->x >= 0) && (event->x <= width)
+      && (((event->y < 0) && (event->y_root >= submenu_top))
+	  || ((event->y >= 0) && (event->y_root <= submenu_bottom))))
+    {
+      /* Set navigation region */
+      /* We fudge/give a little padding in case the user
+       * ``misses the vertex'' of the triangle/is off by a pixel or two.
+       */ 
+      if (menu_item->submenu_direction == GTK_DIRECTION_RIGHT)
+	point[0].x = event->x_root - SUBMENU_NAV_REGION_PADDING; 
+      else                             
+	point[0].x = event->x_root + SUBMENU_NAV_REGION_PADDING;  
+      
+      /* Exiting the top or bottom? */ 
+      if (event->y < 0)
+        { /* top */
+	  point[0].y = event->y_root + SUBMENU_NAV_REGION_PADDING;
+	  point[1].y = submenu_top;
+	}
===

I found the checks in the if statement here rather confusing,
so I moved the checks for empty regions down into the code
where we check if we are exiting the top or bottom.

Anyways, thanks a lot for working on this. I seem to remember that you
had another patch for switching the side of the arrow for
submenus. When I looked at it, you could get that to do strange things
with torn off menus and expose events, but it mostly looked pretty
reasonable. Do you have that or an updated version around somewhere?
(I probably can dig it up out of my mail if necessary.)

Regards,
                                        Owen

patch