Re: Tooltips patch [take 1]

[Kristian Rietveld <kris imendio com>]

Hi Tim,

I've taken over most of the changes and left those out of this mail.
Replies to relevant snippets below ...


On Tue, Jan 09, 2007 at 02:51:56PM +0100, Tim Janik wrote:
> > +static gboolean
> > +gtk_tooltip_paint_window (GtkTooltip *tooltip)
> > +{
> > +  GtkRequisition req;
> > +
> > +  gtk_widget_size_request (tooltip->window, &req);
> > +  gtk_paint_flat_box (tooltip->window->style,
> > +		      tooltip->window->window,
> > +		      GTK_STATE_NORMAL,
> > +		      GTK_SHADOW_OUT,
> > +		      NULL,
> > +		      tooltip->window,
> > +		      "tooltip",
> > +		      0, 0,
> > +		      req.width, req.height);
> 
> erm, this paints a box at the size the window would like to have, not
> necessarily the size it has. size requisition is always a bad figure
> to use for painint, people should always only use the actual allocation
> which may be gotten via widget->allocation or gdk_window_get_size.

Fully agree, but actually this code was taken from the old tooltips
implementation.  Was there a reason to do it like this there?

> > +static void
> > +window_to_alloc (GtkWidget *dest_widget,
> > +		 gint       src_x,
> > +		 gint       src_y,
> > +		 gint      *dest_x,
> > +		 gint      *dest_y)
> > +{
> > +  /* Translate from window relative to allocation relative */
> > +  if (!GTK_WIDGET_NO_WINDOW (dest_widget) && dest_widget->parent)
> > +    {
> > +      gint wx, wy;
> > +      gdk_window_get_position (dest_widget->window, &wx, &wy);
> > +
> > +      src_x += wx - dest_widget->allocation.x;
> > +      src_y += wy - dest_widget->allocation.y;
> > +    }
> > +  else
> > +    {
> > +      src_x -= dest_widget->allocation.x;
> > +      src_y -= dest_widget->allocation.y;
> > +    }
> 
> this doesn't seem right to me. is this code copied from somewhere?

Yes, from gtk_widget_translate_coordinates(), which you suggested me to
look at in one of your earlier replies.

> to go from window relative to allocaiton relative, the necessary
> steps are (should work regardless of NO_WINDOW (widget)):
> 1- figure the window offset relative to the window returned by
>     gtk_widget_get_parent_window().
> 2- add the window offset to the coords (this makes them parent_window
>     relative)
> 3- subtract widget->allocation from the coords (this goes from
>     parent_window relative to widget->allocation relative)
> here, 1 has to be able to deal with an arbitrary intermediate hierarchy
> of gdkwindows.

Hmm, hmm.  Let's first analyse gtk_widget_translate_coordinates(), we
can fix window_to_alloc() later on in svn ...

> > +static GtkWidget *
> > +find_widget_under_pointer (GdkWindow *window,
> > +			   gint      *x,
> > +			   gint      *y)
> > +{
> > +  GtkWidget *event_widget;
> > +  struct ChildLocation child_loc = { NULL, NULL, 0, 0 };
> > +
> > +  child_loc.x = *x;
> > +  child_loc.y = *y;
> > +
> > +  gdk_window_get_user_data (window, (void **)&event_widget);
> > +  if (GTK_IS_CONTAINER (event_widget))
> > +    {
> 
> huh? event->window may as well belong to a non-container widget,
> so i think you need to use GTK_IS_WIDGET instead here.

Then we would call gtk_container_foreach() with a non-container
widget...

> > +      child_loc.child = event_widget;
> 
> this line...
> 
> > +
> > +      window_to_alloc (event_widget,
> > +		       child_loc.x, child_loc.y,
> > +		       &child_loc.x, &child_loc.y);
> > +
> > +      event_widget = child_loc.container = child_loc.child;
> > +      child_loc.child = NULL;
> 
> and these two could be reworked to be less confusing, since
> you don't need child_loc.child readily setup in window_to_alloc().

Indeed, small left-over from the old code.

> > +  else
> > +    {
> > +      guint cursor_size;
> > +
> > +      gdk_window_get_pointer (gdk_screen_get_root_window (gtk_widget_get_screen (tooltip_widget)), &x, &y, NULL);
> 
> as pointed out in an above note, we did 'get_pointer' already.
> each _get_pointer is a server round trip, so we should reuse the coords
> from above.
> also, the coords we really want to use are in the event structure, to
> avoid bugs where a slow application/connections causes the tooltip to be
> positioned completely off-target because the mouse moved since the event
> was originally queued (happens regularly here with v1.2.1 for instance).
> but well, for the moment we don't have those here...

I changed the code to cache the coordinates from the last event and
eliminated all three roundtrips in this function.

> > +  tooltip->visible = TRUE;
> 
> isn't this field duplicating the state of
>    (tooltip->current_window && GTK_WIDGET_VISIBLE (tooltip->current_window))
> ?

Ah, indeed.  Replaced it with a GTK_TOOLTIP_VISIBLE() macro.

> > +static void
> > +gtk_tooltip_start_delay (GdkDisplay *display)
> 
> this should probably be gtk_tooltip_start_delayed() ?

I like gtk_tooltip_start_delay() better, the original idea behind this
name was "GtkTooltip start the delay" and not do a delayed start.

> > +
> > +  /* Handle case where mouse pointer moves out of the window */
> > +  if (!tooltip->keyboard_mode_enabled && tooltip->tooltip_widget != widget)
> 
> hm, as i said above, the tooltip might come from an ancestory of the widget,
> so "tooltip->tooltip_widget != widget" looks wrong to me.
> also, i don't quite get why you talk about pointer moves in focus_out.
> for one, gtk+ widget's don't implement focous-out-on-window-leave,
> and for another we already have motion/enter/leave handling ina different
> place.

I don't fully remember why I had to add this; but something in my
testcase was not working properly.  I tried removing it again and I see
no problems now, so the actual cause was probably fixed meanwhile.  It
looks like the same holds for the check on has_tooltip.

> > +  /* Hide tooltip */
> > +  gtk_tooltip_hide_tooltip (tooltip);
> 
> this comment is redundant to a certain extend ;)

:P

> thanks for the new patch. all in all, the code looks fairly complete already.
> there're some logic issues that still need to be worked out, but
> that should be done in SVN. please check this in as soon as possible,
> if you want to keep the history, even before fixing my above comments.

Cool, I would say we check in the current patch with the fresh changes
on my disk and look at the last coordinate translation issue mentioned
above in svn.  If there are no objections I hope to check this in
(together with doc comments :) at the end of next weekend or probably in
the weekend following that week.


thanks,

-kris.