Bug 697 - GLWindow.setPointerVisible(boolean) fails after several calls
: GLWindow.setPointerVisible(boolean) fails after several calls
Status: RESOLVED FIXED
Product: Newt
Classification: JogAmp_Core
Component: windows
: 1
: pc_all windows
: P3 normal
Assigned To: Julien Gouesse
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-07 12:52 CET by Julien Gouesse
Modified: 2013-04-11 08:06 CEST (History)
2 users (show)

See Also:
Type: DEFECT
SCM Refs:
c2e02b9964860dc59af92187ae124695e1e2f1dc 4db745e84fac610f85ab085e5c147e571e82e008 214794419b2e736d581bb57bf9b5838b09e28f1c dc898f14eeebf524726c820e25ffefb166a6c7f3
Workaround: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Gouesse 2013-03-07 12:52:07 CET
GLWindow.setPointerVisible(boolean) calls ShowCursor(_In_  BOOL bShow) under Windows:
http://msdn.microsoft.com/fr-fr/library/windows/desktop/ms648396%28v=vs.85%29.aspx

It stops working after several calls. I have to enable the debug logs to get some more information. Other Java libraries that allow to hide the cursor usually set a "blank" cursor instead of using the native call above. There are several possible root causes:
- the absolute value of the new display counter becomes so great that at most ten attempts are not enough to show/hide the cursor
- ShowCursor is not reliable enough whatever the way we use it
- the Java method is not called from the thread that creates the GLWindow instance
If one of the 2 first possible root causes is confirmed, the bug must be fixed in NEWT. Otherwise, the bug can be fixed in third party libraries by using the proper thread to perform this call.

The native implementation of this method under Windows is here:
https://github.com/sgothel/jogl/blob/master/src/newt/native/WindowsWindow.c#L1661
Comment 1 Julien Gouesse 2013-03-07 23:23:44 CET
This bug is only reproducible when the user isn't clicking on the window whose cursor is expected to change.
Comment 2 Julien Gouesse 2013-03-08 00:56:39 CET
ShowCursor is used only when WM_SETCURSOR is sent. It means that we have to send WM_SETCURSOR even though the cursor hasn't changed or we will have to implement a more complicated feature to allow setting a native cursor and using a blank cursor to simulate its invisibility.
Comment 3 Julien Gouesse 2013-03-08 00:59:17 CET
This is already fixed in this library:
http://sourceforge.net/p/cimg/bugs/47/
Comment 4 Julien Gouesse 2013-03-08 01:07:48 CET
We can use SendMessage in our method:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms644950%28v=vs.85%29.aspx

SendMessage(HWND,WM_SETCURSOR,0,0) should work.

