[Spice-devel] [PATCH] RedChannelClient: use Gobject properties

[Frediano Ziglio]

> 
> On Fri, Nov 04, 2016 at 11:19:22AM -0500, Jonathon Jongsma wrote:
> > Use g_param_spec_object() instead of g_param_spec_pointer() for the
> > 'client' and 'channel' properties now that these types are GObjects.
> > This improves refcounting and typesafety slightly.
> > ---
> >  server/red-channel-client.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 84dd28f..ad4a545 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -150,10 +150,10 @@ red_channel_client_get_property(GObject *object,
> >              g_value_set_pointer(value, self->priv->stream);
> >              break;
> >          case PROP_CHANNEL:
> > -            g_value_set_pointer(value, self->priv->channel);
> > +            g_value_set_object(value, self->priv->channel);
> 
> If something is using getters, this will introduce a leak, but I could
> not find anything doing g_object_get(..., "channel", ...)
> 

Probably this property can be write-only + construct so we don't
need to get it?
(not a regression by the way, just I noted that one of the last
patch introduced such a kind of property)

> >              break;
> >          case PROP_CLIENT:
> > -            g_value_set_pointer(value, self->priv->client);
> > +            g_value_set_object(value, self->priv->client);
> >              break;
> >          case PROP_MONITOR_LATENCY:
> >              g_value_set_boolean(value, self->priv->monitor_latency);
> > @@ -195,12 +195,10 @@ red_channel_client_set_property(GObject *object,
> >          case PROP_CHANNEL:
> >              if (self->priv->channel)
> >                  g_object_unref(self->priv->channel);
> > -            self->priv->channel = g_value_get_pointer(value);
> > -            if (self->priv->channel)
> > -                g_object_ref(self->priv->channel);
> > +            self->priv->channel = g_value_dup_object(value);
> >              break;
> >          case PROP_CLIENT:
> > -            self->priv->client = g_value_get_pointer(value);
> > +            self->priv->client = g_value_get_object(value);
> 
> Just to be sure, we don't want to take a ref on the client?
> 

Good question. More related to reference counting talk
than this rationale.
Currently behavior is if MainChannelClient is closed all clients
(RedClient and others RedChannelClients) are destroyed.
But for the other RedChannelClients a strong pointer to RedClient
make sense. Not sure if RedClient currently has a strong pointer
to MainChannelClient, surely a RedClient cannot exist without
a MainChannelClient.

> Looks good to me as is.
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 

Agree

Frediano

> >              break;
> >          case PROP_MONITOR_LATENCY:
> >              self->priv->monitor_latency = g_value_get_boolean(value);
> > @@ -312,18 +310,20 @@ static void
> > red_channel_client_class_init(RedChannelClientClass *klass)
> >                                  | G_PARAM_CONSTRUCT_ONLY);
> >      g_object_class_install_property(object_class, PROP_STREAM, spec);
> >  
> > -    spec = g_param_spec_pointer("channel", "channel",
> > -                                "Associated RedChannel",
> > -                                G_PARAM_STATIC_STRINGS
> > -                                | G_PARAM_READWRITE
> > -                                | G_PARAM_CONSTRUCT_ONLY);
> > +    spec = g_param_spec_object("channel", "channel",
> > +                               "Associated RedChannel",
> > +                               RED_TYPE_CHANNEL,
> > +                               G_PARAM_STATIC_STRINGS
> > +                               | G_PARAM_READWRITE
> > +                               | G_PARAM_CONSTRUCT_ONLY);
> >      g_object_class_install_property(object_class, PROP_CHANNEL, spec);
> >  
> > -    spec = g_param_spec_pointer("client", "client",
> > -                                "Associated RedClient",
> > -                                G_PARAM_STATIC_STRINGS
> > -                                | G_PARAM_READWRITE
> > -                                | G_PARAM_CONSTRUCT_ONLY);
> > +    spec = g_param_spec_object("client", "client",
> > +                               "Associated RedClient",
> > +                               RED_TYPE_CLIENT,
> > +                               G_PARAM_STATIC_STRINGS
> > +                               | G_PARAM_READWRITE
> > +                               | G_PARAM_CONSTRUCT_ONLY);
> >      g_object_class_install_property(object_class, PROP_CLIENT, spec);
> >  
> >      spec = g_param_spec_boolean("monitor-latency", "monitor-latency",