[Spice-devel,spice-gtk] RFC: widget: improve undesired key repeatition

Submitted by Marc-André Lureau on April 19, 2012, 1:17 a.m.

Details

Message ID 1334798235-9853-1-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marc-André Lureau April 19, 2012, 1:17 a.m.
The remote end receives key press and key release seperately, but this
can cause extra key repetition between the two event messages:
- if the network is slow
- if the server is busy and doesn't handle the events quickly enough
- if the client is slow or busy

(I think the guest/os couldn't be responsible, otherwise any computer
would be affected when it hangs a little, although I might be wrong, it
could be the events are queued and software properly check state before
repeating)

The client should send a single message MsgcDownUp instead.
This patch prepares for this change, and serves for discussion before
addition of new message/capability:
- delay non-modifier key press event until the event is repeated
- send extra Down & Up event when the key is released

The minor downside of this approach is that it accumulates the initial delay
before repeat. If locally and remotely, we have 400ms before repeat, the
actual repeat on remote will happen after 800ms. There is no easy
solution for that, we could lower the initial delay to decide between
DownUp and Down, probably 100ms would be enough. It could
enable this distinction for remote servers only. Finally the remote
repeat delay could be tweaked via the agent.

This improves the situation for me with remote servers, I no longer get
spurious key repeat, although it might still happen, but less often.
Depending on feeback, I'll follow up with MsgcDownUp addition.
---
 gtk/spice-widget-priv.h |    1 +
 gtk/spice-widget.c      |   59 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 47 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/spice-widget-priv.h b/gtk/spice-widget-priv.h
index 898d86c..dfc9010 100644
--- a/gtk/spice-widget-priv.h
+++ b/gtk/spice-widget-priv.h
@@ -101,6 +101,7 @@  struct _SpiceDisplayPrivate {
     const guint16 const     *keycode_map;
     size_t                  keycode_maplen;
     uint32_t                key_state[512 / 32];
+    int                     key_delayed_scancode;
     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 d355f09..9403513 100644
--- a/gtk/spice-widget.c
+++ b/gtk/spice-widget.c
@@ -888,8 +888,12 @@  static gboolean expose_event(GtkWidget *widget, GdkEventExpose *expose)
 #endif
 
 /* ---------------------------------------------------------------- */
+typedef enum {
+    SEND_KEY_PRESS,
+    SEND_KEY_RELEASE,
+} SendKeyType;
 
-static void send_key(SpiceDisplay *display, int scancode, int down)
+static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboolean press_delayed)
 {
     SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
     uint32_t i, b, m;
@@ -905,15 +909,43 @@  static void send_key(SpiceDisplay *display, int scancode, int down)
     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:
+        if (d->key_delayed_scancode) {
+            /* ensure delayed key is pressed before any new input event */
+            spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
+            d->key_delayed_scancode = 0;
+        }
+
+        /* let the guest handle key repeatition */
+        if (d->key_state[i] & m)
+            break;
+
+        if (press_delayed)
+            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) {
+            /* FIXME: should send a single PRESS & RELEASE message
+             if key_delayed_scancode == scancode */
+            spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
+            d->key_delayed_scancode = 0;
         }
+
         spice_inputs_key_release(d->inputs, scancode);
         d->key_state[i] &= ~m;
+        break;
+
+    default:
+        g_warn_if_reached();
     }
 }
 
@@ -928,7 +960,7 @@  static void release_keys(SpiceDisplay *display)
             continue;
         }
         for (b = 0; b < 32; b++) {
-            send_key(display, i * 32 + b, 0);
+            send_key(display, i * 32 + b, SEND_KEY_RELEASE, FALSE);
         }
     }
 }
@@ -966,9 +998,10 @@  static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
     SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
     int scancode;
 