We can use DefWindowProc elsewhere to force the use of your custom treatment on WM_SETCURSOR:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms633572%28v=vs.85%29.aspx
Comment 5 Sven Gothel 2013-03-26 03:32:42 CET
(In reply to comment #1)
> This bug is only reproducible when the user isn't clicking on the window
> whose cursor is expected to change.

I like to know more details about this statement,
i.e. how do you reproduce this bug.

setPointerVisible(..) shall only impact the pointers visibility state
when it's window has the focus (-> is 'clicked' for example).

unfocusing a window w/ invisible pointer, shall make the pointer visible again,
i.e. when moving to another window, e.g. by pointer motion or focus traversal (TAB).

?
Comment 6 Julien Gouesse 2013-03-28 14:59:37 CET
(In reply to comment #5)
> (In reply to comment #1)
> > This bug is only reproducible when the user isn't clicking on the window
> > whose cursor is expected to change.
> 
> I like to know more details about this statement,
> i.e. how do you reproduce this bug.
> 
I wasn't accurate enough. Actually, this feature almost never works. I succeeded in making it work only once when clicking on the window and calling this method at the same time.

> setPointerVisible(..) shall only impact the pointers visibility state
> when it's window has the focus (-> is 'clicked' for example).
> 
> unfocusing a window w/ invisible pointer, shall make the pointer visible
> again,
> i.e. when moving to another window, e.g. by pointer motion or focus
> traversal (TAB).
> 
> ?
It only impacts the pointers visibility state of the window concerned by this call. When another window acquires the focus, it isn't impacted at all.
Comment 7 Julien Gouesse 2013-03-28 15:07:28 CET
Should I add a field into WindowUserData to store the visibility of the pointers? Is that what you meant?
Comment 8 Julien Gouesse 2013-03-28 15:59:13 CET
I assume I have to add a new method "pointerVisibleChanged" into WindowImpl. Then, I have to use the existing field "pointerVisible" of WindowImpl in the native source code.
Comment 9 Julien Gouesse 2013-03-28 16:09:30 CET
I can call (*env)->CallVoidMethod(env, wud->jinstance, pointerVisibleChangedID, JNI_FALSE, JNI_TRUE); to set "pointerVisible" to true.
Comment 10 Julien Gouesse 2013-03-28 16:17:49 CET
I can call (*env)->CallBooleanMethod(env, window, isPointerVisibleID) to get the value of "pointerVisible" before calling ShowCursor in the case WM_SETCURSOR in the method "wndProc".
Comment 11 Julien Gouesse 2013-03-28 23:47:12 CET
The pull request containing the fix and the unit test is ready:
https://github.com/sgothel/jogl/pull/61
Comment 12 Sven Gothel 2013-04-04 06:24:43 CEST
(In reply to comment #11)
> The pull request containing the fix and the unit test is ready:
> https://github.com/sgothel/jogl/pull/61

- your branch is not clean, pls merge first w/ master (PNGImage collision)
  - also, pls provide a branch w/ _only_ changes for this bug 
- your patch uses a Java callback, even though we discussed otherwise, 
  i.e. using the WindowUserData structure as attached to the window 
- you use a forever-loop, maybe you like to end this after a while to not block the machine ?
Comment 13 Julien Gouesse 2013-04-04 11:30:33 CEST
(In reply to comment #12)
> (In reply to comment #11)
> > The pull request containing the fix and the unit test is ready:
> > https://github.com/sgothel/jogl/pull/61
> 
> - your branch is not clean, pls merge first w/ master (PNGImage collision)
Ok I will try to fix that as soon as possible. I have used GIT for a few months and it is a bit difficult for me.

>   - also, pls provide a branch w/ _only_ changes for this bug 
I will provide a clean pull request containing only the necessary changes

> - your patch uses a Java callback, even though we discussed otherwise, 
>   i.e. using the WindowUserData structure as attached to the window 
I thought the use of the Java callback was acceptable but I will use WindowUserData.

> - you use a forever-loop, maybe you like to end this after a while to not
> block the machine ?
ShowCursor has an effect only when it is called from the thread that created the window, there is no risk to block the machine.
Comment 14 Julien Gouesse 2013-04-04 21:41:49 CEST
I have just cleaned up my mess on Github. I will provide a single commit with the new unit test and the fix very soon.
Comment 15 Sven Gothel 2013-04-05 05:14:53 CEST
(In reply to comment #14)
> I have just cleaned up my mess on Github. I will provide a single commit
> with the new unit test and the fix very soon.

[1] I don't see the reason why you don't use  the safe loop of 
calling ShowCursor(..) - At least this won't allow an infinite loop.

[2] Why providing a Java callback 'pointerVisibleChanged()' ?
Pointer visibility shall be only downstream (Java -> Native), 
there shall be no way otherwise - which is actually guaranteed by 
your patch.

+++

I read references from gaming sites that indeed claim
that cursor visibility / mods shall happen on WM_SETCURSOR msg. .. well, OK then.

I would like to see this patch as follows:

- WM_SETCURSOR event handling only calls ShowCursor(..)
  _if_ it's change is explicitly flagged.
    WindowUserData { 'jboolean pointerVisible' -> 'int pointerVisible' }
  where value 0 indicates no change, -1 hide, 1 show.
  This b/c I don't know how often WM_SETCURSOR is being called,
  and it should only be performed when triggered.
  Otherwise the cursor display counter could overlap !?
  Also I like to see it using the safe-loop for ShowCursor(..) 
  via refactored static c-method -> SafeShowCursor(..) .. see above.

- No Java callback - state is downstream

I could apply the above changes ontop of your patch, 
but ofc would prefer you doing it.
Comment 16 Julien Gouesse 2013-04-05 10:02:22 CEST
(In reply to comment #15)
> (In reply to comment #14)
> > I have just cleaned up my mess on Github. I will provide a single commit
> > with the new unit test and the fix very soon.
> 
> [1] I don't see the reason why you don't use  the safe loop of 
> calling ShowCursor(..) - At least this won't allow an infinite loop.
> 
Ok I will use a safe loop but it is really useless as there can't be concurrent efficient calls to ShowCursor. Keep in mind that I will use a bigger count of attempts than you did in the first implementation, "10" was too small.

> [2] Why providing a Java callback 'pointerVisibleChanged()' ?
> Pointer visibility shall be only downstream (Java -> Native), 
> there shall be no way otherwise - which is actually guaranteed by 
> your patch.
> 
I assume it means my change in src/newt/classes/jogamp/newt/WindowImpl.java is completely useless. I will remove it and its calls in WindowsWindow.c.

> +++
> 
> I read references from gaming sites that indeed claim
> that cursor visibility / mods shall happen on WM_SETCURSOR msg. .. well, OK
> then.
> 
Yes, that's right.

> I would like to see this patch as follows:
> 
> - WM_SETCURSOR event handling only calls ShowCursor(..)
>   _if_ it's change is explicitly flagged.
Ok.

>     WindowUserData { 'jboolean pointerVisible' -> 'int pointerVisible' }
>   where value 0 indicates no change, -1 hide, 1 show.
Ok. Then, pointerVisible will be set to zero at init.

>   This b/c I don't know how often WM_SETCURSOR is being called,
>   and it should only be performed when triggered.
>   Otherwise the cursor display counter could overlap !?
>   Also I like to see it using the safe-loop for ShowCursor(..) 
>   via refactored static c-method -> SafeShowCursor(..) .. see above.
> 
> - No Java callback - state is downstream
Ok.

> 
> I could apply the above changes ontop of your patch, 
> but ofc would prefer you doing it.

I'll do so.
Comment 17 Sven Gothel 2013-04-06 06:29:52 CEST
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > I have just cleaned up my mess on Github. I will provide a single commit
> > > with the new unit test and the fix very soon.
> > 
> > [1] I don't see the reason why you don't use  the safe loop of 
> > calling ShowCursor(..) - At least this won't allow an infinite loop.
> > 
> Ok I will use a safe loop but it is really useless as there can't be
> concurrent efficient calls to ShowCursor. Keep in mind that I will use a
> bigger count of attempts than you did in the first implementation, "10" was
> too small.
> 
> > [2] Why providing a Java callback 'pointerVisibleChanged()' ?
> > Pointer visibility shall be only downstream (Java -> Native), 
> > there shall be no way otherwise - which is actually guaranteed by 
> > your patch.
> > 
> I assume it means my change in src/newt/classes/jogamp/newt/WindowImpl.java
> is completely useless. I will remove it and its calls in WindowsWindow.c.
> 
> > +++
> > 
> > I read references from gaming sites that indeed claim
> > that cursor visibility / mods shall happen on WM_SETCURSOR msg. .. well, OK
> > then.
> > 
> Yes, that's right.
> 
> > I would like to see this patch as follows:
> > 
> > - WM_SETCURSOR event handling only calls ShowCursor(..)
> >   _if_ it's change is explicitly flagged.
> Ok.
> 
> >     WindowUserData { 'jboolean pointerVisible' -> 'int pointerVisible' }
> >   where value 0 indicates no change, -1 hide, 1 show.
> Ok. Then, pointerVisible will be set to zero at init.
> 
> >   This b/c I don't know how often WM_SETCURSOR is being called,
> >   and it should only be performed when triggered.
> >   Otherwise the cursor display counter could overlap !?
> >   Also I like to see it using the safe-loop for ShowCursor(..) 
> >   via refactored static c-method -> SafeShowCursor(..) .. see above.
> > 
> > - No Java callback - state is downstream
> Ok.
> 
> > 
> > I could apply the above changes ontop of your patch, 
> > but ofc would prefer you doing it.
> 
> I'll do so.

your commit c2e02b9964860dc59af92187ae124695e1e2f1dc only contains the unit test
Comment 18 Julien Gouesse 2013-04-06 10:49:25 CEST
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > I have just cleaned up my mess on Github. I will provide a single commit
> > > > with the new unit test and the fix very soon.
> > > 
> > > [1] I don't see the reason why you don't use  the safe loop of 
> > > calling ShowCursor(..) - At least this won't allow an infinite loop.
> > > 
> > Ok I will use a safe loop but it is really useless as there can't be
> > concurrent efficient calls to ShowCursor. Keep in mind that I will use a
> > bigger count of attempts than you did in the first implementation, "10" was
> > too small.
> > 
> > > [2] Why providing a Java callback 'pointerVisibleChanged()' ?
> > > Pointer visibility shall be only downstream (Java -> Native), 
> > > there shall be no way otherwise - which is actually guaranteed by 
> > > your patch.
> > > 
> > I assume it means my change in src/newt/classes/jogamp/newt/WindowImpl.java
> > is completely useless. I will remove it and its calls in WindowsWindow.c.
> > 
> > > +++
> > > 
> > > I read references from gaming sites that indeed claim
> > > that cursor visibility / mods shall happen on WM_SETCURSOR msg. .. well, OK
> > > then.
> > > 
> > Yes, that's right.
> > 
> > > I would like to see this patch as follows:
> > > 
> > > - WM_SETCURSOR event handling only calls ShowCursor(..)
> > >   _if_ it's change is explicitly flagged.
> > Ok.
> > 
> > >     WindowUserData { 'jboolean pointerVisible' -> 'int pointerVisible' }
> > >   where value 0 indicates no change, -1 hide, 1 show.
> > Ok. Then, pointerVisible will be set to zero at init.
> > 
> > >   This b/c I don't know how often WM_SETCURSOR is being called,
> > >   and it should only be performed when triggered.
> > >   Otherwise the cursor display counter could overlap !?
> > >   Also I like to see it using the safe-loop for ShowCursor(..) 
> > >   via refactored static c-method -> SafeShowCursor(..) .. see above.
> > > 
> > > - No Java callback - state is downstream
> > Ok.
> > 
> > > 
> > > I could apply the above changes ontop of your patch, 
> > > but ofc would prefer you doing it.
> > 
> > I'll do so.
> 
> your commit c2e02b9964860dc59af92187ae124695e1e2f1dc only contains the unit
> test

The missing file has been added into the commit 4db745e84fac610f85ab085e5c147e571e82e008.
Comment 19 Sven Gothel 2013-04-11 08:06:05 CEST
Merged _and_ FIXED.