[Spice-devel,spice-gtk,5/5] widget: differentiate key press & release from press only events

Submitted by Marc-André Lureau on Aug. 15, 2012, 7:59 p.m.

Details

Message ID 1345060770-29933-5-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marc-André Lureau Aug. 15, 2012, 7:59 p.m.
Until now, Spice clients only sent seperate key events for press and
release. But this may result in unwanted key repeatition from guest VM
side. It seems OSes have various implementation. While MS Windows rely
on hardware key repeats (which are several sequential press events),
otoh, X11 uses software key repeat (although not Linux keyboard/VT by
default).

We can't easily disable guest side repeaters, as it may be enforced by
other components (a X11 client can adjust each key individually, or
the desktop settings may change it etc.). Neither can we rely only on
guest software repeater as Windows doesn't seem to have one by
default, so we need to keep sending multiple press events as of today.

It seems a nice way to improve the situation is to send a single
"press&release" key event when the user released the key within a
short delay. If the key is pressed for longer, we keep the exisiting
behaviour which has been working pretty well so far, sending seperate
"press", then repeatedly "press", and an ending "release" event.
---
 gtk/spice-widget-priv.h |    2 ++
 gtk/spice-widget.c      |   76 +++++++++++++++++++++++++++++++++++++++++++----
 spice-common            |    2 +-
 3 files changed, 73 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/spice-widget-priv.h b/gtk/spice-widget-priv.h
index 97c6489..87d1de9 100644
--- a/gtk/spice-widget-priv.h
+++ b/gtk/spice-widget-priv.h
@@ -106,6 +106,8 @@  struct _SpiceDisplayPrivate {
     const guint16 const     *keycode_map;
     size_t                  keycode_maplen;
     uint32_t                key_state[512 / 32];
+    int                     key_delayed_scancode;
+    guint                   key_delayed_id;
     SpiceGrabSequence         *grabseq; /* the configured key sequence */
     gboolean                *activeseq; /* the currently pressed keys */
     gint                    mark;
diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
index 8cb7abb..d2b00f7 100644
--- a/gtk/spice-widget.c
+++ b/gtk/spice-widget.c
@@ -393,6 +393,11 @@  static void spice_display_dispose(GObject *obj)
     g_clear_object(&d->session);
     d->gtk_session = NULL;
 
+    if (d->key_delayed_id) {
+        g_source_remove(d->key_delayed_id);
+        d->key_delayed_id = 0;
+    }
+
     G_OBJECT_CLASS(spice_display_parent_class)->dispose(obj);
 }
 
@@ -994,6 +999,40 @@  typedef enum {
     SEND_KEY_RELEASE,
 } SendKeyType;
 
