[PATCH xserver v2] xwayland: Apply "last pointer window" check only to the pointer device

[Olivier Fourdan]

Hi Carlos,

> The checks in xwayland's XYToWindow handler pretty much assumes that the
> sprite is managed by the wl_pointer, which is not entirely right, given
> 1) The Virtual Core Pointer may be controlled from other interfaces, and
> 2) there may be other SpriteRecs than the VCP's.
> 
> This makes XYToWindow calls return a sprite trace with just the root
> window if any of those two assumptions are broken, eg. on touch events.
> 
> So turn the check upside down, first assume that the default XYToWindow
> proc behavior is right, and later cut down the spriteTrace if the
> current device happens to be the pointer and is out of focus. We work
> our way to the device's lastSlave here so as not to break assumption #1
> above.
> 
> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
> ---
>  hw/xwayland/xwayland-input.c | 59
>  ++++++++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 32cfb35..029ae04 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -945,44 +945,65 @@ DDXRingBell(int volume, int pitch, int duration)
>  {
>  }
>  
> -static WindowPtr
> -xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +static Bool
> +sprite_check_lost_focus(SpritePtr sprite, WindowPtr window)
>  {
> -    struct xwl_seat *xwl_seat = NULL;
> -    DeviceIntPtr device;
> -    WindowPtr ret;
> +    DeviceIntPtr device, master;
> +    struct xwl_seat *xwl_seat;
>  
>      for (device = inputInfo.devices; device; device = device->next) {
> +        /* Ignore non-wayland devices */
>          if (device->deviceProc == xwl_pointer_proc &&
> -            device->spriteInfo->sprite == sprite) {
> -            xwl_seat = device->public.devicePrivate;
> +            device->spriteInfo->sprite == sprite)
>              break;
> -        }
>      }
>  
> -    if (xwl_seat == NULL) {
> -        sprite->spriteTraceGood = 1;
> -        return sprite->spriteTrace[0];
> -    }
> +    if (!device)
> +        return FALSE;
> +
> +    xwl_seat = device->public.devicePrivate;
> +
> +    master = GetMaster(device, POINTER_OR_FLOAT);
> +    if (!master || !master->lastSlave)
> +        return FALSE;
>  
> -    screen->XYToWindow = xwl_seat->xwl_screen->XYToWindow;
> +    /* We do want the last active slave, we only check on slave xwayland
> devices
> +     * so we can find out the xwl_seat, but those don't actually own their
> +     * sprite, so the match doesn't mean a lot.
> +     */
> +    if (master->lastSlave == xwl_seat->pointer &&
> +        xwl_seat->focus_window == NULL &&
> +        xwl_seat->last_xwindow == window)
> +        return TRUE;
> +
> +    xwl_seat->last_xwindow = window;
> +    return FALSE;
> +}
> +
> +static WindowPtr
> +xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +{
> +    struct xwl_screen *xwl_screen;
> +    WindowPtr ret;
> +
> +    xwl_screen = xwl_screen_get(screen);
> +
> +    screen->XYToWindow = xwl_screen->XYToWindow;
>      ret = screen->XYToWindow(screen, sprite, x, y);
> -    xwl_seat->xwl_screen->XYToWindow = screen->XYToWindow;
> +    xwl_screen->XYToWindow = screen->XYToWindow;
>      screen->XYToWindow = xwl_xy_to_window;
>  
> -    /* If the pointer has left the Wayland surface but the DIX still
> -     * finds the pointer within the previous X11 window, it means that
> +    /* If the device controlling the sprite has left the Wayland surface but
> +     * the DIX still finds the pointer within the X11 window, it means that
>       * the pointer has crossed to another native Wayland window, in this
>       * case, pretend we entered the root window so that a LeaveNotify
>       * event is emitted.
>       */
> -    if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret) {
> +    if (sprite_check_lost_focus(sprite, ret)) {
>          sprite->spriteTraceGood = 1;
>          return sprite->spriteTrace[0];
>      }
>  
> -    xwl_seat->last_xwindow = ret;
> -
>      return ret;
>  }

It just feels odd that the function is called sprite_check_lost_focus() and updates the xwl_seat's last_xwindow value.

I am no input expert so maybe you'll want to wait for someone else's better review, but as far as I can see it looks good to me, so:

Acked-by: Olivier Fourdan <ofourdan at redhat.com>

Cheers,
Olivier