[Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Amos Kong]

Currently we emit press events of combined keys first, then emit
release events by reverse order. But it doesn't match with physical
keyboard if the keys contain continued & repeated keys.

For example, (qemu) sendkey a-b-b

Current emited events: (actually the second 'presse b' and 'release b'
can't be identified by guest, the interval is too short)
  press a
  press b
  press b
  release b
  release b
  release a

Correct events order of physical keyboard:
  press a
  press b
  release b
  press b
  release b
  release a

This patch fixed the event order if keys contain continued & repeated
keys. his patch based on:
http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg05150.html

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
V2: rebase this patch on Gerd's better fix of release order
---
 ui/input-legacy.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ui/input-legacy.c b/ui/input-legacy.c
index a698a34..3fd3e83 100644
--- a/ui/input-legacy.c
+++ b/ui/input-legacy.c
@@ -95,9 +95,17 @@ void qmp_send_key(KeyValueList *keys, bool has_hold_time, int64_t hold_time,
     for (p = keys; p != NULL; p = p->next) {
         qemu_input_event_send_key(NULL, copy_key_value(p->value), true);
         qemu_input_event_send_key_delay(hold_time);
-        up = g_realloc(up, sizeof(*up) * (count+1));
-        up[count] = copy_key_value(p->value);
-        count++;
+
+        /* release event will be emitted immediately if next key is repeated */
+        if (p->next && p->value->kind == p->next->value->kind &&
+            p->value->qcode == p->next->value->qcode) {
+            qemu_input_event_send_key(NULL, copy_key_value(p->value), false);
+            qemu_input_event_send_key_delay(hold_time);
+        } else {
+            up = g_realloc(up, sizeof(*up) * (count+1));
+            up[count] = copy_key_value(p->value);
+            count++;
+        }
     }
     while (count) {
         count--;
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Gerd Hoffmann]

On Fr, 2014-09-26 at 18:23 +0800, Amos Kong wrote:
> Currently we emit press events of combined keys first, then emit
> release events by reverse order. But it doesn't match with physical
> keyboard if the keys contain continued & repeated keys.
> 
> For example, (qemu) sendkey a-b-b

Hmm, somehow I don't feel like building too much magic into this.
If you want send Ctrl-somekey twice just use two sendkey commands ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Amos Kong]

On Fri, Sep 26, 2014 at 12:36:50PM +0200, Gerd Hoffmann wrote:
> On Fr, 2014-09-26 at 18:23 +0800, Amos Kong wrote:
> > Currently we emit press events of combined keys first, then emit
> > release events by reverse order. But it doesn't match with physical
> > keyboard if the keys contain continued & repeated keys.
> > 
> > For example, (qemu) sendkey a-b-b
> 
> Hmm, somehow I don't feel like building too much magic into this.
> If you want send Ctrl-somekey twice just use two sendkey commands ...


Before this patch, If 'sendkey r-o-o-t', only 'rot' can be inputted.

People want to panic windows by sending Ctrl-Scrool-Scrool
http://msdn.microsoft.com/en-us/library/windows/hardware/ff545499(v=vs.85).aspx
But current events order doesn't work.

In physical keyboard. We can prese Ctrl first, then press & release
Scroll twice, then release Ctrl. It's very common behavior.

So this fix just reference the physical implement, if you want to
input same key twice, you have to release it before second pressing.
(here we ignore the auto-repeat feature)


> cheers,
>   Gerd
> 

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Gerd Hoffmann]

On Fr, 2014-09-26 at 18:53 +0800, Amos Kong wrote:
> On Fri, Sep 26, 2014 at 12:36:50PM +0200, Gerd Hoffmann wrote:
> > On Fr, 2014-09-26 at 18:23 +0800, Amos Kong wrote:
> > > Currently we emit press events of combined keys first, then emit
> > > release events by reverse order. But it doesn't match with physical
> > > keyboard if the keys contain continued & repeated keys.
> > > 
> > > For example, (qemu) sendkey a-b-b
> > 
> > Hmm, somehow I don't feel like building too much magic into this.
> > If you want send Ctrl-somekey twice just use two sendkey commands ...
> 
> 
> Before this patch, If 'sendkey r-o-o-t', only 'rot' can be inputted.

To type 'root' you should use

sendkey r
sendkey o
sendkey o
sendkey t

Multiple keys in sendkey is meant for multiple keys pressed at the same
time, i.e. ctrl-alt-del, not for sending key sequences and typing words.

> People want to panic windows by sending Ctrl-Scrool-Scrool
> http://msdn.microsoft.com/en-us/library/windows/hardware/ff545499(v=vs.85).aspx
> But current events order doesn't work.

sendkey Ctrl-Scroll
sendkey Ctrl-Scroll

> In physical keyboard. We can prese Ctrl first, then press & release
> Scroll twice, then release Ctrl. It's very common behavior.

In most cases it doesn't matter whenever you release the modifier key or
not.  The windows panic hotkey might be the exception from the rule
though.

