The cursor behavior in GTK is wrong

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Bo Yang]

The Wibkit Gtk use WidgetPrivate::cursor to store the current cursor, but the cursor is get through a C++ static variable, and this cause something wrong, when there is frame (the key point is that there are many widget and then many WidgetPrivate::cursor, but the cursor static variable only one). When the cursor cross frame side, it may not be changed...

I will upload a testcase to display the problem and a patch which fix it.

[Bo Yang]

Created attachment 27726 [details]
The testcase file.

Please open the test file. 
It will create a two columns frame, with each on open the http://www.webkit.org/. 
Then , move your mouse across the two frames splitter, you will find the cursor will display incorrectly. I mean, when the mouse is on the splitter, it should be a double arrow, else it should be a pointer. But, WebKit GTK display the cursor wrong.

[Bo Yang]

Created attachment 27727 [details]
The first version

Change the WidgetPriate::cursor to a static memeber, that means the whole process should only use one cursor at one time, this is rational.

[Xan Lopez]

I think this just fixes the bug by accident. By using the same cached cursor for all widgets, and initializing it to 0 on each instance, you effectively disable it once per widget created. Also, how does making the cache per-class makes sense? Could you elaborate?

[Bo Yang]

(In reply to comment #3)
> I think this just fixes the bug by accident. By using the same cached cursor
> for all widgets, and initializing it to 0 on each instance, you effectively
> disable it once per widget created. Also, how does making the cache per-class
> makes sense? Could you elaborate?

Cache per-class means cache per-application. One application hold one cursor type at one time. Make the cursor a static variable, we can trace the cursor type trough all the widgets of the application. I think this make sense. 
Disable it once per widget created indeed make no sense. I forget to delete that line in the patch. I have upload a new patch, thanks. 

[Bo Yang]

Created attachment 27741 [details]
The second version

[Xan Lopez]

(In reply to comment #4)
> (In reply to comment #3)
> > I think this just fixes the bug by accident. By using the same cached cursor
> > for all widgets, and initializing it to 0 on each instance, you effectively
> > disable it once per widget created. Also, how does making the cache per-class
> > makes sense? Could you elaborate?
> 
> Cache per-class means cache per-application. One application hold one cursor
> type at one time. Make the cursor a static variable, we can trace the cursor
> type trough all the widgets of the application. I think this make sense. 
> Disable it once per widget created indeed make no sense. I forget to delete
> that line in the patch. I have upload a new patch, thanks. 
> 

Well, sure, I was hoping for the rationale of how this fixes the problem. I think I get it now though: the issue is with widgets that get set a cursor A, then the user moves elsewhere with the cursor changing, and when he tries to come back and set cursor A again the cache says "no, I have it already", and nothing happens. So making the cache global indeed fixes the issue (and saves memory...).

The last patch looks good to me.

[Xan Lopez]

Created attachment 27743 [details]
cursor.patch

Make the cursor cache just a static file variable and remove some stale code, pointed out by Mark Rowe.

[Mark Rowe (bdash)]

Comment on attachment 27743 [details]
cursor.patch

> +static GdkCursor* lastSetCursor = 0;

There's no need to explicitly initialise this to zero.

> @@ -85,11 +74,11 @@ void Widget::setCursor(const Cursor& cursor)
>      // gdk_window_set_cursor() in certain GDK backends seems to be an
>      // expensive operation, so avoid it if possible.
>  
> -    if (pcur == m_data->cursor)
> +    if (pcur == lastSetCursor)
>          return;
>  
>      gdk_window_set_cursor(gdkDrawable(platformWidget()) ? GDK_WINDOW(gdkDrawable(platformWidget())) : GTK_WIDGET(root()->hostWindow()->platformWindow())->window, pcur);
> -    m_data->cursor = pcur;
> +    lastSetCursor = pcur;
>  }

Ugh, does this code really use pcur as a variable name? :-(

r=me

[Bo Yang]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > I think this just fixes the bug by accident. By using the same cached cursor
> > > for all widgets, and initializing it to 0 on each instance, you effectively
> > > disable it once per widget created. Also, how does making the cache per-class
> > > makes sense? Could you elaborate?
> > 
> > Cache per-class means cache per-application. One application hold one cursor
> > type at one time. Make the cursor a static variable, we can trace the cursor
> > type trough all the widgets of the application. I think this make sense. 
> > Disable it once per widget created indeed make no sense. I forget to delete
> > that line in the patch. I have upload a new patch, thanks. 
> > 
> 
> Well, sure, I was hoping for the rationale of how this fixes the problem. I
> think I get it now though: the issue is with widgets that get set a cursor A,
> then the user moves elsewhere with the cursor changing, and when he tries to
> come back and set cursor A again the cache says "no, I have it already", and
> nothing happens. So making the cache global indeed fixes the issue (and saves
> memory...).

Right! 

[Gustavo Noronha (kov)]

Landed in r41055.