Re: Tooltips patch [take 1]

[Kristian Rietveld <kris imendio com>]

On Wed, Sep 27, 2006 at 03:31:21PM +0200, Tim Janik wrote:
> On Thu, 20 Jul 2006, Kristian Rietveld wrote:
> >  * What to do with gtk_tooltip_force_query_tooltip() and
> >    gtk_tooltip_force_query_tooltip_for_widget().  The former will look up
> >    the widget under the mouse pointer and immediately display that
> >    tooltip.
> 
> that's not what was discussed/proposed though. force_query should simply
> trigger the normal what-tooltip-is-needed-where logic that is also triggered
> when the mouse moves. that's so that applications can notify gtk about the
> need to display a (new) tooltip, but the usual tooltip behaviour like timeouts
> is preserved.

That is one thing we need, but also ...

> >    But I also see uses (and IIRC this has also been requested),
> >    to immediately show the tooltip of a specific widget.
> 
> i'm not sure what use case you're referring to, can you please elaborate?

... there have been requests for (immediately) showing tooltips on-demand:

  http://mail.gnome.org/archives/gtk-devel-list/2006-January/msg00262.html
  http://mail.gnome.org/archives/gtk-devel-list/2006-January/msg00278.html
  http://mail.gnome.org/archives/gtk-devel-list/2006-April/msg00023.html

Use cases I can think of are more like "notification tips".  Do we want to
support those with the tooltips code?  Think of the "New software
installed" tooltip in Windows (triggered by code, positioned by code),
or the notifications in the modern GNOME desktop.

> >    This probably
> >    also needs to display the tooltip above/below the widget and not
> >    the mouse pointer.  Once we decide on a set of functions we should add
> >    those to the header file.
> 
> i'd really like to nail the use cases here first, otherwise we'll end up
> with some wild useless API growth once more. the former API discussions
> tried to adress all use cases raised, either there's some misunderstanding
> in how those are covered, or you're referring to cases not yet raised.
> in either case, a bit of elaboration will help ;)
> 
> >  * Also, do we want to be able to set a positioning function?
> 
> use cases? (again ;)

The notification tips mentioned above or maybe something like the tips
on tree view rows which were discussed on this list earlier.  (However
having the tooltip right below the mouse pointer is probably troublesome
wrt event handling).

> one related use case that comes to my mind here is the info window in the
> maemo platform which is essentially used to display status information and
> tooltips.
> for that it's probably better to plug something even more generic like a
> tooltips display function that can also adjust the tooltip's GtkWindow by
> setting up styles, etc.

That could happen (pretty sure about the tooltips case, less about the
status info case) in the query-tooltip callback if you use a custom
GtkWindow for the tooltip.

Also, in a discussion earlier on this list it was mentioned that theme
engines should be able to configure/customize the positioning of the
tooltip:

  http://mail.gnome.org/archives/gtk-devel-list/2006-April/msg00204.html

We are going to need some API for this, right?