-    SPICE_DEBUG("%s %s: keycode: %d  state: %d  group %d",
+
+    SPICE_DEBUG("%s %s: keycode: %d  state: %d  group %d modifier %d",
             __FUNCTION__, key->type == GDK_KEY_PRESS ? "press" : "release",
-            key->hardware_keycode, key->state, key->group);
+            key->hardware_keycode, key->state, key->group, key->is_modifier);
 
     if (check_for_grab_key(display, key->type, key->keyval)) {
         g_signal_emit(widget, signals[SPICE_DISPLAY_GRAB_KEY_PRESSED], 0);
@@ -990,10 +1023,10 @@  static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
                                             key->hardware_keycode);
     switch (key->type) {
     case GDK_KEY_PRESS:
-        send_key(display, scancode, 1);
+        send_key(display, scancode, SEND_KEY_PRESS, !key->is_modifier);
         break;
     case GDK_KEY_RELEASE:
-        send_key(display, scancode, 0);
+        send_key(display, scancode, SEND_KEY_RELEASE, !key->is_modifier);
         break;
     default:
         break;
@@ -1031,12 +1064,12 @@  void spice_display_send_keys(SpiceDisplay *display, const guint *keyvals,
 
     if (kind & SPICE_DISPLAY_KEY_EVENT_PRESS) {
         for (i = 0 ; i < nkeyvals ; i++)
-            send_key(display, get_scancode_from_keyval(display, keyvals[i]), 1);
+            send_key(display, get_scancode_from_keyval(display, keyvals[i]), SEND_KEY_PRESS, FALSE);
     }
 
     if (kind & SPICE_DISPLAY_KEY_EVENT_RELEASE) {
         for (i = (nkeyvals-1) ; i >= 0 ; i--)
-            send_key(display, get_scancode_from_keyval(display, keyvals[i]), 0);
+            send_key(display, get_scancode_from_keyval(display, keyvals[i]), SEND_KEY_RELEASE, FALSE);
     }
 }
 

Comments

On Thu, Apr 19, 2012 at 03:17:15AM +0200, Marc-André Lureau wrote:
> The remote end receives key press and key release seperately, but this
> can cause extra key repetition between the two event messages:
> - if the network is slow
> - if the server is busy and doesn't handle the events quickly enough
> - if the client is slow or busy
> 
> (I think the guest/os couldn't be responsible, otherwise any computer
> would be affected when it hangs a little, although I might be wrong, it
> could be the events are queued and software properly check state before
> repeating)
> 
> The client should send a single message MsgcDownUp instead.
> This patch prepares for this change, and serves for discussion before
> addition of new message/capability:
> - delay non-modifier key press event until the event is repeated
> - send extra Down & Up event when the key is released
> 
> The minor downside of this approach is that it accumulates the initial delay
> before repeat. If locally and remotely, we have 400ms before repeat, the
> actual repeat on remote will happen after 800ms. There is no easy
> solution for that, we could lower the initial delay to decide between
> DownUp and Down, probably 100ms would be enough. It could
> enable this distinction for remote servers only. Finally the remote
> repeat delay could be tweaked via the agent.
> 
> This improves the situation for me with remote servers, I no longer get
> spurious key repeat, although it might still happen, but less often.
> Depending on feeback, I'll follow up with MsgcDownUp addition.

This sounds like a great idea. I think it should be configurable
though, since for games for instance (yes, yes, I know no one uses spice
for games) or for things like shift-click you would want the key press
to be sent separately. I personally in linux guests turned off key
repeat via xset, that worked for me, but I think your solution is better
since it is guest agnostic, just with an ability to change the behavior
somehow. But we don't want the user to have to decide, so I'm not sure
what the right way is - but enabling this globally is not good enough
for the reasons I pointed out - you want separate events sometimes.
Maybe have the client decide based on qos?

> ---
>  gtk/spice-widget-priv.h |    1 +
>  gtk/spice-widget.c      |   59 ++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/gtk/spice-widget-priv.h b/gtk/spice-widget-priv.h
> index 898d86c..dfc9010 100644
> --- a/gtk/spice-widget-priv.h
> +++ b/gtk/spice-widget-priv.h
> @@ -101,6 +101,7 @@ struct _SpiceDisplayPrivate {
>      const guint16 const     *keycode_map;
>      size_t                  keycode_maplen;
>      uint32_t                key_state[512 / 32];
> +    int                     key_delayed_scancode;
>      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 d355f09..9403513 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -888,8 +888,12 @@ static gboolean expose_event(GtkWidget *widget, GdkEventExpose *expose)
>  #endif
>  
>  /* ---------------------------------------------------------------- */
> +typedef enum {
> +    SEND_KEY_PRESS,
> +    SEND_KEY_RELEASE,
> +} SendKeyType;
>  
> -static void send_key(SpiceDisplay *display, int scancode, int down)
> +static void send_key(SpiceDisplay *display, int scancode, SendKeyType type, gboolean press_delayed)
>  {
>      SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
>      uint32_t i, b, m;
> @@ -905,15 +909,43 @@ static void send_key(SpiceDisplay *display, int scancode, int down)
>      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:
> +        if (d->key_delayed_scancode) {
> +            /* ensure delayed key is pressed before any new input event */
> +            spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
> +            d->key_delayed_scancode = 0;
> +        }
> +
> +        /* let the guest handle key repeatition */
> +        if (d->key_state[i] & m)
> +            break;
> +
> +        if (press_delayed)
> +            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) {
> +            /* FIXME: should send a single PRESS & RELEASE message
> +             if key_delayed_scancode == scancode */
> +            spice_inputs_key_press(d->inputs, d->key_delayed_scancode);
> +            d->key_delayed_scancode = 0;
>          }
> +
>          spice_inputs_key_release(d->inputs, scancode);
>          d->key_state[i] &= ~m;
> +        break;
> +
> +    default:
> +        g_warn_if_reached();
>      }
>  }
>  
> @@ -928,7 +960,7 @@ static void release_keys(SpiceDisplay *display)
>              continue;
>          }
>          for (b = 0; b < 32; b++) {
> -            send_key(display, i * 32 + b, 0);
> +            send_key(display, i * 32 + b, SEND_KEY_RELEASE, FALSE);
>          }
>      }
>  }
> @@ -966,9 +998,10 @@ static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
>      SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
>      int scancode;
>  
> -    SPICE_DEBUG("%s %s: keycode: %d  state: %d  group %d",
> +
> +    SPICE_DEBUG("%s %s: keycode: %d  state: %d  group %d modifier %d",
>              __FUNCTION__, key->type == GDK_KEY_PRESS ? "press" : "release",
> -            key->hardware_keycode, key->state, key->group);
> +            key->hardware_keycode, key->state, key->group, key->is_modifier);
>  
>      if (check_for_grab_key(display, key->type, key->keyval)) {
>          g_signal_emit(widget, signals[SPICE_DISPLAY_GRAB_KEY_PRESSED], 0);
> @@ -990,10 +1023,10 @@ static gboolean key_event(GtkWidget *widget, GdkEventKey *key)
>                                              key->hardware_keycode);
>      switch (key->type) {
>      case GDK_KEY_PRESS:
> -        send_key(display, scancode, 1);
> +        send_key(display, scancode, SEND_KEY_PRESS, !key->is_modifier);
>          break;
>      case GDK_KEY_RELEASE:
> -        send_key(display, scancode, 0);
> +        send_key(display, scancode, SEND_KEY_RELEASE, !key->is_modifier);
>          break;
>      default:
>          break;
> @@ -1031,12 +1064,12 @@ void spice_display_send_keys(SpiceDisplay *display, const guint *keyvals,
>  
>      if (kind & SPICE_DISPLAY_KEY_EVENT_PRESS) {
>          for (i = 0 ; i < nkeyvals ; i++)
> -            send_key(display, get_scancode_from_keyval(display, keyvals[i]), 1);
> +            send_key(display, get_scancode_from_keyval(display, keyvals[i]), SEND_KEY_PRESS, FALSE);
>      }
>  
>      if (kind & SPICE_DISPLAY_KEY_EVENT_RELEASE) {
>          for (i = (nkeyvals-1) ; i >= 0 ; i--)
> -            send_key(display, get_scancode_from_keyval(display, keyvals[i]), 0);
> +            send_key(display, get_scancode_from_keyval(display, keyvals[i]), SEND_KEY_RELEASE, FALSE);
>      }
>  }
>  
> -- 
> 1.7.10
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On 04/19/2012 03:17 AM, Marc-André Lureau wrote:
> The remote end receives key press and key release seperately, but this
> can cause extra key repetition between the two event messages:
> - if the network is slow
> - if the server is busy and doesn't handle the events quickly enough
> - if the client is slow or busy
>
> (I think the guest/os couldn't be responsible, otherwise any computer
> would be affected when it hangs a little, although I might be wrong, it
> could be the events are queued and software properly check state before
> repeating)
>
> The client should send a single message MsgcDownUp instead.
> This patch prepares for this change, and serves for discussion before
> addition of new message/capability:
> - delay non-modifier key press event until the event is repeated
> - send extra Down&  Up event when the key is released
>
> The minor downside of this approach is that it accumulates the initial delay
> before repeat. If locally and remotely, we have 400ms before repeat, the
> actual repeat on remote will happen after 800ms. There is no easy
> solution for that, we could lower the initial delay to decide between
> DownUp and Down, probably 100ms would be enough. It could
> enable this distinction for remote servers only. Finally the remote
> repeat delay could be tweaked via the agent.
>
> This improves the situation for me with remote servers, I no longer get
> spurious key repeat, although it might still happen, but less often.
> Depending on feeback, I'll follow up with MsgcDownUp addition.

This surely is an interesting patch :) As such I've various remarks:

