Re: [PATCH] GtkMenu scrolling function added

[Owen Taylor <otaylor redhat com>]

Thanks a lot for working on this; it's something that has been
needed for a long time. 

> Features
> ========
> - enhance GtkMenu widget with scrolling features
> - Automatically scrolling when the UP/DOWN arrow were clicked.

> - The position of pop-up menu will be left-aligned, if nicessary.

I don't quite understand what you mean by this - I think you
might be referring to the fact that you displace the menu
to the side of the menu item that pops it up if it is so tall
that it will extend past this item.

I'm not sure this is the best behavior - what windows does is always
make it so that the menu starts from the menu bar and goes either
up or down. When the menu hits the edge of the screen, a scrolling
arrow is added. But the menu is always anchored at the menu bar.

Some procedural comments:

 - The indentation needs to match the GTK+ standard. See the
   discussion on this list a few days ago for a summary
   of what that is.

   (A good starting point might be that all the lines in
   a function should have the same indentation depth :-)
   But we are a lot pickier than just that.)

 - No C++ comments, please

 - Patches are preferred in 'diff -u' format, instead of 
   the 'diff -c' format you have.

 - You most likely want to be working on this patch against 
   GTK+-1.3.x instead of GTK+-1.2.8. 

   While we probably will eventually put out a 1.2.9, as
   a final release in the 1.2.x series, all new development
   is supposed to be going into the 1.3.x series, and 
   only bugfixes into 1.2.x. 

   Now, this could conceivably be seen as a bugfix, but even 
   so, the requirements for putting it into 1.2.x would be
   strong.

    - Binary and source compatibility
    - Very low possibility introducing new bugs
  
   Your patch does not maintain binary compatibility, since
   it adds new fields to the GtkMenu structure. 

   In general it will just be easier for everbody if you do it against
   1.3.x The 1.2.x series is a deadend branch.

Some comments from trying out the patch from a user's point
of view:

 - It's rather strange how the menu changes length and the 
   items move when the top arrow is added.

 - Tearoff menus are not handled correctly.

 - If I pop up the menu with a single click, then later 
   click on one of the arrows, then if I drag out of
   the arrow back, it should resume scrolling.

Some implementation issues:

 - Reallocating the children in order to do the scrolling is a rather
   inefficient and complicated way of doing things. What I would do is
   to either put the items inside a viewport, or (perhaps easier)
   implement the same setup yourself. 

   (the way viewport works is that the thing being scrolled is
   inside one large window, which is clipped by a second
   window the size of the viewable window.)

