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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]