Bug 697

This bug is only reproducible when the user isn't clicking on the window whose cursor is expected to change.

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.

This is already fixed in this library:
http://sourceforge.net/p/cimg/bugs/47/

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

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

?

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

Should I add a field into WindowUserData to store the visibility of the pointers? Is that what you meant?

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.

I can call (*env)->CallVoidMethod(env, wud->jinstance, pointerVisibleChangedID, JNI_FALSE, JNI_TRUE); to set "pointerVisible" to true.

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".

The pull request containing the fix and the unit test is ready:
https://github.com/sgothel/jogl/pull/61

(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 ?

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

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.

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

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

(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

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

Merged _and_ FIXED.