[Spice-devel] [PATCH spice-gtk 1/2] cursor: Add cursor shape property
Victor Toso
victortoso at redhat.com
Fri May 19 13:56:32 UTC 2017
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);
> + 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?
> +
> + 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.
>
> 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,
> - (GdkPixbufDestroyNotify)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(display)),
> - 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)
>
> @@ -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/20170519/941f19f8/attachment.sig>
More information about the Spice-devel
mailing list