[Spice-devel] [PATCH spice-gtk 2/3] Add a SpiceGtkSession Class

[Hans de Goede]

Hi,

Thanks for the review!

On 10/05/2011 12:03 AM, Marc-André Lureau wrote:

<snip>

>> +static void spice_gtk_session_init(SpiceGtkSession *gtk_session)
>> +{
>> +    SpiceGtkSessionPrivate *s;
>> +
>> +    SPICE_DEBUG("New gtk session (compiled from package " PACKAGE_STRING ")");
>
> Well we already have version information somewhere, so you may drop
> this line to avoid redundancy in the log. Not a big deal though ;)
>

Dropped.

>> +    s = gtk_session->priv = SPICE_GTK_SESSION_GET_PRIVATE(gtk_session);
>> +    memset(s, 0, sizeof(*s));
>
> I know this is used all over the place, but now that I am acutally
> reviewing it I actually can check it.
>
> gtype actually instantiate with  g_slice_alloc0 (ALIGN_STRUCT
> (total_instance_size) + node->data->instance.private_size);
>
> So there is no need for memset. Feel free to commit a seperate patch
> after for cleanup in the spice-gtk codebase. Thanks :)

I dropped this one. Feel free to do a cleanup patch for the rest of
the code base :)

>
>> +}
>> +
>> +static GObject *
>> +spice_gtk_session_constructor(GType                  gtype,
>> +                              guint                  n_properties,
>> +                              GObjectConstructParam *properties)
>> +{
>> +    GObject *obj;
>> +    SpiceGtkSession *gtk_session;
>> +
>> +    {
>> +        /* Always chain up to the parent constructor */
>> +        GObjectClass *parent_class;
>> +        parent_class = G_OBJECT_CLASS(spice_gtk_session_parent_class);
>> +        obj = parent_class->constructor(gtype, n_properties, properties);
>> +    }
>> +
>> +    gtk_session = SPICE_GTK_SESSION(obj);
>> +    if (!gtk_session->priv->session)
>> +        g_error("SpiceGtKSession constructed without a session");
>> +
>> +    return obj;
>> +}
>> +
>
>
> In term of style you could simply:
>
> GObject *self =
> G_OBJECT_CLASS(spice_gtk_session_parent_class)->constructor(gtype,
> n_properties, properties);
>
> g_warn_if_fail (SPICE_GTK_SESSION(self)->priv->session);
>
> return self;
>
> Still, you will need to deal with that situation later on. So perhaps
> best just to have the check before calling g_object_new():

The purpose of having the check here is to also catch direct
callers of g_object_new() (which there really shouldn't be but better
safe then sorry). Further more this is a g_error and not a warning exactly
so that I don't have to check for not having a session later on.

Calling spice_gtk_session_get with a NULL session is a programming error
which should be fixed right away rather then people living with the warnings.

<snip>

>> @@ -1506,3 +1512,22 @@ void spice_session_get_caches(SpiceSession *session,
>>      if (glz_window)
>>          *glz_window = s->glz_window;
>>   }
>> +
>> +GObject *spice_session_get_gtk_session(SpiceSession *session)
>> +{
>> +    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
>> +
>> +    g_return_val_if_fail(s != NULL, NULL);
>> +
>> +    return s->spice_gtk_session;
>> +}
>> +
>> +void spice_session_set_gtk_session(SpiceSession *session,
>> +                                   GObject *spice_gtk_session)
>> +{
>> +    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
>> +
>> +    g_return_if_fail(s != NULL);
>> +
>> +    s->spice_gtk_session = spice_gtk_session;
>> +}
>
> If we don't want to make this API public at all, we could also use
> gobject_{set,get}_data_full(), and the spice_session_set_gtk_session()
> functions as part of the gtk library.  What do you think?

Good idea! Changed this in my local tree. I'll be working on this some
more today. I'll send an updated patchset by the end of today.

Regards,

Hans