[Spice-devel] [PATCH spice-gtk] RFC: Simplify cursor reset code

[Hans de Goede]

Hi,

On 03/31/2012 02:08 AM, Marc-André Lureau wrote:
> I am not sure what is the exact semantic of cursor reset: it's not
> set, hide or show. Until now all it was doing is setting back default
> cursor on display. However, this is not correct if the cursor is in
> server mode (cursor should stay hidden).
>
> With this change, it now "forget" any previous cursor shape, which I
> think is more appropriate for a "reset", while still reseting default
> cursor if in client mode.
>
> This is not changing any apparent behaviour though, just cleaning up a
> bit the code and fixing a potential mouse being wrongly shown.

Hmm, after this patch d->mouse_cursor can be NULL, were before it
would never be NULL and several places rely on it not being NULL.

IE cursor_set does an unconditional:

gdk_cursor_unref(d->mouse_cursor);

Which would cause a NULL deref if done after your new reset ...

Proposed fix below.


> ---
>   gtk/spice-widget.c |   44 +++++++++++++++++++++-----------------------
>   1 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> index 0defb60..c4ccc56 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -306,19 +306,9 @@ static void spice_display_dispose(GObject *obj)
>       G_OBJECT_CLASS(spice_display_parent_class)->dispose(obj);
>   }
>
> -static void spice_display_finalize(GObject *obj)
> +static void cursor_free(SpiceDisplay *display)
>   {
> -    SpiceDisplay *display = SPICE_DISPLAY(obj);
> -    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> -
> -    SPICE_DEBUG("Finalize spice display");
> -
> -    if (d->grabseq) {
> -        spice_grab_sequence_free(d->grabseq);
> -        d->grabseq = NULL;
> -    }
> -    g_free(d->activeseq);
> -    d->activeseq = NULL;
> +    SpiceDisplayPrivate *d = display->priv;
>
>       if (d->show_cursor) {
>           gdk_cursor_unref(d->show_cursor);
> @@ -330,10 +320,23 @@ static void spice_display_finalize(GObject *obj)
>           d->mouse_cursor = NULL;
>       }
>
> -    if (d->mouse_pixbuf) {
> -        g_object_unref(d->mouse_pixbuf);
> -        d->mouse_pixbuf = NULL;
> +    g_clear_object(&d->mouse_pixbuf);
> +}
> +
> +static void spice_display_finalize(GObject *obj)
> +{
> +    SpiceDisplay *display = SPICE_DISPLAY(obj);
> +    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> +
> +    SPICE_DEBUG("Finalize spice display");
> +
> +    if (d->grabseq) {
> +        spice_grab_sequence_free(d->grabseq);
> +        d->grabseq = NULL;
>       }
> +    g_free(d->activeseq);
> +    d->activeseq = NULL;
> +    cursor_free(display);
>
>       G_OBJECT_CLASS(spice_display_parent_class)->finalize(obj);
>   }
> @@ -1723,15 +1726,10 @@ static void cursor_move(SpiceCursorChannel *channel, gint x, gint y, gpointer da
>   static void cursor_reset(SpiceCursorChannel *channel, gpointer data)
>   {
>       SpiceDisplay *display = data;
> -    GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
>
> -    if (!window) {
> -        SPICE_DEBUG("%s: no window, returning",  __FUNCTION__);
> -        return;
> -    }
> -
> -    SPICE_DEBUG("%s",  __FUNCTION__);
> -    gdk_window_set_cursor(window, NULL);
> +    SPICE_DEBUG("%s", __FUNCTION__);
> +    cursor_free(display);

I believe you should add the following line here:
        display->priv->mouse_cursor = get_blank_cursor();

To fix the issue I've described above.

> +    update_mouse_pointer(display);
>   }
>
>   static void disconnect_main(SpiceDisplay *display)


Regards,

Hans