[Spice-devel] [PATCH spice-gtk v2 3/3] widget: Set cursor during construction

[Victor Toso]

Hi,

On Thu, May 25, 2017 at 03:13:13PM +0200, Pavel Grunt wrote:
> On Wed, 2017-05-24 at 09:22 +0200, Victor Toso wrote:
> > On Tue, May 23, 2017 at 04:56:26PM +0200, Pavel Grunt wrote:
> > > In a multimonitor environment can easily happen that a cursor is
> > > set
> > > before some of the SpiceDisplays are created. IOW the first
> > > created
> > > SpiceDisplay has the cursor but others don't.
> > > 
> > > To avoid the issue set the initial cursor using the "cursor"
> > > property
> > > of the SpiceCursorChannel.
> > > 
> > > Resolves:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1411380
> > > ---
> > > v2: changed to "early return"
> > > ---
> > >  src/spice-widget.c | 36 +++++++++++++++++++-----------------
> > >  1 file changed, 19 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > index b1c8ab1..a117484 100644
> > > --- a/src/spice-widget.c
> > > +++ b/src/spice-widget.c
> > > @@ -2646,23 +2646,24 @@ static void cursor_set(SpiceCursorChannel
> > > *channel,
> > >      cursor_invalidate(display);
> > > 
> > >      g_object_get(G_OBJECT(channel), "cursor", &cursor_shape,
> > > NULL);
> > > -    if (cursor_shape != NULL && cursor_shape->data != NULL) {
> > > -        g_clear_object(&d->mouse_pixbuf);
> > > -        d->mouse_pixbuf = gdk_pixbuf_new_from_data(cursor_shape-
> > > >data,
> > > -                                                   GDK_COLORSPACE
> > > _RGB,
> > > -                                                   TRUE, 8,
> > > -                                                   cursor_shape-
> > > >width,
> > > -                                                   cursor_shape-
> > > >height,
> > > -                                                   cursor_shape-
> > > >width * 4,
> > > -                                                   NULL, NULL);
> > > -        d->mouse_hotspot.x = cursor_shape->hot_spot_x;
> > > -        d->mouse_hotspot.y = cursor_shape->hot_spot_y;
> > > -        cursor =
> > > gdk_cursor_new_from_pixbuf(gtk_widget_get_display(GTK_WIDGET(displ
> > > ay)),
> > > -                                            d->mouse_pixbuf,
> > > -                                            d->mouse_hotspot.x,
> > > -                                            d->mouse_hotspot.y);
> > > -    } else
> > > -        g_warn_if_reached();
> > > +    if (G_UNLIKELY(cursor_shape == NULL || cursor_shape->data ==
> > > NULL)) {
> > > +        return;
> > > +    }
> > 
> > Is it unlikely? Do we always receive cursor->data?
> It is unlikely, i've never seen this warning. I change it to return,
> because it is now called in the widget construction. I can change it
> back if it is preferred.

I think it is fine to keep the return :)

Cheers,

> 
> > 
> > Before it was g_warn_if_reached() which means we would very likely
> > be
> > calling update_mouse_pointer() and cursor_invalidate(). Not calling
> > this functions now is part of the solution to rhbz#1411380? If so, I
> > would mention it in the commit log too.
> > 
> > Acked-by: Victor Toso <victortoso at redhat.com>
> > 
> > > +
> > > +    g_clear_object(&d->mouse_pixbuf);
> > > +    d->mouse_pixbuf = gdk_pixbuf_new_from_data(cursor_shape-
> > > >data,
> > > +                                               GDK_COLORSPACE_RGB
> > > ,
> > > +                                               TRUE, 8,
> > > +                                               cursor_shape-
> > > >width,
> > > +                                               cursor_shape-
> > > >height,
> > > +                                               cursor_shape-
> > > >width * 4,
> > > +                                               NULL, NULL);
> > > +    d->mouse_hotspot.x = cursor_shape->hot_spot_x;
> > > +    d->mouse_hotspot.y = cursor_shape->hot_spot_y;
> > > +    cursor =
> > > gdk_cursor_new_from_pixbuf(gtk_widget_get_display(GTK_WIDGET(displ
> > > ay)),
> > > +                                        d->mouse_pixbuf,
> > > +                                        d->mouse_hotspot.x,
> > > +                                        d->mouse_hotspot.y);
> > >  s
> > >  #if HAVE_EGL
> > >      if (egl_enabled(d))
> > > @@ -2970,6 +2971,7 @@ static void channel_new(SpiceSession *s,
> > > SpiceChannel *channel, gpointer data)
> > >          spice_g_signal_connect_object(channel, "cursor-reset",
> > >                                        G_CALLBACK(cursor_reset),
> > > display, 0);
> > >          spice_channel_connect(channel);
> > > +        cursor_set(d->cursor, NULL, display);
> > >          return;
> > >      }
> > >  
> > > -- 
> > > 2.13.0
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170525/62d4a083/attachment.sig>