> >    widget_signals[SHOW] =
> >      g_signal_new (I_("show"),
> >  		  G_TYPE_FROM_CLASS (gobject_class),
> > @@ -1388,6 +1421,18 @@ gtk_widget_class_init (GtkWidgetClass *k
> >  		  _gtk_marshal_BOOLEAN__BOXED,
> >  		  G_TYPE_BOOLEAN, 1,
> >  		  GDK_TYPE_EVENT | G_SIGNAL_TYPE_STATIC_SCOPE);
> > +  widget_signals[QUERY_TOOLTIP] =
> > +    g_signal_new (I_("query_tooltip"),
> 
> nit pick, the canonical name here is query-tooltip.

Is there actually a standard for canonical names? (Isn't it -?) The names in
gtkwidget.c and several other widgets seem to use both - and _.  (And
the last signal in gtkwidget.c used _, so I used that too :).

> > +
> > +      tmp = (tooltip_window != NULL || (tooltip_markup ? strlen (tooltip_markup) : 0));
> > +      gtk_widget_set_has_tooltip (widget, tmp);
> > +      break;
> 
> you do realize though, that empty strings "" still get copied and allocate space,
> allthough you regard them as has-tooltips=FALSE? ;)

I think I read your original proposal a bit too closely then:

  - setting GtkWidget::tooltip-markup should automatically set:
    GtkWidget::has-tooltip = (custom_window != NULL ||
                              GtkWidget::tooltip-markup != "");

I changed it to set has-tooltip for tooltip_markup == NULL now.

> > @@ -1274,6 +1275,13 @@ gtk_main_do_event (GdkEvent *event)
> >      {
> >        _gtk_clipboard_handle_event (&event->owner_change);
> >        return;
> > +    }
> > +
> > +  if (event->type == GDK_LEAVE_NOTIFY
> > +      || event->type == GDK_MOTION_NOTIFY
> > +      || event->type == GDK_SCROLL)
> > +    {
> > +      _gtk_tooltip_handle_event (event);
> 
> hm, why exactly isn't GDK_ENTER_NOTIFY also interesting here?

We don't use EnterNotify to determine whether we need to start the
tooltip delay.  We start the tooltip delay once we have received a
MotionNotify event.

> also, note that you're trying to handle tooltiping *before* ordinary
> event handling here.
> i don't think that makes much sense, as handling motion/neter/leave
> will often change app/widget/window state. that is especially true
> for GDK_SCROLL, you'll allmost always query the wrong tooltip this
> way.
> 
> instead, you should be calling gtk_tooltip_trigger_tooltip_query()
> (or a GdkEvent* variant thereof), after the normal event handling
> is done. (and be carefull there about event->any.window still
> existing, currently, gtk_get_event_widget() does this for you).

So that means gtk_tooltip_trigger_tooltip_query() would have to handle
the events we now handle in _gtk_tooltip_handle_event()?  I don't think
that makes much sense.  Right now, I've just moved the call to
_gtk_tooltip_handle_event() to below the regular event handling, which
works just fine.

> > +struct _GtkTooltipPrivate
> > +{
> > +  GtkWidget *window;
> > +  GtkWidget *alignment;
> > +  GtkWidget *box;
> > +  GtkWidget *image;
> > +  GtkWidget *label;
> > +  GtkWidget *custom_widget;
> > +
> > +  guint timeout_id;
> 
> this can be moved into struct _GtkTooltip if we decide to leave the
> Tooltip class local to the .c file like you did it. which makes sense
> i think, since there is no point in ever deriving from it or fiddling
> with instances of that class.

Agreed, got rid of the private struct.

> > +/* variables */
> > +static GtkWidget *current_tooltip_widget = NULL;
> > +static GtkWindow *current_window = NULL;
> > +static GtkTooltip *current_tooltip = NULL;
> 
> hm, this doesn't look all that good to me.
> for one, tooltip state should be kept in GtkTooltip since it'll be needed
> per display anyway, and for another, there's no need to keep the tooltip
> widget handle around after completion of querying the tooltip before
> showing it.

Moved all the tooltip state info to GtkTooltip.  It's all per-display now.
However, for the keyboard tooltips I do need to keep a keyboard_widget
pointer in order to get it working reliably.

> > +  /* Position the tooltip */
> > +  if (tooltip_keyboard_mode_enabled)
> > +    {
> > +      gdk_window_get_origin (widget->window, &x, &y);
> > +      if (GTK_WIDGET_NO_WINDOW (widget))
> > +        {
> > +	  x += widget->allocation.x;
> > +	  y += widget->allocation.y;
> > +	}
> > +
> > +      /* FIXME: want to get this right */
> > +      x += widget->allocation.width / 2;
> > +      y += widget->allocation.height + 4;
> 
> ok, fix it then ;)
> do you have any questions regarding the positioning?

I am still thinking on what the best way to implement the positioning
behavior pointed out in this mail is:

  http://mail.gnome.org/archives/gtk-devel-list/2006-April/msg00201.html

I don't really like the arbitrary 4 in the current code ;)


> all in all, this looks like a substantial prototype implementation
> already, but it's certainly still lacking in some key areas.
> in any case, it's so large already, that patch+review is becoming
> tedious as development process, therefore i think we should get this
> into CVS in its current form and build/extend/fix from there.
> a branch might be good for that but it wouldn't be too bad having it
> in HEAD already either.
> that'll also allow concurrent hacking on the new tooltip implementation.

I am almost done processing all of your comments.  I think it's a good
idea to post a patch here once more and then commit on HEAD and continue
from there.



thanks,

-kris.