Bug 32819 - [GTK] Change the tooltip implementation
: [GTK] Change the tooltip implementation
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-12-21 03:59 PST by
Modified: 2010-01-14 03:57 PST (History)


Attachments
Initial proposed patch (5.65 KB, patch)
2009-12-21 04:36 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (5.67 KB, patch)
2009-12-22 03:01 PST, Alejandro G. Castro
gns: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch (6.40 KB, patch)
2010-01-11 10:41 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-12-21 03:59:15 PST
Check the comments in the bug 15793 for more information about the current solution. The proposal is to use the query-tooltip signal to handle it.
------- Comment #1 From 2009-12-21 04:36:00 PST -------
Created an attachment (id=45324) [details]
Initial proposed patch
------- Comment #2 From 2009-12-21 04:38:06 PST -------
Attachment 45324 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebview.cpp:2634:  Missing space after ,  [whitespace/comma] [3]
WebKit/gtk/webkit/webkitwebview.cpp:2635:  webkit_web_view_query_tooltip is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.cpp:2686:  Missing space after ,  [whitespace/comma] [3]
WebKit/gtk/webkit/webkitwebview.cpp:2687:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitwebview.cpp:2689:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitwebview.cpp:4045:  webkit_web_view_set_tooltip_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.cpp:4052:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 7
------- Comment #3 From 2009-12-22 03:01:59 PST -------
Created an attachment (id=45378) [details]
Proposed patch

Style review.
------- Comment #4 From 2009-12-22 03:04:47 PST -------
Attachment 45378 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebview.cpp:2635:  webkit_web_view_query_tooltip is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.cpp:4045:  webkit_web_view_set_tooltip_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2
------- Comment #5 From 2010-01-07 17:04:00 PST -------
(From update of attachment 45378 [details])
> +#if GTK_CHECK_VERSION(2, 12, 0)
> +    priv->tooltipText = 0;
> +    gtk_widget_set_has_tooltip(GTK_WIDGET(webView), TRUE);
> +    g_signal_connect(webView, "query-tooltip", G_CALLBACK(webkit_web_view_query_tooltip), 0);
> +#endif

You should override the GtkWidget class method instead of connecting to the signal.

> +void webkit_web_view_set_tooltip_text(WebKitWebView* webView, const char* tooltip)
> +{
> +    WebKitWebViewPrivate* priv = webView->priv;
> +    g_free(priv->tooltipText);
> +    if (tooltip && *tooltip != '\0')
> +        priv->tooltipText = g_strdup(tooltip);
> +    else
> +        priv->tooltipText = 0;
> +}

What about the 'has-tooltip' property? You should probably set it to TRUE/FALSE, as appropriate, here. Otherwise the tooltips will only be displayed when the webview has keyboard focus, as I understand it. The patch looks awesome otherwise, nice work!
------- Comment #6 From 2010-01-11 04:38:42 PST -------
(In reply to comment #5)
> (From update of attachment 45378 [details] [details])
> > +#if GTK_CHECK_VERSION(2, 12, 0)
> > +    priv->tooltipText = 0;
> > +    gtk_widget_set_has_tooltip(GTK_WIDGET(webView), TRUE);
> > +    g_signal_connect(webView, "query-tooltip", G_CALLBACK(webkit_web_view_query_tooltip), 0);
> > +#endif
> 
> You should override the GtkWidget class method instead of connecting to the
> signal.
> 

Yep, I'll do it.

> > +void webkit_web_view_set_tooltip_text(WebKitWebView* webView, const char* tooltip)
> > +{
> > +    WebKitWebViewPrivate* priv = webView->priv;
> > +    g_free(priv->tooltipText);
> > +    if (tooltip && *tooltip != '\0')
> > +        priv->tooltipText = g_strdup(tooltip);
> > +    else
> > +        priv->tooltipText = 0;
> > +}
> 
> What about the 'has-tooltip' property? You should probably set it to
> TRUE/FALSE, as appropriate, here. Otherwise the tooltips will only be displayed
> when the webview has keyboard focus, as I understand it. The patch looks
> awesome otherwise, nice work!

AFAIK the property is a widget property and you just have to set it to TRUE once in order to receive the tooltip events. But now that you point out this, one interesting thing would be to avoid the gtkwidget check and use just the webkit one, i.e. set query-tooltip always to FALSE and use gtk_tooltip_trigger_tooltip_query. I'll try to do this.

Thanks for the comments :).
------- Comment #7 From 2010-01-11 10:41:45 PST -------
Created an attachment (id=46288) [details]
Proposed patch

I did both modifications, with this approach we do not ask gtk to send queries if webkit did not set a string in the tooltip. I guess it is a better option to avoid launching the query signal when we know there is no string to be rendered. I've also reviewed the gtk+ version check.
------- Comment #8 From 2010-01-11 10:46:51 PST -------
Attachment 46288 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebview.cpp:1281:  webkit_web_view_query_tooltip is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/webkit/webkitwebview.cpp:4044:  webkit_web_view_set_tooltip_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2
------- Comment #9 From 2010-01-14 03:03:36 PST -------
(From update of attachment 46288 [details])
Looks great to me.
------- Comment #10 From 2010-01-14 03:57:06 PST -------
(From update of attachment 46288 [details])
Clearing flags on attachment: 46288

Committed r53256: <http://trac.webkit.org/changeset/53256>
------- Comment #11 From 2010-01-14 03:57:11 PST -------
All reviewed patches have been landed.  Closing bug.