Bug 23989 - The cursor behavior in GTK is wrong
Summary: The cursor behavior in GTK is wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-02-17 05:08 PST by Bo Yang
Modified: 2009-02-18 04:17 PST (History)
1 user (show)

See Also:


Attachments
The testcase file. (172 bytes, text/html)
2009-02-17 05:25 PST, Bo Yang
no flags Details
The first version (931 bytes, patch)
2009-02-17 05:27 PST, Bo Yang
no flags Details | Formatted Diff | Diff
The second version (1003 bytes, patch)
2009-02-17 18:35 PST, Bo Yang
no flags Details | Formatted Diff | Diff
cursor.patch (3.70 KB, patch)
2009-02-18 01:21 PST, Xan Lopez
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bo Yang 2009-02-17 05:08:22 PST
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.
Comment 1 Bo Yang 2009-02-17 05:25:49 PST
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.
Comment 2 Bo Yang 2009-02-17 05:27:46 PST
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.
Comment 3 Xan Lopez 2009-02-17 11:47:13 PST
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?
Comment 4 Bo Yang 2009-02-17 18:34:25 PST
(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. 
Comment 5 Bo Yang 2009-02-17 18:35:05 PST
Created attachment 27741 [details]
The second version
Comment 6 Xan Lopez 2009-02-18 00:40:24 PST
(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.
Comment 7 Xan Lopez 2009-02-18 01:21:29 PST
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.
Comment 8 Mark Rowe (bdash) 2009-02-18 01:25:30 PST
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
Comment 9 Bo Yang 2009-02-18 02:10:18 PST
(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! 
Comment 10 Gustavo Noronha (kov) 2009-02-18 04:17:58 PST
Landed in r41055.