Bug 32819 – [GTK] Change the tooltip implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

Created attachment 45324 [details]
Initial proposed patch

[WebKit Review Bot]

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

[Alejandro G. Castro]

Created attachment 45378 [details]
Proposed patch

Style review.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

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!

[Alejandro G. Castro]

(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 :).

[Alejandro G. Castro]

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.

[WebKit Review Bot]

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

[Xan Lopez]

Comment on attachment 46288 [details]
Proposed patch

Looks great to me.

[WebKit Commit Bot]

Comment on attachment 46288 [details]
Proposed patch

Clearing flags on attachment: 46288

Committed r53256: <http://trac.webkit.org/changeset/53256>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.