1) If we move ahead with this approach, there is 1 bug in there which should
be fixed, the code after the patch depends on the client having keyrepeat
configured, so if a user dislikes key-repeat and has it disabled, things will
break.

Instead you should start a timer when as delayed keypress is registered and
after say 100ms send the keypress. This also avoids doubling the keyrepeat
delay.


2) This patch does 2 things:

a) It (tries to) stop sending keyboard events for client side key-repeat
generated events, to let the guest handle key-repeat

b) It tries to avoid undesired guest side generated key repetition

I believe these are 2 separate issues, and this should be done in 2 separate
patches. Note that the patch does not *actually* fix a. It tries to, but it
fails, since on key-repeat first a KeyRelease event will be send, immediately
followed by a KeyPress, so the new check on press:
+        /* let the guest handle key repeatition */
+        if (d->key_state[i] & m)
+            break;

Won't trigger because of the release being send first clearing the key_state
entry. Note I'm basing this on experience with X11, it may be that gtk somehow
handles key-repeats specially and filters out the release event for repeats?

Also wrt a), this actually is a regression compared to spicec, which has
this "gem" to filter out repeats, based on the knowledge that for repeat
events you get a release + press immediately following each other and with
the same timestamp (disclaimer I did not write this "gem"):

         XEvent next_event;
         Bool check = XCheckWindowEvent(x_display, red_window->_win, ~long(0), &next_event);
         if (check) {
             XPutBackEvent(x_display, &next_event);
             if ((next_event.type == KeyPress) &&
                 (event.xkey.keycode == next_event.xkey.keycode) &&
                 (event.xkey.time == next_event.xkey.time)) {
                 break;
             }
         }