> So this fix just reference the physical implement, if you want to
> input same key twice, you have to release it before second pressing.
> (here we ignore the auto-repeat feature)

sendkey doesn't cover that use case indeed.

/me wonders what happened to the input-send-event patch from marcelo,
see http://patchwork.ozlabs.org/patch/360649/

According to patchwork I've picked it up.  But it is neither upstream
nor in my local input branch.  And I can't remember what happened :(
Marcelo, any clue?  Maybe I should just re-queue it ...

The input-send-event gives you fine-grained control about the exact
input event sequence and it can handle your use case without problems.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

On 09/26/2014 04:53 AM, Amos Kong wrote:
> On Fri, Sep 26, 2014 at 12:36:50PM +0200, Gerd Hoffmann wrote:
>> On Fr, 2014-09-26 at 18:23 +0800, Amos Kong wrote:
>>> Currently we emit press events of combined keys first, then emit
>>> release events by reverse order. But it doesn't match with physical
>>> keyboard if the keys contain continued & repeated keys.
>>>
>>> For example, (qemu) sendkey a-b-b
>>
>> Hmm, somehow I don't feel like building too much magic into this.
>> If you want send Ctrl-somekey twice just use two sendkey commands ...
> 
> 
> Before this patch, If 'sendkey r-o-o-t', only 'rot' can be inputted.

sendkey with multiple keys is designed for shift modifiers, NOT for
repeated keys.  'sendkey r-o-o-t' makes no sense; it is the equivalent
of mashing down the 'r', 'o', and 't' keys at once.  To send four
separate key events, you need to do 'sendkey r; sendkey o; sendkey o;
sendkey t'.  (Or libvirt should learn some syntactic sugar so you could
do 'sendkey r -- o -- o -- t' and sequence it automatically).

I don't think this patch is worth the effort.

> 
> People want to panic windows by sending Ctrl-Scrool-Scrool

s/Scrool/Scroll/

> http://msdn.microsoft.com/en-us/library/windows/hardware/ff545499(v=vs.85).aspx
> But current events order doesn't work.
> 
> In physical keyboard. We can prese Ctrl first, then press & release
> Scroll twice, then release Ctrl. It's very common behavior.
> 

Hmm.  This almost argues that we need a way to send a press without a
release, or a release without a press, rather than trying to make
'sendkey Ctrl-Scroll-Scroll' emulate double scroll pushes all while ctrl
is still held down.  But this example is MUCH more realistic than
'r-o-o-t', so maybe there is merit to your patch after all.

> So this fix just reference the physical implement, if you want to
> input same key twice, you have to release it before second pressing.
> (here we ignore the auto-repeat feature)
> 


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Marcelo Tosatti]

On Fri, Sep 26, 2014 at 01:24:05PM +0200, Gerd Hoffmann wrote:
> On Fr, 2014-09-26 at 18:53 +0800, Amos Kong wrote:
> > On Fri, Sep 26, 2014 at 12:36:50PM +0200, Gerd Hoffmann wrote:
> > > On Fr, 2014-09-26 at 18:23 +0800, Amos Kong wrote:
> > > > Currently we emit press events of combined keys first, then emit
> > > > release events by reverse order. But it doesn't match with physical
> > > > keyboard if the keys contain continued & repeated keys.
> > > > 
> > > > For example, (qemu) sendkey a-b-b
> > > 
> > > Hmm, somehow I don't feel like building too much magic into this.
> > > If you want send Ctrl-somekey twice just use two sendkey commands ...
> > 
> > 
> > Before this patch, If 'sendkey r-o-o-t', only 'rot' can be inputted.
> 
> To type 'root' you should use
> 
> sendkey r
> sendkey o
> sendkey o
> sendkey t
> 
> Multiple keys in sendkey is meant for multiple keys pressed at the same
> time, i.e. ctrl-alt-del, not for sending key sequences and typing words.
> 
> > People want to panic windows by sending Ctrl-Scrool-Scrool
> > http://msdn.microsoft.com/en-us/library/windows/hardware/ff545499(v=vs.85).aspx
> > But current events order doesn't work.
> 
> sendkey Ctrl-Scroll
> sendkey Ctrl-Scroll
> 
> > In physical keyboard. We can prese Ctrl first, then press & release
> > Scroll twice, then release Ctrl. It's very common behavior.
> 
> In most cases it doesn't matter whenever you release the modifier key or
> not.  The windows panic hotkey might be the exception from the rule
> though.
> 
> > So this fix just reference the physical implement, if you want to
> > input same key twice, you have to release it before second pressing.
> > (here we ignore the auto-repeat feature)
> 
> sendkey doesn't cover that use case indeed.
> 
> /me wonders what happened to the input-send-event patch from marcelo,
> see http://patchwork.ozlabs.org/patch/360649/
> 
> According to patchwork I've picked it up.  But it is neither upstream
> nor in my local input branch.  And I can't remember what happened :(
> Marcelo, any clue?  Maybe I should just re-queue it ...

I was wondering the same... just requeue please. Let me know if it fails 
to apply and i'll rebase/resend.

