[Spice-devel] [PATCH spice-gtk 1/2] spice-widget: cleanup mouse handling.

[Alon Levy]

On Fri, Dec 09, 2011 at 06:46:51PM +0100, Hans de Goede wrote:

Personal best #comment/#code ? :)

> Some background on this patch, we currently behave
> as follows in server mouse mode:
> 
> 1) When spicy first connects, a frozen guest cursor is shown
>  (no mouse events are send to the guest) and the client cursor
>  is set to the standard right pointer,
> 
> 2) When one clicks on / inside the spice-widget, the mouse is
>  grabbed, the client cursor is hidden and the guest pointer
>  becomes alive (gets events).
> 
> 3) On ungrab we move back to state 1)
> 
> Which is fine, but the code implementing it is somewhat convoluted.
> We have update_mouse_pointer(), which does more then just update the mouse
> pointer, it also calls try_mouse_grab(), and we've update_mouse_grab(),
> which calls update_mouse_pointer() rather then try_mouse_grab().
> 
> * I. lets look at what we currently have in update_mouse_pointer(): *
> 
>     case SPICE_MOUSE_MODE_SERVER:
>         if (!d->mouse_grab_active) {
>             if (gdk_window_get_cursor(window) != NULL)
>                 gdk_window_set_cursor(window, NULL);
>         } else {
>             try_mouse_grab(display);
>         }
>         break;
> 
> Now lets invert the test to make it more readable:
> 
>     case SPICE_MOUSE_MODE_SERVER:
>         if (d->mouse_grab_active) {
>             try_mouse_grab(display);
>         } else {
>             if (gdk_window_get_cursor(window) != NULL)
>                 gdk_window_set_cursor(window, NULL);
>         }
>         break;
> 
> Hmm, so if we're grabbing the mouse, we try to grab the mouse
> -> does not make sense, esp not since try_mouse_grab() has:
> 
>     if (d->mouse_grab_active)
>         return;
> 
> So we can drop the else block.
> 
> But why the if? Well that would be because when we're grabbing
> the windows should have a blank cursor. But where does that get set?
> 
> The blank cursor gets set in do_pointer_grab(), by specifying a blank
> cursor as the cursor to show as long as the grab is active.
> 
> Since that cursor will be active as long as we're grabbing, so we can
> drop the if (!d->mouse_grab_active) check as well.
> 
> And with the mouse_grab_active check gone, we no longer need to call
> update_mouse_pointer() from try_mouse_ungrab().
> 
> * II. lets look at update_mouse_grab() *
> 
> So currently when the mouse should be grabbed according to the
> mouse-grab and disable-inputs properties, update_mouse_grab() calls
> update_mouse_pointer(), which as discussed above does nothing with the grab,
> as the code path calling try_mouse_grab() is dead code. The right thing to do
> would of course be to have update_mouse_grab() call try_mouse_grab() in this
> case.
> 
> But, that would mean that as soon as the property is changed the cursor will
> get torn away from whereever it is and get grabbed, which may not always
> be desirable. To fix this this patch also moves the mouse_have_pointer
> and keyboard_have_focus checks from mouse_update() to try_mouse_grab()
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> 
> foo
> ---
>  gtk/spice-widget.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> index e270c06..bff529c 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -575,12 +575,8 @@ static void update_mouse_pointer(SpiceDisplay *display)
>              gdk_window_set_cursor(window, d->mouse_cursor);
>          break;
>      case SPICE_MOUSE_MODE_SERVER:
> -        if (!d->mouse_grab_active) {
> -            if (gdk_window_get_cursor(window) != NULL)
> -                gdk_window_set_cursor(window, NULL);
> -        } else {
> -            try_mouse_grab(display);
> -        }
> +        if (gdk_window_get_cursor(window) != NULL)
> +            gdk_window_set_cursor(window, NULL);
>          break;
>      default:
>          g_warn_if_reached();
> @@ -597,6 +593,11 @@ static void try_mouse_grab(SpiceDisplay *display)
>      if (d->disable_inputs)
>          return;
>  
> +    if (!d->mouse_have_pointer)
> +        return;
> +    if (!d->keyboard_have_focus)
> +        return;
> +
>      if (!d->mouse_grab_enable)
>          return;
>      if (d->mouse_mode != SPICE_MOUSE_MODE_SERVER)
> @@ -654,7 +655,6 @@ static void try_mouse_ungrab(SpiceDisplay *display)
>  
>      d->mouse_grab_active = false;
>  
> -    update_mouse_pointer(display);
>      g_signal_emit(display, signals[SPICE_DISPLAY_MOUSE_GRAB], 0, false);
>  }
>  
> @@ -663,7 +663,7 @@ static void update_mouse_grab(SpiceDisplay *display)
>      SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
>  
>      if (d->mouse_grab_enable && !d->disable_inputs)
> -        update_mouse_pointer(display);
> +        try_mouse_grab(display);
>      else
>          try_mouse_ungrab(display);
>  }
> @@ -1419,9 +1419,7 @@ static void mouse_update(SpiceChannel *channel, gpointer data)
>          try_mouse_ungrab(display);
>          break;
>      case SPICE_MOUSE_MODE_SERVER:
> -        if (d->mouse_have_pointer &&
> -            d->keyboard_have_focus)
> -            try_mouse_grab(display);
> +        try_mouse_grab(display);
>          break;
>      default:
>          g_warn_if_reached();
> -- 
> 1.7.7.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel