Re: [PATCH resend] winex11.drv: Try harder to grab pointer.

[Zhiyi Zhang]

On 9/4/19 9:00 PM, Rémi Bernon wrote:
> On 9/4/19 2:09 PM, Zhiyi Zhang wrote:
> > Some window managers temporarily grab the pointer during window transition.
> > We need to wait a while for window manager to release its grab, otherwise
> > XGrabPointer may fail with AlreadyGrabbed, causing cursor clipping to fail.
> > Signed-off-by: Zhiyi Zhang
> >
> > dlls/winex11.drv/mouse.c | 21 ++++++++++++++++++---
> >   1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c
> > index f737a306a5..7501959369 100644
> > --- a/dlls/winex11.drv/mouse.c
> > +++ b/dlls/winex11.drv/mouse.c
> > @@ -378,9 +378,13 @@ static BOOL grab_clipping_window( const RECT *clip )
> >   {
> >       static const WCHAR messageW[] = {'M','e','s','s','a','g','e',0};
> >       struct x11drv_thread_data *data = x11drv_thread_data();
> > +    static DWORD timeout = 5000;
> > +    static DWORD step = 100;
> > +    DWORD time = 0;
> >       Window clip_window;
> >       HWND msg_hwnd = 0;
> >       POINT pos;
> > +    INT ret;
> >         if (GetWindowThreadProcessId( GetDesktopWindow(), NULL ) == GetCurrentThreadId())
> >           return TRUE;  /* don't clip in the desktop process */
> > @@ -416,13 +420,24 @@ static BOOL grab_clipping_window( const RECT *clip )
> >           clip->right < clip_rect.right || clip->bottom < clip_rect.bottom)
> >           data->warp_serial = NextRequest( data->display );
> >   -    if (!XGrabPointer( data->display, clip_window, False,
> > -                       PointerMotionMask | ButtonPressMask | ButtonReleaseMask,
> > -                       GrabModeAsync, GrabModeAsync, clip_window, None, CurrentTime ))
> > +    /* Some windows managers temporarily grab the pointer during window transition. Retry grabbing. */
> > +    do
> > +    {
> > +        ret = XGrabPointer( data->display, clip_window, False, PointerMotionMask | ButtonPressMask | ButtonReleaseMask,
> > +                            GrabModeAsync, GrabModeAsync, clip_window, None, CurrentTime );
> > +        if (ret == AlreadyGrabbed || ret == GrabFrozen)
> > +        {
> > +            time += step;
> > +            Sleep(step);
> > +        }
> > +    } while ((ret == AlreadyGrabbed || ret == GrabFrozen) && time < timeout);
> > -
> >
> > +    if (ret == GrabSuccess)
> >           clipping_cursor = TRUE;
> >         if (!clipping_cursor)
> >       {
> > +        ERR("Failed to grab pointer\n");
> >           disable_xinput2();
> >           DestroyWindow( msg_hwnd );
> >           return FALSE;
>
> I think this will not fix the issue in a reliable way. If the application fails to grab the cursor, it's usually because the WM is already grabbing it. The timeout makes the behavior arbitrarily differ when the user moves a window for more than 5s, although IMHO we should aim for consistent behavior - systematically fail to clip the cursor when the user moves the window for example, XOR retry indefinitely until it succeeds.
>
> Some games already retry clipping the cursor at every frame, so in this case they will eventually succeed. Some others don't and expect it to succeed, but for these I believe we should instead delay and retry to clip the cursor after the WM releases its grab.

Yes, with this it's still unreliable. It's supposed to apply with your patches as well, then it should be in much better shape.