[virt-tools-list] [libosinfo v3] API to query required user avatar format

[Christophe Fergeau]

On Thu, Nov 22, 2012 at 03:51:59PM +0200, Zeeshan Ali (Khattak) wrote:
> On Thu, Nov 22, 2012 at 11:26 AM, Christophe Fergeau
> <cfergeau at redhat.com> wrote:
> > On Wed, Nov 21, 2012 at 07:39:05PM +0200, Zeeshan Ali (Khattak) wrote:
> >> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
> > In particular, is pygtk
> > able to handle it? I seem to remember some problems there.
> 
> I was told that this should work with python bindings at least and
> when I reported back that it works with Vala bindings out of the box,
> tomeu on #introspection was impressed. :)  If it doesn't actually work
> for pygtk, its really not our problem as long as
> gobject-instrospection (from Boxes POV, vapigen) supports it and can
> handle it nicely.

Well, we want libosinfo to work nicely from python, so if it was something
that cannot work in pygtk, it would be better to avoid using that, just as
we'd avoid things that are not convenient for vala.
> >
> > I think the default value will not get set without G_PARAM_CONSTRUCT_ONLY |
> > G_PARAM_WRITABLE, did you test that specific case?
> 
> We don't need or want to set the default prop. We just translate the
> string we get from XML to int on runtime and if string is not there,
> we assume the default: -1.

Ah ok, I didn't look carefully enough at the getters ;) Good for me then.

> >> +/**
> >> + * osinfo_install_script_get_avatar_format
> >> + *
> >> + * Some install scripts have restrictions on the format of the user avatar. Use
> >> + * this method to retrieve those restrictions in the form of an
> >> + * #OsinfoAvatarFormat instance.
> >> + *
> >> + * Returns: (transfer full): The avatar format, or NULL if there is no restrictions on the
> >> + *                           format of avatar
> >> + */
> >> +OsinfoAvatarFormat *osinfo_install_script_get_avatar_format(OsinfoInstallScript *script)
> >> +{
> >> +    g_return_val_if_fail(OSINFO_IS_INSTALL_SCRIPT(script), NULL);
> >> +
> >> +    if (script->priv->avatar == NULL)
> >> +        return NULL;
> >> +
> >> +    return g_object_ref(script->priv->avatar);
> >
> > The libosinfo object getters I looked at are (transfer none), any specific reason for
> > this one to be (transfer full) ?
> 
> We already have some exception to this in libosinfo, especially in
> case of lists and list objects.

I could only find one such getter, and it was (transfer full) because it
creates the list every time the getter is called rather than caching it.

> The general practice in the gobject
> world (I think I actually read this in gobject docs) is to always
> return a new ref for objects so I usually just follow that unless
> project in question has an explicit rule to not follow it. What do you
> think we should do here?

I couldn't find any cases of getters of cached objects returning a new ref
to it (I haven't looked at all getters though), so I'd rather we don't
return a ref in this case.

> 
> >> +}
> >> +
> >>  struct _OsinfoInstallScriptGenerateData {
> >>      GSimpleAsyncResult *res;
> >>      OsinfoOs *os;
> >> diff --git a/osinfo/osinfo_install_script.h b/osinfo/osinfo_install_script.h
> >> index 036b572..c6bc2df 100644
> >> --- a/osinfo/osinfo_install_script.h
> >> +++ b/osinfo/osinfo_install_script.h
> >> @@ -24,6 +24,7 @@
> >>  #include <glib-object.h>
> >>  #include <gio/gio.h>
> >>  #include <osinfo/osinfo_install_config_param.h>
> >> +#include <osinfo/osinfo_avatar_format.h>
> >>
> >>  #ifndef __OSINFO_INSTALL_SCRIPT_H__
> >>  #define __OSINFO_INSTALL_SCRIPT_H__
> >> @@ -106,6 +107,11 @@ const gchar *osinfo_install_script_get_output_filename(OsinfoInstallScript *scri
> >>
> >>  const gchar *osinfo_install_script_get_expected_filename(OsinfoInstallScript *script);
> >>
> >> +void osinfo_install_script_set_avatar_format(OsinfoInstallScript *script,
> >> +                                             OsinfoAvatarFormat *avatar);
> >> +
> >
> > This symbol is not exported so should not appear in a public header.
> 
> Actually I forgot to export it. :) But now that you mention it, yeah I
> think we should put it in a private header..

Oh, I'm not saying it should not be exported, I just wanted to point out
the inconsistency. So it's your call.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20121122/4b23c471/attachment.sig>