So I suggest splitting this patch into 2, and independent of the discussion
wrt b) fix a) asap.


3) As Alon already mentioned the initial delay is undesirable in some cases,
so we should make it configurable, if you fix 1) by starting a timer on
key-press-delay, then you could make the time configurable, with 0 meaning
disable key-press-delay.


4) As Alon already said we could just disable key-repeat on the guest. We
could have the agent do this and also add an agent capability for this, and then
of both client and agent support it, have the agent disable key-repeat on the
guest, and not ignore key-repeat on the client. Note that this will make some
apps unhappy which don't want key-repeat, and try to disable it on the guest, ie
a spice-client running inside a spice-displayed-vm...


Note I think that your fix is better then what I suggest in 4, but we do really
need to make the delay configurable and not rely on key-repeat being enabled on
the client.

Regards,

Hans
Hi

Now that I implemented it a bit more correctly, using a timer, and
splitting the in two patches, I realize that it doesn't work with
windows. Windows expect to receive several key press events
apparently. Hans, the hack you mentionned in spicec doesn't seem to be
what you said, since spicec client repeatly sends down messages when
the key is pressed (btw, Gtk doesn't behave the way you described
either, so this previous patch was working fine)

So it seems like we should keep sending repeat key press in fact. And
it's probably better to rely on client-side repeat, since letting the
server handling it may lead to precisely the bug I was trying to fix.
And we should teach the agent to disable the key repeat function,
which is a trival xset -r, or XGetKeyboardControl /
XChangeKeyboardControl equivalent when a client is connected. Any
other opinion?
On Fri, Apr 20, 2012 at 01:59:05AM +0200, Marc-André Lureau wrote:
> Hi
> 
> Now that I implemented it a bit more correctly, using a timer, and
> splitting the in two patches, I realize that it doesn't work with
> windows. Windows expect to receive several key press events
> apparently. Hans, the hack you mentionned in spicec doesn't seem to be
> what you said, since spicec client repeatly sends down messages when
> the key is pressed (btw, Gtk doesn't behave the way you described
> either, so this previous patch was working fine)
> 
> So it seems like we should keep sending repeat key press in fact. And
> it's probably better to rely on client-side repeat, since letting the
> server handling it may lead to precisely the bug I was trying to fix.
> And we should teach the agent to disable the key repeat function,
> which is a trival xset -r, or XGetKeyboardControl /
> XChangeKeyboardControl equivalent when a client is connected. Any
> other opinion?

