[Spice-devel] [PATCH spice-gtk 1/2] cursor: Add cursor shape property
Pavel Grunt
pgrunt at redhat.com
Mon May 22 08:27:38 UTC 2017
On Fri, 2017-05-19 at 15:56 +0200, Victor Toso wrote:
> Hi,
>
> On Fri, May 12, 2017 at 01:00:00PM +0200, Pavel Grunt wrote:
> > It provides information about the remote cursor similar to the
> > signal
> > "cursor-set". The benefit over the signal is that the property can
> > be
> > queried at any time.
> >
> > Users of the "cursor-set" signal should migrate to
> > "notify::cursor".
> >
> > Related:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1411380
> > ---
> > doc/reference/spice-gtk-sections.txt | 3 ++
> > src/channel-cursor.c | 99
> > +++++++++++++++++++++++++++++++++++-
> > src/channel-cursor.h | 25 +++++++++
> > src/map-file | 1 +
> > src/spice-glib-sym-file | 1 +
> > src/spice-widget.c | 31 ++++++-----
> > 6 files changed, 144 insertions(+), 16 deletions(-)
> >
> > diff --git a/doc/reference/spice-gtk-sections.txt
> > b/doc/reference/spice-gtk-sections.txt
> > index 6f49df3..82c930e 100644
> > --- a/doc/reference/spice-gtk-sections.txt
> > +++ b/doc/reference/spice-gtk-sections.txt
> > @@ -180,6 +180,8 @@ SPICE_IS_DISPLAY_CHANNEL_CLASS
> > SPICE_DISPLAY_CHANNEL_GET_CLASS
> > SPICE_TYPE_GL_SCANOUT
> > spice_gl_scanout_get_type
> > +SPICE_TYPE_CURSOR_SHAPE
> > +spice_cursor_shape_get_type
> > <SUBSECTION Private>
> > SpiceDisplayChannelPrivate
> > </SECTION>
> > @@ -189,6 +191,7 @@ SpiceDisplayChannelPrivate
> > <TITLE>SpiceCursorChannel</TITLE>
> > SpiceCursorChannel
> > SpiceCursorChannelClass
> > +SpiceCursorShape
> > <SUBSECTION Standard>
> > SPICE_CURSOR_CHANNEL
> > SPICE_IS_CURSOR_CHANNEL
> > diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> > index 609243b..25ba366 100644
> > --- a/src/channel-cursor.c
> > +++ b/src/channel-cursor.c
> > @@ -36,8 +36,9 @@
> > * The Spice protocol defines a set of messages for controlling
> > cursor
> > * shape and position on the remote display area. The cursor
> > changes
> > * that should be reflected on the display are notified by
> > - * signals. See for example #SpiceCursorChannel::cursor-set
> > - * #SpiceCursorChannel::cursor-move signals.
> > + * signals. See for example #SpiceCursorChannel::cursor-set and
> > + * #SpiceCursorChannel::cursor-move signals and the
> > #SpiceCursorChannel:cursor
> > + * property.
> > */
> >
> > #define
> > SPICE_CURSOR_CHANNEL_GET_PRIVATE(obj)
> > \
> > @@ -55,6 +56,13 @@ struct display_cursor {
> > struct _SpiceCursorChannelPrivate {
> > display_cache *cursors;
> > gboolean init_done;
> > + SpiceCursorShape last_cursor;
> > +};
> > +
> > +/* Properties */
> > +enum {
> > + PROP_0,
> > + PROP_CURSOR,
> > };
> >
> > enum {
> > @@ -74,7 +82,30 @@ static void
> > channel_set_handlers(SpiceChannelClass *klass);
> >
> > G_DEFINE_TYPE(SpiceCursorChannel, spice_cursor_channel,
> > SPICE_TYPE_CHANNEL)
> >
> > +static SpiceCursorShape *spice_cursor_shape_copy(const
> > SpiceCursorShape *cursor);
> > +static void spice_cursor_shape_free(SpiceCursorShape *cursor);
> > +
> > +G_DEFINE_BOXED_TYPE(SpiceCursorShape, spice_cursor_shape,
> > + (GBoxedCopyFunc)spice_cursor_shape_copy,
> > + (GBoxedFreeFunc)spice_cursor_shape_free)
> > +
> > /* --------------------------------------------------------------
> > ---- */
> > +static SpiceCursorShape *spice_cursor_shape_copy(const
> > SpiceCursorShape *cursor)
> > +{
>
> g_return_val_if_fail(cursor != NULL, NULL);
ok
>
> > + SpiceCursorShape *new_cursor = g_new(SpiceCursorShape, 1);
> > + *new_cursor = *cursor;
> > + new_cursor->data = g_memdup(cursor->data, cursor->width *
> > cursor->height * 4);
> > +
> > + return new_cursor;
> > +}
> > +
> > +static void spice_cursor_shape_free(SpiceCursorShape *cursor)
> > +{
> > + g_return_if_fail(cursor != NULL);
> > +
> > + g_free(cursor->data);
> > + g_free(cursor);
> > +}
> >
> > static void spice_cursor_channel_init(SpiceCursorChannel
> > *channel)
> > {
> > @@ -107,15 +138,60 @@ static void
> > spice_cursor_channel_reset(SpiceChannel *channel, gboolean
> > migrating
> > SPICE_CHANNEL_CLASS(spice_cursor_channel_parent_class)-
> > >channel_reset(channel, migrating);
> > }
> >
> > +static void spice_cursor_channel_dispose(GObject *object)
> > +{
> > + SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(object)-
> > >priv;
> > +
> > + g_clear_pointer(&c->last_cursor.data, g_free);
>
> Out of curiosity, putting on _finalize was not enough?
I copied pattern used in other classes which works according to glib
documentation:
the dispose function is supposed to drop all references to other
objects, but keep the instance otherwise intact, so that client method
invocations still work. It may be run multiple times (due to reference
loops). Before returning, dispose should chain up to the dispose
method of the parent class.
>
> > +
> > + if (G_OBJECT_CLASS(spice_cursor_channel_parent_class)-
> > >dispose)
> > + G_OBJECT_CLASS(spice_cursor_channel_parent_class)-
> > >dispose(object);
> > +}
> > +
> > +static void spice_cursor_channel_get_property(GObject *object,
> > + guint prop_id
> > ,
> > + GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + SpiceCursorChannel *channel = SPICE_CURSOR_CHANNEL(object);
> > + SpiceCursorChannelPrivate *c = channel->priv;
> > +
> > + switch (prop_id) {
> > + case PROP_CURSOR:
> > + g_value_set_static_boxed(value, c->last_cursor.data ? &c-
> > >last_cursor : NULL);
> > + break;
> > + default:
> > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,
> > pspec);
> > + break;
> > + }
> > +}
> > +
> > static void
> > spice_cursor_channel_class_init(SpiceCursorChannelClass *klass)
> > {
> > GObjectClass *gobject_class = G_OBJECT_CLASS(klass);
> > SpiceChannelClass *channel_class =
> > SPICE_CHANNEL_CLASS(klass);
> >
> > + gobject_class->dispose = spice_cursor_channel_dispose;
> > gobject_class->finalize = spice_cursor_channel_finalize;
> > + gobject_class->get_property =
> > spice_cursor_channel_get_property;
> > channel_class->channel_reset = spice_cursor_channel_reset;
> >
> > /**
> > + * SpiceCursorChannel:cursor:
> > + *
> > + * The last #SpiceCursorShape received.
> > + *
> > + * Since: 0.34
> > + */
> > + g_object_class_install_property
> > + (gobject_class, PROP_CURSOR,
> > + g_param_spec_boxed("cursor",
> > + "Last cursor shape",
> > + "Last cursor shape received from the
> > server",
> > + SPICE_TYPE_CURSOR_SHAPE,
> > + G_PARAM_READABLE |
> > + G_PARAM_STATIC_STRINGS));
> > + /**
> > * SpiceCursorChannel::cursor-set:
> > * @cursor: the #SpiceCursorChannel that emitted the signal
> > * @width: width of the shape
> > @@ -128,6 +204,8 @@ static void
> > spice_cursor_channel_class_init(SpiceCursorChannelClass *klass)
> > *
> > * The #SpiceCursorChannel::cursor-set signal is emitted to
> > modify
> > * cursor aspect and position on the display area.
> > + *
> > + * Deprecated: 0.34: Use #SpiceCursorChannel:cursor notify
> > instead.
> > **/
> > signals[SPICE_CURSOR_SET] =
> > g_signal_new("cursor-set",
> > @@ -399,7 +477,24 @@ cache_add:
> > /* coroutine context */
> > static void emit_cursor_set(SpiceChannel *channel, display_cursor
> > *cursor)
> > {
> > + SpiceCursorChannelPrivate *c;
> > +
> > g_return_if_fail(cursor != NULL);
> > +
> > + c = SPICE_CURSOR_CHANNEL(channel)->priv;
> > +
>
> Remove trailing space
>
> > + c->last_cursor.type = cursor->hdr.type;
> > + c->last_cursor.width = cursor->hdr.width;
> > + c->last_cursor.height = cursor->hdr.height;
> > + c->last_cursor.hot_spot_x = cursor->hdr.hot_spot_x;
> > + c->last_cursor.hot_spot_y = cursor->hdr.hot_spot_y;
> > + g_clear_pointer(&c->last_cursor.data, g_free);
> > + if (cursor->data != NULL) {
> > + c->last_cursor.data = g_memdup(cursor->data,
> > + cursor->hdr.width *
> > cursor->hdr.height * 4);
> > + }
>
> Documentation says that g_memdup() will return NULL if cursor->data
> is
> null, so you can remove the if(). (Looking at the g_memdup() code,
> it
> does will not warn/critical so removing if() is fine)
>
> > +
> > + g_coroutine_object_notify(G_OBJECT(channel), "cursor");
> > g_coroutine_signal_emit(channel, signals[SPICE_CURSOR_SET],
> > 0,
> > cursor->hdr.width, cursor-
> > >hdr.height,
> > cursor->hdr.hot_spot_x, cursor-
> > >hdr.hot_spot_y,
> > diff --git a/src/channel-cursor.h b/src/channel-cursor.h
> > index 20f0380..a90c739 100644
> > --- a/src/channel-cursor.h
> > +++ b/src/channel-cursor.h
> > @@ -37,6 +37,29 @@ typedef struct _SpiceCursorChannel
> > SpiceCursorChannel;
> > typedef struct _SpiceCursorChannelClass SpiceCursorChannelClass;
> > typedef struct _SpiceCursorChannelPrivate
> > SpiceCursorChannelPrivate;
> >
> > +#define SPICE_TYPE_CURSOR_SHAPE (spice_cursor_shape_get_type ())
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> Extra space!
>
> To be honest, that's pretty common with _get_type()
>
> -> grep -rniI "spice_.*get_type (" src/ | wc -l ==> 22
> -> grep -rniI "spice_.*get_type(" src/ | wc -l ==> 57
>
> But without space wins (besides or coding style)
>
> > +/**
> > + * SpiceCursorShape:
> > + * @type: a #SpiceCursorType of @data
> > + * @width: a width of the remote cursor
> > + * @height: a height of the remote cursor
> > + * @hot_spot_x: a 'x' coordinate of the remote cursor
> > + * @hot_spot_y: a 'y' coordinate of the remote cursor
> > + * @data: image data of the remote cursor
> > + *
> > + * The #SpiceCursorShape structure defines the remote cursor's
> > shape.
> > + *
> > + */
> > +typedef struct _SpiceCursorShape SpiceCursorShape;
> > +struct _SpiceCursorShape {
> > + SpiceCursorType type;
> > + guint16 width;
> > + guint16 height;
> > + guint16 hot_spot_x;
> > + guint16 hot_spot_y;
> > + gpointer data;
> > +};
> > +
> > /**
> > * SpiceCursorChannel:
> > *
> > @@ -76,6 +99,8 @@ struct _SpiceCursorChannelClass {
> >
> > GType spice_cursor_channel_get_type(void);
> >
> > +GType spice_cursor_shape_get_type(void) G_GNUC_CONST;
> > +
> > G_END_DECLS
> >
> > #endif /* __SPICE_CLIENT_CURSOR_CHANNEL_H__ */
> > diff --git a/src/map-file b/src/map-file
> > index 31cafc2..668ff41 100644
> > --- a/src/map-file
> > +++ b/src/map-file
> > @@ -20,6 +20,7 @@ spice_channel_test_common_capability;
> > spice_channel_type_to_string;
> > spice_client_error_quark;
> > spice_cursor_channel_get_type;
> > +spice_cursor_shape_get_type;
> > spice_display_change_preferred_compression;
> > spice_display_change_preferred_video_codec_type;
> > spice_display_channel_get_type;
> > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> > index d73f799..e061744 100644
> > --- a/src/spice-glib-sym-file
> > +++ b/src/spice-glib-sym-file
> > @@ -18,6 +18,7 @@ spice_channel_test_common_capability
> > spice_channel_type_to_string
> > spice_client_error_quark
> > spice_cursor_channel_get_type
> > +spice_cursor_shape_get_type
> > spice_display_change_preferred_compression
> > spice_display_change_preferred_video_codec_type
> > spice_display_channel_get_type
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 8203d55..b1c8ab1 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -2635,29 +2635,32 @@ static void mark(SpiceDisplay *display,
> > gint mark)
> > }
> >
> > static void cursor_set(SpiceCursorChannel *channel,
> > - gint width, gint height, gint hot_x, gint
> > hot_y,
> > - gpointer rgba, gpointer data)
> > + G_GNUC_UNUSED GParamSpec *pspec,
> > + gpointer data)
>
> This changes is to move the callback from cursor-set signal to
> cursor
> property. I'd move it to a follow up patch.
>
> > {
> > SpiceDisplay *display = data;
> > SpiceDisplayPrivate *d = display->priv;
> > GdkCursor *cursor = NULL;
> > + SpiceCursorShape *cursor_shape = NULL;
>
> Don't assign NULL here.
Why?
>
> >
> > cursor_invalidate(display);
> >
> > - g_clear_object(&d->mouse_pixbuf);
> > -
> > - if (rgba != NULL) {
> > - d->mouse_pixbuf = gdk_pixbuf_new_from_data(g_memdup(rgba,
> > width * height * 4),
> > + 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,
> > - width,
> > - height,
> > - width * 4,
> > - (GdkPixbufDest
> > royNotify)g_free, NULL);
> > - d->mouse_hotspot.x = hot_x;
> > - d->mouse_hotspot.y = hot_y;
> > + 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,
> > hot_x, hot_y);
> > + d->mouse_pixbuf,
> > + d->mouse_hotspot.x,
> > + d->mouse_hotspot.y);
> > } else
> > g_warn_if_reached();
>
> It would make sense with the property callback to make this an early
> return in case cursor_shape is NULL. (/me is almost always looking
> forward to early returns and less code indentation)
ok
Thanks,
Pavel
>
> >
> > @@ -2958,7 +2961,7 @@ static void channel_new(SpiceSession *s,
> > SpiceChannel *channel, gpointer data)
> > if (id != d->channel_id)
> > return;
> > d->cursor = SPICE_CURSOR_CHANNEL(channel);
> > - spice_g_signal_connect_object(channel, "cursor-set",
> > + spice_g_signal_connect_object(channel, "notify::cursor",
> > G_CALLBACK(cursor_set),
> > display, 0);
>
> Cheers,
> toso
>
> > spice_g_signal_connect_object(channel, "cursor-move",
> > G_CALLBACK(cursor_move),
> > display, 0);
> > --
> > 2.12.2
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list