[Spice-devel] [PATCH spice-gtk 5/5] widget: differentiate key press & release from press only events
Hans de Goede
hdegoede at redhat.com
Thu Aug 16 02:02:07 PDT 2012
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
More information about the Spice-devel
mailing list