Using the agent to disable key repeat sounds fine, but you want to do
that for windows as well.

Just to be clear, you propose the client generates the repeat, and the
agent disable the guest repeat, so if there is jitter in the network the
repeated key press events as seen by the guest would have differing time
difference.

> 
> -- 
> Marc-André Lureau
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi

On Sun, Apr 22, 2012 at 9:40 AM, Alon Levy <alevy@redhat.com> wrote:
> On Fri, Apr 20, 2012 at 01:59:05AM +0200, Marc-André Lureau wrote:
>> So it seems like we should keep sending repeat key press in fact. And
>> it's probably better to rely on client-side repeat, since letting the
>> server handling it may lead to precisely the bug I was trying to fix.
>> And we should teach the agent to disable the key repeat function,
>> which is a trival xset -r, or XGetKeyboardControl /
>> XChangeKeyboardControl equivalent when a client is connected. Any
>> other opinion?
>
> Using the agent to disable key repeat sounds fine, but you want to do
> that for windows as well.

Windows doesn't need change, since it seems to use "hardware" key repeat.

> Just to be clear, you propose the client generates the repeat, and the
> agent disable the guest repeat, so if there is jitter in the network the
> repeated key press events as seen by the guest would have differing time
> difference.

Not sure what you mean by "differing time difference", but somehow it
should be under client control whether there is key repeat.

Hans gave a longer reply, but unfortunately, it was only addressed to
me. Hans can you resend?

The interesting is that disabling x11 key repeat will not work with
only Down...Down events.. It will need Down/Up (press-release).
On Sun, Apr 22, 2012 at 03:16:20PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Sun, Apr 22, 2012 at 9:40 AM, Alon Levy <alevy@redhat.com> wrote:
> > On Fri, Apr 20, 2012 at 01:59:05AM +0200, Marc-André Lureau wrote:
> >> So it seems like we should keep sending repeat key press in fact. And
> >> it's probably better to rely on client-side repeat, since letting the
> >> server handling it may lead to precisely the bug I was trying to fix.
> >> And we should teach the agent to disable the key repeat function,
> >> which is a trival xset -r, or XGetKeyboardControl /
> >> XChangeKeyboardControl equivalent when a client is connected. Any
> >> other opinion?
> >
> > Using the agent to disable key repeat sounds fine, but you want to do
> > that for windows as well.
> 
> Windows doesn't need change, since it seems to use "hardware" key repeat.
> 
> > Just to be clear, you propose the client generates the repeat, and the
> > agent disable the guest repeat, so if there is jitter in the network the
> > repeated key press events as seen by the guest would have differing time
> > difference.
> 
> Not sure what you mean by "differing time difference", but somehow it
> should be under client control whether there is key repeat.

I mean that if you have the client send three key down messages, as a
result of the user pressing a key and not letting go, you would get
receive times:

send-time   receive-time  receive-time-difference send-time-difference
0           0.3
1           1.2           0.9                     1.0
2           2.3           1.1                     1.0

For instance.

> 
> Hans gave a longer reply, but unfortunately, it was only addressed to
> me. Hans can you resend?
> 
> The interesting is that disabling x11 key repeat will not work with
> only Down...Down events.. It will need Down/Up (press-release).
> 
> 
> -- 
> Marc-André Lureau
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On 04/20/2012 01:59 AM, Marc-André Lureau wrote:
> Hi
>
> Now that I implemented it a bit more correctly, using a timer, and
> splitting the in two patches, I realize that it doesn't work with
> windows. Windows expect to receive several key press events
> apparently.

Hmm, that would mean that windows expect the keyboard itself to handle
keyrepeat. IOW it depends on hardware keyrepeat... ?

