[Spice-devel] [PATCH] Grab keyboard based on session focus

[Pavel Grunt]

Hi Snir,

thank you for the patch, it looks good. I put some comments inline.

On Wed, 2015-11-11 at 12:45 +0200, Snir Sheriber wrote:
> When using multiple monitors moving mouse between monitors releases
> keyboard grab.
> 
> Reproduce bug
> -Open multiple monitors remote-viewer session
> -Click on one of the monitors to get focus & keyboard-grab
> -Move mouse to another monitor and try any keyboard command (do not click)
> At this point all keyboard commands are being executed on the client machine
> instead of the remote machine
> 
> I added additional session focus test in addition to the
> monitor focus test when trying keyboard-grab
> 
> Resolves: rhbz#1275231
> 
> ---
> 
> No GObject properties was set
> ---
>  src/spice-gtk-session-priv.h |  2 ++
>  src/spice-gtk-session.c      | 19 +++++++++++++++++++
>  src/spice-widget.c           | 10 +++++-----
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/src/spice-gtk-session-priv.h b/src/spice-gtk-session-priv.h
> index 91304b2..2a9b752 100644
> --- a/src/spice-gtk-session-priv.h
> +++ b/src/spice-gtk-session-priv.h
> @@ -28,6 +28,8 @@ gboolean spice_gtk_session_get_read_only(SpiceGtkSession
> *self);
>  void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self);
>  void spice_gtk_session_set_pointer_grabbed(SpiceGtkSession *self, gboolean
> grabbed);
>  gboolean spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self);
> +void spice_gtk_session_set_keyboard_grabbed(SpiceGtkSession *self, gboolean
> grabbed);
> +gboolean spice_gtk_session_get_keyboard_grabbed(SpiceGtkSession *self);
>  
>  G_END_DECLS
>  
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 5abb16c..7b413d2 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -64,6 +64,7 @@ struct _SpiceGtkSessionPrivate {
>      gboolean                auto_usbredir_enable;
>      int                     auto_usbredir_reqs;
>      gboolean                pointer_grabbed;
> +    gboolean                keyboard_grabbed;
>  };
>  
>  /**
> @@ -1220,6 +1221,16 @@ void
> spice_gtk_session_set_pointer_grabbed(SpiceGtkSession *self, gboolean grabb
>      g_object_notify(G_OBJECT(self), "pointer-grabbed");
>  }
>  
> +
> +G_GNUC_INTERNAL
> +void spice_gtk_session_set_keyboard_grabbed(SpiceGtkSession *self, gboolean
> grabbed)
> +{
> +    g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> +
> +    self->priv->keyboard_grabbed = grabbed;
> +    //g_object_notify(G_OBJECT(self), "pointer-grabbed");
this line should be removed
> +}
> +
>  G_GNUC_INTERNAL
>  gboolean spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self)
>  {
> @@ -1227,3 +1238,11 @@ gboolean
> spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self)
>  
>      return self->priv->pointer_grabbed;
>  }
> +
> +G_GNUC_INTERNAL
> +gboolean spice_gtk_session_get_keyboard_grabbed(SpiceGtkSession *self)
> +{
> +    g_return_val_if_fail(SPICE_IS_GTK_SESSION(self), FALSE);
> +
> +    return self->priv->keyboard_grabbed;
> +}
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 503f82a..99050b5 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -206,9 +206,8 @@ static void update_size_request(SpiceDisplay *display)
>  static void update_keyboard_focus(SpiceDisplay *display, gboolean state)
>  {
>      SpiceDisplayPrivate *d = display->priv;
> -
you don't have to remove the blank lines
>      d->keyboard_have_focus = state;
> -

> +    spice_gtk_session_set_keyboard_grabbed(d->gtk_session, state);
>      /* keyboard grab gets inhibited by usb-device-manager when it is
>         in the process of redirecting a usb-device (as this may show a
>         policykit dialog). Making autoredir/automount setting changes while
> @@ -730,15 +729,16 @@ static void try_keyboard_grab(SpiceDisplay *display)
>          return;
>      if (d->disable_inputs)
>          return;
> -

>      if (d->keyboard_grab_inhibit)
>          return;
>      if (!d->keyboard_grab_enable)
>          return;
>      if (d->keyboard_grab_active)
>          return;
> -    if (!d->keyboard_have_focus)
> -        return;
> +    if (!spice_gtk_session_get_keyboard_grabbed(d->gtk_session)){
a missing space before '{'
> +        if (!d->keyboard_have_focus)
> +            return;
is it needed? Can the gtk session grab be False and d->keyboard_have_focus True?
> +    }
>      if (!d->mouse_have_pointer)
>          return;
>      if (d->keyboard_grab_released)

Thanks,

Pavel