Bug 32819 - [GTK] Change the tooltip implementation
: [GTK] Change the tooltip implementation
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-21 03:59 PST by Alejandro G. Castro
Modified: 2010-01-14 03:57 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 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 Alejandro G. Castro 2009-12-21 04:36:00 PST
Created attachment 45324 [details]
Initial proposed patch
Comment 2 WebKit Review Bot 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 Alejandro G. Castro 2009-12-22 03:01:59 PST
Created attachment 45378 [details]
Proposed patch

Style review.
Comment 4 WebKit Review Bot 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 Gustavo Noronha (kov) 2010-01-07 17:04:00 PST
Comment on attachment 45378 [details]
Proposed patch

> +#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 Alejandro G. Castro 2010-01-11 04:38:42 PST
(In reply to comment #5)
> (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.
> 

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 Alejandro G. Castro 2010-01-11 10:41:45 PST
Created attachment 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 WebKit Review Bot 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 Xan Lopez 2010-01-14 03:03:36 PST
Comment on attachment 46288 [details]
Proposed patch

Looks great to me.
Comment 10 WebKit Commit Bot 2010-01-14 03:57:06 PST
Comment on attachment 46288 [details]
Proposed patch

Clearing flags on attachment: 46288

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