[Spice-devel] [spice-gtk 2/2] Handle pause key correctly

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> On Tue, 2016-07-26 at 10:04 +0100, Frediano Ziglio wrote:
> > Windows does not like Pause key sent with same scancodes as Break.
> > Although is the same physical key the two functions send two complitely
> typo - completely

Got it, fixed.

> > different set of codes.
> > Tested with Linux and Windows clients.
> > Tested with Linux, Windows and DOS guests.
> > On Windows guest VK_PAUSE was not arriving correctly.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  src/spice-widget.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index c7dd553..7890f70 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -1286,6 +1286,25 @@ static gboolean key_press_delayed(gpointer data)
> >      return FALSE;
> >  }
> >  
> > +static bool send_pause(SpiceDisplay *display, GdkEventType type)
> > +{
> > +    SpiceInputsChannel *inputs = display->priv->inputs;
> > +
> > +    /* Send proper scancodes.
> > +     * Note that this key send all scancode when pressed
> > +     * but nothing when released.
> 
> What is the reason for that ?
> 
> So (in client):
>  Press "Pause"
>  Press something else
>  Release something else
>  Release "Pause"
> will produce (in guest):
>  Press "Pause"
>  Release "Pause"
>  Press something else
>  Release something else
> 
> It is a different behavior than in the client
> 

This will send the same hardware sequence as hardware Pause produce.
The guest will get a pause sequence, there is no "something else".
Actually the software fake a press+release and you can think
the events are separate but if you physically press pause
you will get press and release at the same time while release
will be silent.
When Windows receives the E1 1D 45 sequence it just sent a single
WM_KEYDOWN with virtual code VK_PAUSE and scancode 0x45.

> > +     * The 0x21d is a sort of Third-Ctrl while
> > +     * 0x45 is the NumLock.
> > +     */
> > +    if (type == GDK_KEY_PRESS) {
> > +        spice_inputs_key_press(inputs, 0x21d);
> > +        spice_inputs_key_press(inputs, 0x45);
> > +        spice_inputs_key_release(inputs, 0x21d);
> > +        spice_inputs_key_release(inputs, 0x45);
> 
> Do you know why it is needed to enter these "manually" ? Is the keymap
> wrongly
> generated?
> 

There are some problems:
- send_key does not support scancodes > 512 and adding support for 0x21d
  (extending key_state) just for this does not make much sense to me;
- the key (as you can see) send multiple scancodes and the keymap table just
  store one, still I don't see much value supporting multiple scancodes in
  keymap table just for this;
- strangely looks like 0xff13 (Pause) was send as Break (scancode 326) and the
  row in keymaps.csv is 
KEY_PAUSE,119,,,,198,98,,326,72,VK_PAUSE,0x013,0x66,0x66,XK_Pause,0xff13
  while the Break key is empty
KEY_BREAK,0x19b,,,,,,,,,,,,
  if you fill KEY_BREAK nothing happens, if you change KEY_PAUSE you break
  both functions (Break and Pause)

> > +    }
> > +    return true;
> > +}
> > +
> >  static void send_key(SpiceDisplay *display, int scancode, SendKeyType
> >  type,
> > gboolean press_delayed)
> >  {
> >      SpiceDisplayPrivate *d = display->priv;
> > @@ -1479,6 +1498,14 @@ static gboolean key_event(GtkWidget *widget,
> > GdkEventKey *key)
> >      if (!d->inputs)
> >          return true;
> >  
> > +    if (key->keyval == GDK_KEY_Pause
> > +#ifdef G_OS_WIN32
> > +        /* for some reason GDK does not fill keyval for VK_PAUSE */
> 
> rather bug for gtk ?
> 

I think so. The keyval is filled with 0xffffff.

> > +        || key->hardware_keycode == VK_PAUSE
> > +#endif
> > +        ) {
> > +        return send_pause(display, key->type);
> > +    }
> >      if (!scancode)
> >          scancode = vnc_display_keymap_gdk2xtkbd(d->keycode_map, d-
> > >keycode_maplen,
> >                                                  key->hardware_keycode);
> 
> 
> Thanks,
> Pavel
> 
> 

Frediano