> Hans, the hack you mentionned in spicec doesn't seem to be
> what you said, since spicec client repeatly sends down messages when
> the key is pressed

Ah I was expecting spicec to have a key-state array like spice-gtk and
ignore the repeated press events since the key was already pressed. But
up on further examination of the code that is indeed not what is
happening, what is happening on key repeat is:

X-windows sends:
Keypress, timestamp X
Keyrelease + Keypress, both with timestamp X + something

spicec filters out the Keyrelease caused by X11 key-repeat, but it
does not filter out the press, so indeed it sends repeated down
messages, without any release/up messages in between (where as
X-windows does send release events in between).

> (btw, Gtk doesn't behave the way you described
> either, so this previous patch was working fine)

So gtk also filters out the release events and only gives repeated press
events for key-repeat?

> So it seems like we should keep sending repeat key press in fact. And
> it's probably better to rely on client-side repeat, since letting the
> server handling it may lead to precisely the bug I was trying to fix.
> And we should teach the agent to disable the key repeat function,
> which is a trival xset -r, or XGetKeyboardControl /
> XChangeKeyboardControl equivalent when a client is connected. Any
> other opinion?

I just tried the following:
1) start a linux guest
2) connect with either remote-viewer or spicec (does not matter)
3) start terminal, do xset -r
4) Keep the "a" key pressed

Result: no repeat

This is sort of what I was afraid of, the IMHO hack where the client
handles repeats and sends repeated pressed without releases in between
simply gets ignored on Linux guests, I tried with showkey and the kernel
does pass the press events but X11 simply ignores the repeated presses,
this also explains why we did not see much of a problem from this before,
otherwise we would have seen double the repeat rate under X11 before
(both from its internal repeat as from the extra events).

So we seem to have the following situation:

1) Windows wants / expect repeated press events for repeats, iow
repeat is handled client side, as we are currently doing

2) Linux ignores repeated press events, but has software keyrepeat
build into X11

Which also explains why we've only been seeing the spurious
key repeat with linux guests. Note I'm sure if all reports actually
are Linux guests, does someone know?

So we need to come up with some fix for 2. Options I see are:

1) Marc-André's initial approach, with the small delay, modified to
    keep the repeated keypress events, so as to not break windows.

2) Make Linux be more like windows, what I've in mind here is:
2.1) Add an agent-channel capability called "x11_client_side_keyrepeat"
2.2) Modify the linux agent and spice-gtk to support this capability
2.3) If both sides have the capability then:
2.4) The agent will do the equivalent of xset -r
2.5) spice-gtk will send release + press events on repeat instead
      of just press events

Disadvantage of 1:
-Adds extra latency to keypress handling, but we could make this
  configurable and disable it on low latency links / only enable
  it on high latency links

Disadvantage of 2:
-This causes the guest to think the user is hitting the key X times
  a second, some apps (ie spicec and spice-gtk) may show unexpecting
  behavior because of this

Regards,

Hans
Hi,

On 04/22/2012 03:16 PM, Marc-André Lureau wrote:
> Hi
>
> On Sun, Apr 22, 2012 at 9:40 AM, Alon Levy<alevy@redhat.com>  wrote:
>> On Fri, Apr 20, 2012 at 01:59:05AM +0200, Marc-André Lureau wrote:
>>> So it seems like we should keep sending repeat key press in fact. And
>>> it's probably better to rely on client-side repeat, since letting the
>>> server handling it may lead to precisely the bug I was trying to fix.
>>> And we should teach the agent to disable the key repeat function,
>>> which is a trival xset -r, or XGetKeyboardControl /
>>> XChangeKeyboardControl equivalent when a client is connected. Any
>>> other opinion?
>>
>> Using the agent to disable key repeat sounds fine, but you want to do
>> that for windows as well.
>
> Windows doesn't need change, since it seems to use "hardware" key repeat.
>
>> Just to be clear, you propose the client generates the repeat, and the
>> agent disable the guest repeat, so if there is jitter in the network the
>> repeated key press events as seen by the guest would have differing time
>> difference.
>
> Not sure what you mean by "differing time difference", but somehow it
> should be under client control whether there is key repeat.
>
> Hans gave a longer reply, but unfortunately, it was only addressed to
> me. Hans can you resend?

Oops that was not my intention. I've resend it now including the list.

Regards,

Hans