Re: [PATCH xserver 2/4 v3] xwayland: add a server sync before repeating keys

[Olivier Fourdan]

Hi

- Original Message -
> > On 9 March 2016 at 09:31, Olivier Fourdan  wrote:
> > > @@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat
> > > *seat,
> > >  ActivateDevice(xwl_seat->keyboard, TRUE);
> > >  }
> > >  EnableDevice(xwl_seat->keyboard, TRUE);
> > > +master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
> > > +if (master)
> > > +master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
> > 
> > This needs to be done on xwl_seat->keyboard as well: both devices
> > generate events. You can see this with 'xinput test'.
> 
> keyboard_check_repeat() operates on the xwl_seat itself, so I reckon it works
> for both devices.
> 
> If I explicitely set the callback function for both devices as you suggest,
> key repeat becomes completely erratic as both devices share the same
> xwl_seat, so doing it on the master device alone seems to work better.

Sorry for the delay...

My problem was that each device is attached to an xwl_seat, bit xwl_seats do 
have multiple devices, including multiple keyboards.

With your example, both "xwayland-keyboard" and "Virtual core XTEST keyboard" 
belong to the same xwl_seat, so using the same bool flag for both would 
obviously not work with my previous implementation, so it needed a bit more 
surgery^W rework...

The "public.devicePrivate" is used to point to the xwl_seat so we cannot use 
that to store the "pending sync" flag. The idea is to add a list of devices 
with pending sync to the wl_seat, and check if a given device is among that 
list, and remove it once we receive the callback notification for that device.

I will be sending an updated series (4 patches, so it will be easier to review 
at once) with this implementation in a second.

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/4 v3] xwayland: add a server sync before repeating keys

[Olivier Fourdan]

Hi Daniel,

Thanks for the feedback!

- Original Message -
> Hi,
> 
> On 9 March 2016 at 09:31, Olivier Fourdan  wrote:
> > @@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat
> > *seat,
> >  ActivateDevice(xwl_seat->keyboard, TRUE);
> >  }
> >  EnableDevice(xwl_seat->keyboard, TRUE);
> > +master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
> > +if (master)
> > +master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
> 
> This needs to be done on xwl_seat->keyboard as well: both devices
> generate events. You can see this with 'xinput test'.

keyboard_check_repeat() operates on the xwl_seat itself, so I reckon it works 
for both devices.

If I explicitely set the callback function for both devices as you suggest, key 
repeat becomes completely erratic as both devices share the same xwl_seat, so 
doing it on the master device alone seems to work better.

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/4 v3] xwayland: add a server sync before repeating keys

[Daniel Stone]

Hi,

On 9 March 2016 at 09:31, Olivier Fourdan  wrote:
> @@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
>  ActivateDevice(xwl_seat->keyboard, TRUE);
>  }
>  EnableDevice(xwl_seat->keyboard, TRUE);
> +master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
> +if (master)
> +master->key->xkbInfo->checkRepeat = keyboard_check_repeat;

This needs to be done on xwl_seat->keyboard as well: both devices
generate events. You can see this with 'xinput test'.

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 2/4 v3] xwayland: add a server sync before repeating keys

[Olivier Fourdan]

Key repeat is handled by the X server, but input events need to be
processed and forwarded by the Wayland compositor first.

Make sure the Wayland compositor is actually processing events, to
avoid repeating keys in Xwayland while the Wayland compositor cannot
deal with input events for whatever reason, thus not dispatching key
release events, leading to repeated keys while the user has already
released the key.

Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=762618
Signed-off-by: Olivier Fourdan 
---
 v2: A slightly less hack-y-ish version of the patch
 Use xwl_seat to store the pending sync, set up callback fpr keyboard
 in seat_handle_capabilities() and add a call to _dispatch_pending()
 in the callback function to make sure we didn't miss a reply
 v3: Small clean-up,
 Remove the call to wl_display_dispatch_pending() that we added
 in v2 as we shall do it in two other separate patches to follow.

 hw/xwayland/xwayland-input.c | 38 ++
 hw/xwayland/xwayland.h   |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index f9e3255..28d8b54 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -527,6 +527,40 @@ keyboard_handle_modifiers(void *data, struct wl_keyboard 
*keyboard,
 }
 
 static void
+sync_callback(void *data, struct wl_callback *callback, uint32_t serial)
+{
+struct xwl_seat *xwl_seat = data;
+
+xwl_seat->sync_pending = FALSE;
+wl_callback_destroy(callback);
+}
+
+static const struct wl_callback_listener sync_listener = {
+   sync_callback
+};
+
+static Bool
+keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr xkbi, unsigned key)
+{
+struct xwl_seat *xwl_seat = dev->public.devicePrivate;
+struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;
+struct wl_callback *callback;
+
+if (!xwl_seat->sync_pending) {
+callback = wl_display_sync (xwl_screen->display);
+wl_callback_add_listener(callback, _listener, xwl_seat);
+xwl_seat->sync_pending = TRUE;
+
+return TRUE;
+}
+
+ErrorF("Key repeat discarded, Wayland compositor doesn't seem to "
+   "be processing events fast enough!\n");
+
+return FALSE;
+}
+
+static void
 keyboard_handle_repeat_info (void *data, struct wl_keyboard *keyboard,
  int32_t rate, int32_t delay)
 {
@@ -716,6 +750,7 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
  enum wl_seat_capability caps)
 {
 struct xwl_seat *xwl_seat = data;
+DeviceIntPtr master;
 
 if (caps & WL_SEAT_CAPABILITY_POINTER && xwl_seat->wl_pointer == NULL) {
 xwl_seat->wl_pointer = wl_seat_get_pointer(seat);
@@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
 ActivateDevice(xwl_seat->keyboard, TRUE);
 }
 EnableDevice(xwl_seat->keyboard, TRUE);
+master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
+if (master)
+master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
 } else if (!(caps & WL_SEAT_CAPABILITY_KEYBOARD) && xwl_seat->wl_keyboard) 
{
 wl_keyboard_release(xwl_seat->wl_keyboard);
 xwl_seat->wl_keyboard = NULL;
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 2a54183..aaf3b40 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -138,6 +138,8 @@ struct xwl_seat {
 size_t keymap_size;
 char *keymap;
 struct wl_surface *keyboard_focus;
+
+Bool sync_pending;
 };
 
 struct xwl_output {
-- 
2.5.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel