Re: Tooltips patch [take 1]

[Tim Janik <timj imendio com>]

On Fri, 19 Jan 2007, Kristian Rietveld wrote:

> Hi Tim,

> On Tue, Jan 09, 2007 at 02:51:56PM +0100, Tim Janik wrote:

> > 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?

not sure, maybe tooltips used to resize and show the toplevel window 
directly when this code was written.

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

that's an implementation detail, child_location_foreach() has the
correct GTK_IS_CONTAINER check to avoid recursion on non-containers.
the logic of child_location_foreach() still needs to be executed
for the event_widget here, regardless of whether it's a container
or not. you can get that behaviour by calling child_location_foreach()
directly initially, instead of indirectly through gtk_container_foreach.

> > > +  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).

that was meant to say "gaim v1.2.1" btw

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

great.

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

ok, check it in already! ;)

> thanks,
>
> -kris.

---
ciaoTJ