> The input-send-event gives you fine-grained control about the exact
> input event sequence and it can handle your use case without problems.
> 
> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Amos Kong]

On Fri, Sep 26, 2014 at 01:24:05PM +0200, Gerd Hoffmann wrote:
> On Fr, 2014-09-26 at 18:53 +0800, Amos Kong wrote:
> > On Fri, Sep 26, 2014 at 12:36:50PM +0200, Gerd Hoffmann wrote:
> > > On Fr, 2014-09-26 at 18:23 +0800, Amos Kong wrote:
> > > > Currently we emit press events of combined keys first, then emit
> > > > release events by reverse order. But it doesn't match with physical
> > > > keyboard if the keys contain continued & repeated keys.
> > > > 
> > > > For example, (qemu) sendkey a-b-b
> > > 
> > > Hmm, somehow I don't feel like building too much magic into this.
> > > If you want send Ctrl-somekey twice just use two sendkey commands ...
> > 
> > 
> > Before this patch, If 'sendkey r-o-o-t', only 'rot' can be inputted.
> 
> To type 'root' you should use
> 
> sendkey r
> sendkey o
> sendkey o
> sendkey t
> 
> Multiple keys in sendkey is meant for multiple keys pressed at the same
> time, i.e. ctrl-alt-del, not for sending key sequences and typing words.
> 
> > People want to panic windows by sending Ctrl-Scrool-Scrool
> > http://msdn.microsoft.com/en-us/library/windows/hardware/ff545499(v=vs.85).aspx
> > But current events order doesn't work.
> 
> sendkey Ctrl-Scroll
> sendkey Ctrl-Scroll
> 
> > In physical keyboard. We can prese Ctrl first, then press & release
> > Scroll twice, then release Ctrl. It's very common behavior.
> 
> In most cases it doesn't matter whenever you release the modifier key or
> not.  The windows panic hotkey might be the exception from the rule
> though.

It doesn't matter, so users might release the modifier key or not.
we should make both works

1)
sendkey Ctrl-Scroll
sendkey Ctrl-Scroll

2)
sendkey Ctrl-Scroll-Scroll
 
> > So this fix just reference the physical implement, if you want to
> > input same key twice, you have to release it before second pressing.
> > (here we ignore the auto-repeat feature)
> 
> sendkey doesn't cover that use case indeed.

original sendkey didn't cover it, but my change isn't added a split
feature, but try to adapt native behavior by a little change.
It doesn't effect original right behavior, but process repeated case
in native way.
 
> /me wonders what happened to the input-send-event patch from marcelo,
> see http://patchwork.ozlabs.org/patch/360649/
> 
> According to patchwork I've picked it up.  But it is neither upstream
> nor in my local input branch.  And I can't remember what happened :(
> Marcelo, any clue?  Maybe I should just re-queue it ...
> 
> The input-send-event gives you fine-grained control about the exact
> input event sequence and it can handle your use case without problems.
> 
> cheers,
>   Gerd

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Gerd Hoffmann]

  Hi,

> > /me wonders what happened to the input-send-event patch from marcelo,
> > see http://patchwork.ozlabs.org/patch/360649/
> > 
> > According to patchwork I've picked it up.  But it is neither upstream
> > nor in my local input branch.  And I can't remember what happened :(
> > Marcelo, any clue?  Maybe I should just re-queue it ...
> 
> I was wondering the same... just requeue please. Let me know if it fails 
> to apply and i'll rebase/resend.

Doesn't apply any more, so yes, please rebase & resend.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Gerd Hoffmann]

  Hi,

> It doesn't matter, so users might release the modifier key or not.
> we should make both works
> 
> 1)
> sendkey Ctrl-Scroll
> sendkey Ctrl-Scroll

Good to know this works.

> 2)
> sendkey Ctrl-Scroll-Scroll

Why?  This tries to squeeze something into the sendkey interface which
it doesn't was designed for, and IMO this isn't a good idea, especially
if we have something better at hand (marcelos patch).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] ui/input: fix event emitting of repeated combined keys

[Amos Kong]

On Mon, Sep 29, 2014 at 11:09:56AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > It doesn't matter, so users might release the modifier key or not.
> > we should make both works
> > 
> > 1)
> > sendkey Ctrl-Scroll
> > sendkey Ctrl-Scroll
> 
> Good to know this works.
> 
> > 2)
> > sendkey Ctrl-Scroll-Scroll
> 
> Why? 

Quote from http://msdn.microsoft.com/en-us/library/windows/hardware/ff545499(v=vs.85).aspx

 The keyboard crash can be initiated by using the following hotkey
 sequence: _Hold down_ the rightmost CTRL key, and press the SCROLL LOCK
 key twice.

> This tries to squeeze something into the sendkey interface which
> it doesn't was designed for, and IMO this isn't a good idea, especially
> if we have something better at hand (marcelos patch).
 
My patch fixed this problem, but it's a hack. Marcelo's patch works in
(press/release) event level, it's more clear.

Thanks.

> cheers,
>   Gerd
> 

-- 
			Amos.