Re: scrolling menus

[Owen Taylor <otaylor redhat com>]

Alexander Larsson <alla lysator liu se> writes:

> Ok, here is the latest and greatest. It has no flicker and no known
> bugs. It generally kicks ass.

Yes, it is looking and feeling quite good. I think it is about
ready to go into CVS.

Some remaining comments:
 
 * Once a menu is torn off, it should be vertically resizable. 
   Allowing it to be vertically resizeable but not horizontally
   resizable is just a bit tricky with GTK+ currently, but what
   you can do is get the size requisition of the toplevel and
   then call gtk_window_set_geometry_hints() with the appropriate
   values derived from that.

 * Do we really need the addition of the scroll_offset parameter
   to GtkMenuPositionFunc as well as the automatic handling
   of offscreen positioning? 

   It concerns me from two levels:

    1) Figuring out the proper setting of scroll_offset
       is not going to be easy - for instance, do you need to take
       into the account the size of the scroll arrow? 

    2) If it isn't absolutely necessary, I'm hesitant to break
       the API in this way. The number of actual source-incompatible
       changes in GTK+-2.0 has been kept suprisingly small to this 
       point.

 * The handling of the scroll_adjustment in the code seems clumsy -
   there are several pieces of code that individually handles the
   menu->tearoff_adjustment and calling gtk_menu_scroll_to().

   One possibly better way of doing this is to handle setting
   menu->tearoff_adjustment in gtk_menu_scroll_to(), and
   then in gtk_menu_scrollbar_changed (), do:

    if (adjustment->value != menu->scroll_offset)
      gtk_menu_scroll_to (menu, adjustment->value);

   to prevent infinite loops.

 * You should probably adjust testgtk.c so at least one of the popup
   menus in the menus test is long enough to typically test the scrolling
   menu code. I keep on having to hack it every time I try out
   a new version of your patch.

And a few more specific code comments:

[ in destroy ]

> +  if (menu->timeout_id)
> +    {
> +      g_source_remove (menu->timeout_id);
> +      menu->timeout_id = 0;
> +    }
> +  

This sequence occurs, to my count, something like 9 times. Even
though it is only 5-6 lines, a function gtk_menu_stop_scrolling()
probably would be nice. 

>  static void
> +gtk_menu_real_insert (GtkMenuShell     *menu_shell,
> +		      GtkWidget        *child,
> +		      gint              position)

Could you move gtk_menu_append/prepend/insert into gtkcompat.h
and make them #defines? (Nothing to do with your code, just noticed
when I was suprised you needed the _real_ here)

> -      gtk_menu_reparent (menu, menu->toplevel, TRUE);
> +      gtk_menu_reparent (menu, menu->toplevel, FALSE);

I think we can just always unrealize now and remove the last parameter
to gtk_menu_reparent.

>  	{
> -	  gtk_menu_reparent (menu, menu->tearoff_window, FALSE);
> +	  /* We force an unrealize here so that we don't trigger redrawing/
> +	   * clearing code - this is to avoid flicker.
> +	   */
> +	  gtk_menu_reparent (menu, menu->tearoff_hbox, TRUE);

Why don't you move this comment to gtk_menu_reparent(). Also,
if you felt like putting an abstract of my mail as to _why_
it stops flicker into the code, that probably would be a big
help to someone in the future.

> +  attributes.y = border_width + widget->style->ythickness;
> +  attributes.width = MAX (1, (gint)widget->allocation.width - attributes.x * 2);
> +  attributes.height = MAX (1, (gint)widget->allocation.height - attributes.y * 2);

These (gint) casts are no longer necessary, since I went through
and got rid of all uses of unsigned numbers for dimensions.

> +  x = (GTK_CONTAINER (menu)->border_width + widget->style->xthickness);
> +  y = (GTK_CONTAINER (menu)->border_width + widget->style->ythickness);

Excess parens.

> +      top_pos = height - 2*border_y - (menu->upper_arrow_visible?MENU_SCROLL_ARROW_HEIGHT:0);

Similar to binary operators, there should be spaces spaces around ? and :.

> +  if (widget && GTK_IS_MENU(widget))
                              ^ add space

> +      if (child == menu_item) {

'{' goes on next line.

> +	      if ((y > 0) && !menu->tearoff_active) {

The same.

>    if (GTK_WIDGET_IS_SENSITIVE (menu_item->submenu))
>      {
> +      guint32 etime;
> +      GdkEvent *event = gtk_get_current_event ();
> +
> +      etime = event ? gdk_event_get_time (event) : GDK_CURRENT_TIME;
> +      
>        gtk_menu_popup (GTK_MENU (menu_item->submenu),
>  		      GTK_WIDGET (menu_item)->parent,
>  		      GTK_WIDGET (menu_item),
>  		      gtk_menu_item_position_menu,
>  		      menu_item,
>  		      GTK_MENU_SHELL (GTK_WIDGET (menu_item)->parent)->button,
> -		      0);
> +		      etime);
>      }
>  }

Out of curiousity, why this change? Also, you can use 
gtk_get_current_event_time() to simplify this some.
  
> -  if (include_internals && menu_item->submenu)
> -    (* callback) (menu_item->submenu, callback_data);

Needs to be in ChangeLog.

> +static void
> +gtk_menu_shell_real_insert (GtkMenuShell *menu_shell,
> +			    GtkWidget    *child,
> +			    gint          position)
> +{
>    g_return_if_fail (menu_shell != NULL);
>    g_return_if_fail (GTK_IS_MENU_SHELL (menu_shell));
>    g_return_if_fail (child != NULL);

You don't need g_return_if_fail() of this type in the implementation -
you should assume that the wrapper does these checks. 
g_return_if_fail() are basically for public entry points to
make sure that the person using the functionality hasn't screwed
up; not to try and catch some horrible internal error that
would result in gtk_menu_shell_real_insert() being called with
something that isn't a GtkMenuShell. 

(Same applies to gtk_menu_shell_real_select_item())

So, as I said, this looks good to me. I think it would be good
to get it into CVS so other people can try it out. Once the
above items are resolved, why don't you go ahead and commit.

Regards,
                                        Owen