>   static gint
> + gtk_menu_key_release (GtkWidget *widget,
> + 			GdkEventKey *event)
> + {
> + 	GtkMenuShell *menu_shell;
> + 
> + 	g_return_val_if_fail (widget != NULL, FALSE);
> + 	g_return_val_if_fail (GTK_IS_MENU (widget), FALSE);
> + 	g_return_val_if_fail (event != NULL, FALSE);
> +       
> + 	menu_shell = GTK_MENU_SHELL (widget);
> + 	menu_shell->ignore_enter = FALSE ;
> + 
> + 	return TRUE;

What's this about? Connecting to key_release is generally a bad
idea, since, unlike button_press/release, there is no guarantee
that they will be paired.

> + 	if (GTK_MENU (widget)->top_scroll_arrow_lit != old_top_lit
> + 		|| GTK_MENU (widget)->bottom_scroll_arrow_lit != old_bottom_lit)
> + 		gtk_menu_paint (widget) ;

In general, you should not draw directly in response to events;
rather you should invalidate the area to be repainted when GKT+
becomes idle. (gtk_widget_queue_draw() in GTK+-1.2, 
gdk_window_invalidate_* is probably better to use in GTK+-1.3 though
the first still works.)

> ***************
> *** 1234,1239 ****
> --- 1507,1884 ----
>     gtk_container_foreach (GTK_CONTAINER (widget), (GtkCallback) gtk_widget_show_all, NULL);
>   }
>   
> + /*  Set the scrolling state of the menu.  It is harmless to call this
> +  *  if there is no change in scrolling state.  (That is, it won't interrupt
> +  *  or reset the scrolling timers if the scrolling settings are the same).
> +  */
> + void
> + gtk_menu_set_scrolling (GtkMenu    *menu,
> +            gboolean     scroll_up,
> +            gboolean     scroll_down)
> + {
> +   if (!scroll_up && menu->scroll_up_source_tag)
> +     {
> +       g_source_remove (menu->scroll_up_source_tag);
> +       menu->scroll_up_source_tag = 0;
> +     }
> + 
> +   if (!scroll_down && menu->scroll_down_source_tag)
> +     {
> +       g_source_remove (menu->scroll_down_source_tag);
> +       menu->scroll_down_source_tag = 0;
> +     }
> + 
> +   if(scroll_up && !menu->scroll_up_source_tag)
> +     {                                       
> +       if (gtk_menu_scroll_up (menu, 1))
> +    menu->scroll_up_source_tag = g_timeout_add (MENU_SCROLL_TIMEOUT, gtk_menu_timeout_up, menu);
> +     }
> + 
> +   if(scroll_down && !menu->scroll_down_source_tag)
> +     {
> +       if (gtk_menu_scroll_down (menu, 1))
> +    menu->scroll_down_source_tag = g_timeout_add (MENU_SCROLL_TIMEOUT, gtk_menu_timeout_down, menu);
> +     }

I don't see a reason for having two separate timeouts here. You already
have the scroll_up_arrow_lit and scroll_down_arrow_lit flags that
should tell you what you are doing. (I might name them scrolling_up
and scrolling_down)

> + 	on_line = screen_height - border * 2 ;
> + 	children_copy = g_list_copy (GTK_MENU_SHELL (menu)->children);
> + 	children_copy = g_list_reverse(children_copy);  
> + 	on_child = g_list_length (GTK_MENU_SHELL (menu)->children) -1;
> + 	child = children_copy ;
> + 	if (menu->last_scroll_child != -1)
> + 	{
> + 		on_child = menu->last_scroll_child ;
> + 		child = g_list_nth (child, 
> + 			g_list_length (children_copy)-1 - on_child) ;
> + 	}
> + 	while (child && on_line >= y)
> + 	{
> + 		GtkWidget *widget;
> + 
> + 		widget = GTK_WIDGET (child->data);
> + 		if ((on_child <= menu->last_scroll_child
> + 			|| menu->last_scroll_child == -1)
> + 			&& GTK_WIDGET_VISIBLE (widget))
> + 		{
> +   			gtk_widget_size_request (widget, &requisition);
> + 
> + 			if (on_line + requisition.height +
> + 					MENU_SCROLL_ARROW_HEIGHT < y
> + 				&& child->next)
> + 				break ;
> + 
> + 			on_line += requisition.height ;
> + 		}
> + 
> + 		on_child--;
> + 		child = child->next;
> +     }
> + 
> + 	g_list_free(children_copy);

You really want to avoid continually converting between indices and
list positions - it's slow and it makes for fairly clumsy code.
I think the followign should be equivalent to the above. (Not
extensively checked.)

=================
  on_line = screen_height - border * 2;

  if (menu->last_scroll_child != -1)
    child = g_list_nth (GTK_MENU_SHELL (menu)->children,
			menu->last_scroll_child);
  else
    child = g_list_last (GTK_MENU_SHELL (menu)->children);

  while (child && on_line >= y)
    {
      GtkWidget *widget = child->data;

      gtk_widget_get_child_requisition (widget, &requisition);

      if (on_line + requisition.height +
	  MENU_SCROLL_ARROW_HEIGHT < y
	  && child->prev)
        break ;
      
      on_line += requisition.height ;
      
      child = child->prev;
    }
================ 


> + gboolean
> + gtk_menu_scroll_up (GtkMenu    *menu,
> +            guint        scroll_count)
> + {

[..lots of code ..]     

> + }
> + 
> + gboolean
> + gtk_menu_scroll_down (GtkMenu  *menu,
> +              guint      scroll_count)
> + {

[ ... lots of almost identical code ...]

> + }                        

You should try and avoid having this much code duplicated
in two places .. maybe the right thing to do is to
have:

 gtk_menu_scroll (GtkMenu *menu, gint scroll_count);

Where scroll_count < 0 indicates scrolling up.

> + void     gtk_menu_set_scrolling (GtkMenu *menu, 
> + 								gboolean scroll_up,
p> + 								gboolean scroll_down) ;

This should be named _gtk_menu_set_scrolling() and marked in
the header has an internal function. (If it needs to be exported
from gtkmenu.c at all ... see below.)

> ! 			if (menu_shell->active_menu_item && GTK_IS_MENU(menu_shell))
> ! 			{
> ! 				gint idx ;
> ! 				gint first_idx ;
> ! 				gint last_idx ;
> ! 
> ! 				first_idx = GTK_MENU(menu_shell)->first_scroll_child ;

Making the GtkMenuShell implementation need to know about GtkMenu
probably means that there is something wrong about the way things
are set up. For button_press/button_release, you can have a handler
in GtkMenu that checks to see if the event should be intercepted,
and if not, moves it to the parent menu.

gtk_menu_shell_move_selected is a little tricker, since it isn't 
virtualized currently. What I would suggest is that you add:

 void (*select_item) (GtkMenuShell *menu_shell, GtkWidget *menu_item);

To the GtkMenuShellClass structure, move the current implementation
of gtk_menu_shell_select_item to a gtk_menu_shell_real_select_item(),
make gtk_menu_shell_select_item() call the virtual function, and
override the virtual function in GtkMenu to scroll the newly selected
item on screen before chaining to the parent implementation.

Regards,
                                        Owen