+static void key_press_and_release(SpiceDisplay *display)
+{
+    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
+
+    if (d->key_delayed_scancode == 0)
+        return;
+
+    spice_inputs_key_press_and_release(d->inputs, d->key_delayed_scancode);
+    d->key_delayed_scancode = 0;
+
+    if (d->key_delayed_id) {
+        g_source_remove(d->key_delayed_id);
+        d->key_delayed_id = 0;
+    }
+}
+
+static gboolean key_press_delayed(gpointer data)
+{
+    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(data);
+
+    if (d->key_delayed_scancode == 0)
+        goto end;
+
+    spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
+    d->key_delayed_scancode = 0;
+
+    if (d->key_delayed_id)
+        g_source_remove(d->key_delayed_id);
+
+end:
+    d->key_delayed_id = 0;
+    return FALSE;
+}
+
 static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboolean press_delayed)
 {
     SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
@@ -1010,15 +1049,40 @@  static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboo
     m = (1 << b);
     g_return_if_fail(i < SPICE_N_ELEMENTS(d->key_state));
 
-    if (down) {
-        spice_inputs_key_press(d->inputs, scancode);
+    switch (type) {
+    case SEND_KEY_PRESS:
+        /* ensure delayed key is pressed before any new input event */
+        key_press_delayed(display);
+
+        if (press_delayed &&
+            d->keypress_delay != 0 &&
+            !(d->key_state[i] & m)) {
+            g_warn_if_fail(d->key_delayed_id == 0);
+            d->key_delayed_id = g_timeout_add(d->keypress_delay, key_press_delayed, display);
+            d->key_delayed_scancode = scancode;
+        } else
+            spice_inputs_key_press(d->inputs, scancode);
+
         d->key_state[i] |= m;
-    } else {
-        if (!(d->key_state[i] & m)) {
-            return;
+        break;
+
+    case SEND_KEY_RELEASE:
+        if (!(d->key_state[i] & m))
+            break;
+
+        if (d->key_delayed_scancode == scancode)
+            key_press_and_release(display);
+        else {
+            /* ensure delayed key is pressed before other key are released */
+            key_press_delayed(display);
+            spice_inputs_key_release(d->inputs, scancode);
         }
-        spice_inputs_key_release(d->inputs, scancode);
+
         d->key_state[i] &= ~m;
+        break;
+
+    default:
+        g_warn_if_reached();
     }
 }
 
diff --git a/spice-common b/spice-common
index c2adbb0..31f1bff 160000
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@ 
-Subproject commit c2adbb00dc0b29de0fe297f241fb0efeb4a81510
+Subproject commit 31f1bff472da61ba07121ed31536c4af864c4a8f

Comments

I haven't looked at the patch, but the log has some typos:

On Wed, Aug 15, 2012 at 10:59:30PM +0300, Marc-André Lureau wrote:
> Until now, Spice clients only sent seperate key events for press and

separate

> release. But this may result in unwanted key repeatition from guest VM

repetition

> side. It seems OSes have various implementation. While MS Windows rely

implementations, relies

> on hardware key repeats (which are several sequential press events),
> otoh, X11 uses software key repeat (although not Linux keyboard/VT by

'repeats' maybe

> default).
> 
> We can't easily disable guest side repeaters, as it may be enforced by
> other components (a X11 client can adjust each key individually, or
> the desktop settings may change it etc.). Neither can we rely only on
> guest software repeater as Windows doesn't seem to have one by
> default, so we need to keep sending multiple press events as of today.
> 
> It seems a nice way to improve the situation is to send a single
> "press&release" key event when the user released the key within a
> short delay. If the key is pressed for longer, we keep the exisiting

existing

> behaviour which has been working pretty well so far, sending seperate

separate


Christophe

> "press", then repeatedly "press", and an ending "release" event.
> ---
>  gtk/spice-widget-priv.h |    2 ++
>  gtk/spice-widget.c      |   76 +++++++++++++++++++++++++++++++++++++++++++----
>  spice-common            |    2 +-
>  3 files changed, 73 insertions(+), 7 deletions(-)
> 
> diff --git a/gtk/spice-widget-priv.h b/gtk/spice-widget-priv.h
> index 97c6489..87d1de9 100644
> --- a/gtk/spice-widget-priv.h
> +++ b/gtk/spice-widget-priv.h
> @@ -106,6 +106,8 @@ struct _SpiceDisplayPrivate {
>      const guint16 const     *keycode_map;
>      size_t                  keycode_maplen;
>      uint32_t                key_state[512 / 32];
> +    int                     key_delayed_scancode;
> +    guint                   key_delayed_id;
>      SpiceGrabSequence         *grabseq; /* the configured key sequence */
>      gboolean                *activeseq; /* the currently pressed keys */
>      gint                    mark;
> diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> index 8cb7abb..d2b00f7 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -393,6 +393,11 @@ static void spice_display_dispose(GObject *obj)
>      g_clear_object(&d->session);
>      d->gtk_session = NULL;
>  
> +    if (d->key_delayed_id) {
> +        g_source_remove(d->key_delayed_id);
> +        d->key_delayed_id = 0;
> +    }
> +
>      G_OBJECT_CLASS(spice_display_parent_class)->dispose(obj);
>  }
>  
> @@ -994,6 +999,40 @@ typedef enum {
>      SEND_KEY_RELEASE,
>  } SendKeyType;
>  
> +static void key_press_and_release(SpiceDisplay *display)
> +{
> +    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> +
> +    if (d->key_delayed_scancode == 0)
> +        return;
> +
> +    spice_inputs_key_press_and_release(d->inputs, d->key_delayed_scancode);
> +    d->key_delayed_scancode = 0;
> +
> +    if (d->key_delayed_id) {
> +        g_source_remove(d->key_delayed_id);
> +        d->key_delayed_id = 0;
> +    }
> +}
> +
> +static gboolean key_press_delayed(gpointer data)
> +{
> +    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(data);
> +
> +    if (d->key_delayed_scancode == 0)
> +        goto end;
> +
> +    spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
> +    d->key_delayed_scancode = 0;
> +
> +    if (d->key_delayed_id)
> +        g_source_remove(d->key_delayed_id);
> +
> +end:
> +    d->key_delayed_id = 0;
> +    return FALSE;
> +}
> +
>  static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboolean press_delayed)
>  {
>      SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> @@ -1010,15 +1049,40 @@ static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboo
>      m = (1 << b);
>      g_return_if_fail(i < SPICE_N_ELEMENTS(d->key_state));
>  
> -    if (down) {
> -        spice_inputs_key_press(d->inputs, scancode);
> +    switch (type) {
> +    case SEND_KEY_PRESS:
> +        /* ensure delayed key is pressed before any new input event */
> +        key_press_delayed(display);
> +
> +        if (press_delayed &&
> +            d->keypress_delay != 0 &&
> +            !(d->key_state[i] & m)) {
> +            g_warn_if_fail(d->key_delayed_id == 0);
> +            d->key_delayed_id = g_timeout_add(d->keypress_delay, key_press_delayed, display);
> +            d->key_delayed_scancode = scancode;
> +        } else
> +            spice_inputs_key_press(d->inputs, scancode);
> +
>          d->key_state[i] |= m;
> -    } else {
> -        if (!(d->key_state[i] & m)) {
> -            return;
> +        break;
> +
> +    case SEND_KEY_RELEASE:
> +        if (!(d->key_state[i] & m))
> +            break;
> +
> +        if (d->key_delayed_scancode == scancode)
> +            key_press_and_release(display);
> +        else {
> +            /* ensure delayed key is pressed before other key are released */
> +            key_press_delayed(display);
> +            spice_inputs_key_release(d->inputs, scancode);
>          }
> -        spice_inputs_key_release(d->inputs, scancode);
> +
>          d->key_state[i] &= ~m;
> +        break;
> +
> +    default:
> +        g_warn_if_reached();
>      }
>  }
>  
> diff --git a/spice-common b/spice-common
> index c2adbb0..31f1bff 160000
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit c2adbb00dc0b29de0fe297f241fb0efeb4a81510
> +Subproject commit 31f1bff472da61ba07121ed31536c4af864c4a8f
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On 08/15/2012 09:59 PM, Marc-André Lureau wrote:
> Until now, Spice clients only sent seperate key events for press and
> release. But this may result in unwanted key repeatition from guest VM
> side. It seems OSes have various implementation. While MS Windows rely
> on hardware key repeats (which are several sequential press events),
> otoh, X11 uses software key repeat (although not Linux keyboard/VT by
> default).
>
> We can't easily disable guest side repeaters, as it may be enforced by
> other components (a X11 client can adjust each key individually, or
> the desktop settings may change it etc.). Neither can we rely only on
> guest software repeater as Windows doesn't seem to have one by
> default, so we need to keep sending multiple press events as of today.
>
> It seems a nice way to improve the situation is to send a single
> "press&release" key event when the user released the key within a
> short delay. If the key is pressed for longer, we keep the exisiting
> behaviour which has been working pretty well so far, sending seperate
> "press", then repeatedly "press", and an ending "release" event.
> ---
>   gtk/spice-widget-priv.h |    2 ++
>   gtk/spice-widget.c      |   76 +++++++++++++++++++++++++++++++++++++++++++----
>   spice-common            |    2 +-
>   3 files changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/gtk/spice-widget-priv.h b/gtk/spice-widget-priv.h
> index 97c6489..87d1de9 100644
> --- a/gtk/spice-widget-priv.h
> +++ b/gtk/spice-widget-priv.h
> @@ -106,6 +106,8 @@ struct _SpiceDisplayPrivate {
>       const guint16 const     *keycode_map;
>       size_t                  keycode_maplen;
>       uint32_t                key_state[512 / 32];
> +    int                     key_delayed_scancode;
> +    guint                   key_delayed_id;
>       SpiceGrabSequence         *grabseq; /* the configured key sequence */
>       gboolean                *activeseq; /* the currently pressed keys */
>       gint                    mark;
> diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> index 8cb7abb..d2b00f7 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -393,6 +393,11 @@ static void spice_display_dispose(GObject *obj)
>       g_clear_object(&d->session);
>       d->gtk_session = NULL;
>
> +    if (d->key_delayed_id) {
> +        g_source_remove(d->key_delayed_id);
> +        d->key_delayed_id = 0;
> +    }
> +
>       G_OBJECT_CLASS(spice_display_parent_class)->dispose(obj);
>   }
>
> @@ -994,6 +999,40 @@ typedef enum {
>       SEND_KEY_RELEASE,
>   } SendKeyType;
>
> +static void key_press_and_release(SpiceDisplay *display)
> +{
> +    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> +
> +    if (d->key_delayed_scancode == 0)
> +        return;
> +
> +    spice_inputs_key_press_and_release(d->inputs, d->key_delayed_scancode);
> +    d->key_delayed_scancode = 0;
> +
> +    if (d->key_delayed_id) {
> +        g_source_remove(d->key_delayed_id);
> +        d->key_delayed_id = 0;
> +    }
> +}
> +
> +static gboolean key_press_delayed(gpointer data)
> +{
> +    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(data);
> +
> +    if (d->key_delayed_scancode == 0)
> +        goto end;

At "end" you set d->key_delayed_id to 0 without removing it, in essence assuming
that if d->key_delayed_scancode == 0  no timer will be running. You make the same
(correct) assumption in key_press_and_release above. I'm fine with the assumption,
but then please make the code identical as above, iow replace "goto end;" with
"return FALSE;" and ...

> +
> +    spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
> +    d->key_delayed_scancode = 0;
> +
> +    if (d->key_delayed_id)
> +        g_source_remove(d->key_delayed_id);
> +
> +end:
> +    d->key_delayed_id = 0;

make this bit:

     if (d->key_delayed_id) {
         g_source_remove(d->key_delayed_id);
         d->key_delayed_id = 0;
     }


> +    return FALSE;
> +}
> +
>   static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboolean press_delayed)
>   {
>       SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> @@ -1010,15 +1049,40 @@ static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboo
>       m = (1 << b);
>       g_return_if_fail(i < SPICE_N_ELEMENTS(d->key_state));
>
> -    if (down) {
> -        spice_inputs_key_press(d->inputs, scancode);

Hmm, the replacement of "if (down)" with the switch case should have been done in the previous patch,
so un-ack the previous one, as this way we will have a commit in git which does not compile.

> +    switch (type) {
> +    case SEND_KEY_PRESS:
> +        /* ensure delayed key is pressed before any new input event */
> +        key_press_delayed(display);
> +
> +        if (press_delayed &&
> +            d->keypress_delay != 0 &&
> +            !(d->key_state[i] & m)) {
> +            g_warn_if_fail(d->key_delayed_id == 0);
> +            d->key_delayed_id = g_timeout_add(d->keypress_delay, key_press_delayed, display);
> +            d->key_delayed_scancode = scancode;
> +        } else
> +            spice_inputs_key_press(d->inputs, scancode);
> +
>           d->key_state[i] |= m;
> -    } else {
> -        if (!(d->key_state[i] & m)) {
> -            return;
> +        break;
> +
> +    case SEND_KEY_RELEASE:
> +        if (!(d->key_state[i] & m))
> +            break;
> +
> +        if (d->key_delayed_scancode == scancode)
> +            key_press_and_release(display);
> +        else {
> +            /* ensure delayed key is pressed before other key are released */
> +            key_press_delayed(display);
> +            spice_inputs_key_release(d->inputs, scancode);
>           }
> -        spice_inputs_key_release(d->inputs, scancode);
> +
>           d->key_state[i] &= ~m;
> +        break;
> +
> +    default:
> +        g_warn_if_reached();
>       }
>   }
>
> diff --git a/spice-common b/spice-common
> index c2adbb0..31f1bff 160000
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit c2adbb00dc0b29de0fe297f241fb0efeb4a81510
> +Subproject commit 31f1bff472da61ba07121ed31536c4af864c4a8f

I assume this is to update protocol to get the new SPICE_MSGC_INPUTS_KEY_SCANCODE
define. If so then this should be part of patch 2/5 not 5/5

Regards,

Hans