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@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel