[Spice-devel] [PATCH spice-gtk] RFC: release pointer grab on grab-broken

[Hans de Goede]

Hi,

On 03/26/2013 09:33 PM, Marc-André Lureau wrote:
> On windows, the client receives a WM_KILLFOCUS event which generates
> solely a keyboard grab-broken event.
>
> This event is received when pressing ctrl-alt-del (to show up the task
> manager), and we need to release the pointer grab and clip region in
> this case for the client to be usable.
>
> This also clear the clipping region when the client pops-up a dialog.
>
> Since keyboard focus is a pre-requisite for pointer grab, it sounds
> logical to release the pointer grab if losing keyboard focus.

Hmm, I consider the adding of the try_mouse_ungrab(display) call
to try_keyboard_ungrab(display) adding of an undesirable side effect,
and thus breaking the principle of least surprise.

IOW someone who does not know every detail of the code and adds a
try_keyboard_ungrab(display) somewhere may end up being surprised
that the mouse also get ungrabbed.

See my comments below for a proposed alternative fix.

>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=857114
> https://bugzilla.redhat.com/show_bug.cgi?id=922818
> ---
>   gtk/spice-widget.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> index fb8094e..324f5ee 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -477,15 +477,13 @@ static GdkCursor* get_blank_cursor(void)
>   static gboolean grab_broken(SpiceDisplay *self, GdkEventGrabBroken *event,
>                               gpointer user_data G_GNUC_UNUSED)
>   {
> -    SpiceDisplayPrivate *d = self->priv;
> +    SPICE_DEBUG("%s (implicit: %d, keyboard: %d)", __FUNCTION__,
> +                event->implicit, event->keyboard);
>
> -    SPICE_DEBUG("%s (%d)", __FUNCTION__, event->implicit);
>       if (event->keyboard) {
> -        d->keyboard_grab_active = false;
> -        g_signal_emit(self, signals[SPICE_DISPLAY_KEYBOARD_GRAB], 0, false);
> +        try_keyboard_ungrab(self);
>       } else {

Instead of adding the try_mouse_ungrab call to try_keyboard_ungrab
I would like to see the above block changed to:

      if (event->keyboard) {
         try_keyboard_ungrab(self);
         try_mouse_ungrab(self);
      } else {

This should also fix the issue you're trying to fix. With the added
advantage that it will release the mouse even if for some reason
d->keyboard_grab_active is false when we get the grab_broken.

> -        d->mouse_grab_active = false;
> -        g_signal_emit(self, signals[SPICE_DISPLAY_MOUSE_GRAB], 0, false);
> +        try_mouse_ungrab(self);
>       }
>
>       return false;
> @@ -751,6 +749,9 @@ static void try_keyboard_ungrab(SpiceDisplay *display)
>   #endif
>       d->keyboard_grab_active = false;
>       g_signal_emit(widget, signals[SPICE_DISPLAY_KEYBOARD_GRAB], 0, false);
> +
> +    /* for consistency, there should not be only a mouse grab */
> +    try_mouse_ungrab(display);
>   }

And then the call here can be dropped...

Regards,

Hans