Re: scrolling menus

[Alexander Larsson <alla lysator liu se>]

On 7 Nov 2000, Owen Taylor wrote:

> 
> 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.
 I'll look into it.
 
>  * 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.

Well, the parameter was introduced to be able to have different behaviour
between the optionmenu and submenus from item menus (menubars). 

I've been thinking, and i can't come up with a placement strategy that
works for both cases and has no extra arguments. But it would be possible
to change the scroll_offset argument to a boolean that decides whether to
cut off the menu or push the menu into the visible area. That would
probably be easier to understand by the developer (i.e. none of the
scrolling stuff leaks out from gtkmenu.c).

Is that good enught?
 
>  * 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.
 Will do.
 
>  * 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.
 Will do.
 
> > -      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.

No, changing the parameter to TRUE in gtk_menu_popup causes flicker in a
torn off menu when poping up the corresponding menu from the menubar.

As before i don't claim to understand the reasoning, but the current way
just looks best.
 
> >  	{
> > -	  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.

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

Well, It isn't strictly necessary anymore. When menus from menubars poped
up covering the menubar this was necessary to be able to determine if the
user clicked once quick or held down the mouse button.

Since menubars are not covered by menus anymore i will remove this.
   
/ Alex