Re: [patch] Scrolling menus

[otaylor redhat com]

Alexander Larsson <alla lysator liu se> writes:

Let me first note some problems with the behavior from testing:

 - I think it looks a little funny to have the menu items just cut
   off by the scroll arrows. 
  
   Perhaps there should be some separator or relief between the two-
   e.g., make the scroll arrows raised buttons.

 - If you pop up the menu with a single click, then you have to
   keep on clicking on the button for each scroll incroment.
   If you get into a rut of doing this, then when the scroll
   arrow disappears, you will select the first menu item.

   There should be an autorepeat here.

   For torn off menus, when this happens, you untear the menu.
   I tend to think that the tear-off indicator probably shouldn't
   scroll, though implementing that is not easy. 

   (Note that in IE, for the Favorites menu, only the favorites
   scroll, and not the "Edit", etc, options, so the general facilitiy
   to define an unscrolling top section could be useful.)

 - Also, when you are over an arrow, popped up submenus stay up
   permanently.

 - The fact that when you tear off a menu, it tears off at the
   size that it was when you tore it off and it stays that way
   is a little odd. I almost think that if you have a scrolling
   menu and you tear it off, you should get a scrollbar.

 - Something weird happens with option menus - when an option
   menu is popped up with scroll arrows, the current item is
   not selected on popup as it is in other cases, so you have
   to move off it and then move back.

   Also, with option menus, in my opinion, if you pop it up
   with only one line showing, then as you scroll, more lines
   should become visible. That is, the menu should move onscreen
   as you scroll, not just stay small and scroll.

   Finally, with option menus, the limiting case where the menu is
   right on the edge of the screen works really badly - you can get
   scroll arrows and nothing else. (In fact, I was able to get a
   assertion failure in gtk_menu_handle_scrolling in this case) I'm
   not really sure what to do here - the suggestion in the previous
   paragraph will help some, since if you scroll a bit, more will be
   visible. If that is not sufficient, then perhaps we need to force
   one line of the menu to be always visible, even if that breaks
   it so that the current menu item isn't immediately selected.

Those are the main problems I noticed fooling around with it.

> Ok, here is an update on the scrolling menu patch. I've fixed tons of
> boundary conditions, so the behaviour is a lot more consistent now.
> 
> There is still one issue I know off. gtk_menu_shell_insert() (called by
> _add, _append & _prepend) doesn't call gtk_widget_set_parent_window(child,
> menu->item_window) for GtkMenus. Is there any way to do this nicely?

I think you'll have to virtualize gtk_menu_shell_insert().

A few random notes about the patch, though I haven't gone through
it in detail:

> Index: gtkitemfactory.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkitemfactory.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 gtkitemfactory.c
> --- gtkitemfactory.c	2000/07/26 11:32:44	1.31
> +++ gtkitemfactory.c	2000/10/13 15:14:44
> @@ -1386,6 +1386,7 @@ static void
>  gtk_item_factory_menu_pos (GtkMenu  *menu,
>  			   gint     *x,
>  			   gint     *y,
> +			   gint     *scroll_offset,
>  			   gpointer  func_data)

Hmmm, source incompatible change. if we do want to make the change,
then we need a documentation pach, and also a addition to Changes-2.0.txt.

Though, perhaps the right thing to do is for the menu_pos func to
position the menu offscreen, and let the code that calls it worry
about translating that into a scroll offset. Would that work?

> @@ -188,6 +206,8 @@ gtk_menu_window_event (GtkWidget *window
>      {
>      case GDK_KEY_PRESS:
>      case GDK_KEY_RELEASE:
> +    case GDK_BUTTON_PRESS:
> +    case GDK_BUTTON_RELEASE:
>        gtk_widget_event (menu, event);
>        handled = TRUE;
>        break;

Looking at this, itis probably better to fix the above to:

 handled = gtk_widget_event (menu, event);
 break;

> @@ -225,9 +245,22 @@ gtk_menu_init (GtkMenu *menu)

>    GTK_WIDGET_SET_FLAGS (menu, GTK_FLOATING);
>    menu->needs_destruction_ref_count = TRUE;
>  
> +  menu->view_window = NULL;
> +  menu->item_window = NULL;

I'd probably call this ->bin_window just for consistency with
viewport.

> +  menu->first_item = 0;
> +  menu->scrolling_direction  = 0;
> +  menu->timeout_id = 0;
> +  
>    menu->tearoff_window = NULL;
>    menu->torn_off = FALSE;
>  
> +  menu->upper_arrow_visible = FALSE;
> +  menu->lower_arrow_visible = FALSE;
> +  menu->upper_arrow_prelight = FALSE;
> +  menu->lower_arrow_prelight = FALSE;
> +  menu->handling_arrow_click = FALSE;
> +  
>    MENU_NEEDS_RESIZE (menu) = TRUE;
>  }
>  
> @@ -241,6 +274,9 @@ gtk_menu_destroy (GtkObject *object)
>  
>    menu = GTK_MENU (object);
>    
> +  if (menu->timeout_id)
> +    g_source_remove (menu->timeout_id);

->destroy is allowed to be called multiple times now, so anything
like this should raise a red flag. You almost certainly want:
 
   if (menu->timeout_id)
     {
       g_source_remove (menu->timeout_id);
       menu->timeout_id = 0;
     }

> +  if (GTK_WIDGET_CLASS (parent_class)->unrealize)
> +    (* GTK_WIDGET_CLASS (parent_class)->unrealize) (widget);

It's a little bit of a matter of taste, but my opinion is
for something like unrealize() where you know GtkWidget has
an implementation, and handlers are required to chain, you
don't need to check to see if the parent is NULL or not.

Regards,
                                        Owen