[Spice-devel] [PATCH spice-gtk 1/2] cursor: Add cursor shape property
Victor Toso
victortoso at redhat.com
Mon May 22 08:45:01 UTC 2017
Hi,
On Mon, May 22, 2017 at 10:27:38AM +0200, Pavel Grunt wrote:
> 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.
Ah, yes, I do like and prefer dispose() too to clear things up. The
question was more or less OT.
>
> >
> > > +
> > > + 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?
It will be assigned below. Not having a default assignment may help with
code changes as compiler should warn with -Wuninitialized.
Maybe we should add that to coding style to avoid it unless in
situations like:
for (it = list; it != NULL; it = it->next) {
SpiceChannel *channel = SPICE_CHANNEL(it->data);
...
}
>
> > >
> > > 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
Cheers!
>
> >
> > >
> > > @@ -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
-------------- 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/20170522/6a1601fd/attachment.sig>
More information about the Spice-devel
mailing list