[Spice-devel] [PATCH spice-gtk] RFC: widget: improve undesired key repeatition
Alon Levy
alevy at redhat.com
Wed Apr 18 22:40:26 PDT 2012
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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list