Re: Preliminary submenu patch

[Owen Taylor <otaylor redhat com>]

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

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

It turned out that my problem wasn't this but a stupid problem with
not making clean after the libraries changed name so I was running
old versions without your patch. Once I fixed that, your patch
worked as advertised.

Trying things out, I found that the behavior of your patch was 
pretty good, except that occasionally you could get the mouse
pointer within the triangle without intending to be in that state.

This is confusing because the menu just sticks there forever.

Adding a timeout I think would ameliorate the problem. A former
Apple UI designer I talked to indicated that the Macintosh behavior
is basically triangle + timeout, and thought that an appropriate
timeout would be 1/3 or 1/2 second. 
 
> 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.

I think things have basically returned to stability now. 
 
> > > 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?

Refcounted objects other than widgets should be dereferenced with
unref().

Widgets are a special case - they have both unref() and destroy(),
with destroy() meaning, essentially, "break all references".

For non-refcounted objects, GTK+ and GLib either call the free function
destroy() or free(). It's pretty equally split between the two - I
tend to use free() for new objects as to avoid confusion with
the meaning of gtk_widget_destroy().
 
> > >     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

Probably it was a question of the setting of the owner_events argument.
The documentation for XGrabPointer() should be useful in explaining
this. (If owner_events=TRUE, then events are delivered normally to
any window of the application.)

However, you don't want to call gdk_pointer_grab() here, because GTK+
relies on having a grab during menu navigation and gdk_pointer_grab()
would interfere with that.

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

grab_focus() / grab_default() set the keyboard focus or default button
to a particular widget and are unrelated.

The more related function is gtk_grab_add(). This adds a GTK+ grab
which is a different concept than a GDK grab / X Grab. An X grab
change the window stored in an event, while a GTK+ grab simply changes
the widget the event is delivered to. 

It probably would work to use a gtk_grab_add(), handle 
enter/leave/motion_notify there, and when you get an event outside
of the the keep up region, replay it via gtk_main_do_event(). 

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

Hmm, OK, that probably makes sense if the grab stuff doesn't work.
 
> 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?

Hopefully the above will help a bit. The code for gtk_grab_add() is
pretty simple, so that may be another good source of info.

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

Well, that is fine - C automatically converts based on the function
prototype. GTK+ is simply not going to be used with a pre-ANSI compiler.

If you are really concerned about rounding - I wouldn't be - the
double stuff is mostly there for drawing programs that want subpixel
positioning - then I'd use math.h round().

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

I think it is pretty safe to say we won't be adding another type
of vertical menu any time soon.
 
> The code's in gtkmenushell b/c of gtkmenushell dealing with
> enter/leave events.

OK. Adding GtkMenu enter/leave handlers would make sense to me.
 
> 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 certainly believe that you would never need multiple keep-up regions
at once. It just seemed a little cleaner to me not to have it as a
global if it referred to a particular GtkMenu. But really, I'm pretty
indifferent on this issue - whichever makes the code simpler and
cleaner.

[...]

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

As mentioned above - that was my fault not yours. Sorry about the slow
response here.

Regards,
                                        Owen