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

[Hans de Goede]

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