Bug 27792

The current state of my vala bindings branch is basically the contact list channels and their dependencies work.

There's plenty of work to be done before they're generally useful, but my development of libfolks should push the important bits along rather quickly.

Simon, you said you'd rather merge sooner rather than later, so should we just review it in terms of it not mangling the build system too badly right now, merge it, and then continue work (not closing this bug until they're deemed generally-useful, with some test cases, etc.)?

This "see also" bug link (bgo #615119) is to a bug in Vala itself (which is blocking this bug).

(In reply to comment #2)
> This "see also" bug link (bgo #615119) is to a bug in Vala itself (which is
> blocking this bug).

Fixed upstream.

Updating the branch to one rebased upon the latest tp-glib master.

OK, I think the branch is ready for review. It's not perfect, but I think it's a good starting point. And it's probably best to review now before it gets any bigger.

Simon, up for a review?

Implementing this directly on the gobject-introspect support is blocked on this bug: https://bugzilla.gnome.org/show_bug.cgi?id=618660

My bindings branch based on g-i is here:

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=shortlog;h=refs/heads/vala-bindings-gi

The first two commits aren't Vala-specific, as far as I can tell, but I haven't checked that they don't break anything for the other bindings.

There's more work that can be done (eg, marking all the nullable parameters as (allow-none)), but I'm going to hold off on that until bgo #618660 gets fixed.

Add another blocker bug from Gnome Bugzilla

Add another blocker bug from bgo.

Updating the link to my latest g-i branch. It's closer to complete, though still blocked on the bgo bugs linked under "See Also".

I don't see much in here that can't be merged (at least when it's fixed), even if the resulting Vala doesn't actually work.

Danielle, I'd appreciate your comments on the annotations.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=341964c1fc269c6b
> Return value: (element-type GLib.Value):

Not true at all. You've annotated it as a GPtrArray<GValue>, but it's a GPtrArray<TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS>, i.e. a GPtrArray of GValueArray<a{sv}, as>.

Can't we (skip) this function and let bindings use the property? This function is basically a "C binding" for the property anyway.

If this patch was correct, I'd be happy to merge it immediately, even if the Vala bindings still need fixes - it's not blocked.

> +/* XXX: the Return value line is > 80 chars because of bgo #618569 */

Does bgo#618569 actually break our bindings, or just make their documentation wrong? If it's the latter, we don't need to go to great lengths to avoid it - just get it fixed upstream (which has indeed been done) and the documentation will become correct eventually. I'm happy to bump the required g-i version.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=52a1f81c160ca90c2
> Add the proper namespace for TpDBusDaemon function annotations.

I'd be happy to merge this immediately, even if the Vala bindings still need fixes, as long as it doesn't break mainstream g-i.

Please do a git grep for "(type Object)" and fix them all, though - I'm sure there are more instances.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=b37a38f7f1005922
> Hide symbols that vapigen can't handle.

There are really two patches here.

Part 1, hiding tp_base_client_implement_foo, is OK for the moment, if bindings can't handle it; or we could even turn these three methods into normal vfuncs (there's enough ABI padding to accomodate them) and make the _implement_ thing really simple.

> +/* FIXME: this is a work-around for a vapigen bug */
> +/* hide this from the g-i scanner, since vapigen doesn't know how to
> + * handle it */

These are basically OK, because they're not useful to things outside telepathy-glib anyway (TpProxyFeature is an incomplete type, for everyone not in telepathy-glib).

I'd prefer the dummy version of list_features to be "GCallback _internal_list_features" or something.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=dca6893a95
> - ... (element-type Tp.ChannelRequest)...
> + ... (element-type TelepathyGLib.ChannelRequest)...

Looks good, and is not blocked. Please do a git grep and fix any other occurrences too.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=ebb14ca0dc4cf33f7d
> Bump requirement for g-i, to handle our use of GStrv

Not blocked, looks fine.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=dac7d0849fd8b4d7412
> Update the build system to build the Vala bindings (when enabled).

> +if HAVE_VALA
> +VALA_DIR = vala

Stylistically, I'd prefer to discard the VALA_DIR variable entirely, and use:

    if HAVE_VALA
    SUBDIRS += vala
    endif

I think that's clearer.

> +  PKG_CHECK_MODULES(LIBTEST_VALA_LOWLEVEL,
> +                    [glib-2.0 >= 2.22,
> +                     gobject-2.0 >= 2.22,
> +                     telepathy-glib])

I'm not sure what you're trying to do here but I'm pretty sure it's wrong. We're building telepathy-glib, not using the system copy.

> +VAPIGEN = $(shell pkg-config vala-1.0 --variable="vapigen")

This should come from configure.ac - ask pkg-config, and AC_SUBST it.

> +++ b/vala/telepathy-vala.pc.in

There should be an accompanying telepathy-vala-uninstalled.pc.in.

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=4dc1c9bbbb84c790a
> Add a few more g-i annotations for (uint -> TpHandle) and (uint -> TpConnectionStatus

This is two patches, as the commit message indicates :-P

> -   * @old_status: old #TpAccount:connection-status
> -   * @new_status: new #TpAccount:connection-status
> -   * @reason: the #TpAccount:connection-status-reason
> +   * @old_status: (type TpConnectionStatus): old #TpAccount:connection-status
> +   * @new_status: (type TpConnectionStatus): new #TpAccount:connection-status
> +   * @reason: (type TpConnectionStatusReason): the
> +   *  #TpAccount:connection-status-reason

Careful; @reason can have values that are outside the range of a TpConnectionStatusReason (and in principle, so can the statuses!). Will g-i and Vala cope?

> - * @handles: (array length=n_handles) (element-type uint): An array of handles
> - *  of type %TP_HANDLE_TYPE_CONTACT representing the desired contacts
> + * @handles: (array length=n_handles) (element-type TelepathyGLib.Handle): An
> + *  array of handles of type %TP_HANDLE_TYPE_CONTACT representing the desired
> + *  contacts

Having a handle not be a uint would be a massive compatibility break anyway, and TpHandle is just a typedef for guint. Is this a functional change, or just for documentation?

It seems that Danni specifically annotated this to be of element type uint: deleting the element-type annotation ought to make it be Handle automatically. Ask Danni whether annotating it back to Handle will break Python/JS?

> - * @features: (array length=n_features) (allow-none) (element-type uint): An array of features that
> - *  must be ready for use (if supported) before the callback is called (may
> - *  be %NULL if @n_features is 0)
> + * @features: (array length=n_features) (allow-none)
> + *  (element-type TelepathyGLib.ContactFeature): An array of features that must
> + *  be ready for use (if supported) before the callback is called (may be %NULL
> + *  if @n_features is 0)

It seems that Danni specifically annotated this to be of element type uint: deleting the element-type annotation ought to make it be ContactFeature automatically. Ask Danni whether annotating it back to Handle will break Python/JS?

(In reply to comment #10)

> http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=341964c1fc269c6b
> > Return value: (element-type GLib.Value):

Also it's GObject.Value

> http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=4dc1c9bbbb84c790a
> > Add a few more g-i annotations for (uint -> TpHandle) and (uint -> TpConnectionStatus
> 
> This is two patches, as the commit message indicates :-P
> 
> > -   * @old_status: old #TpAccount:connection-status
> > -   * @new_status: new #TpAccount:connection-status
> > -   * @reason: the #TpAccount:connection-status-reason
> > +   * @old_status: (type TpConnectionStatus): old #TpAccount:connection-status
> > +   * @new_status: (type TpConnectionStatus): new #TpAccount:connection-status
> > +   * @reason: (type TpConnectionStatusReason): the
> > +   *  #TpAccount:connection-status-reason

It will be TelepathyGLib.ConnectionStatus etc. -- I don't know whether PyGI/GJS does any specific validation on this value. Will have to check.

> It seems that Danni specifically annotated this to be of element type uint:
> deleting the element-type annotation ought to make it be Handle automatically.
> Ask Danni whether annotating it back to Handle will break Python/JS?

Yeah, this was added because gjs doesn't handle arrays of non-basic types very well.

(In reply to comment #10)

The new branch should address all the problems below - please do a final check on it.

> Can't we (skip) this function and let bindings use the property? This function
> is basically a "C binding" for the property anyway.

Yup, (skip)ped.
 
> If this patch was correct, I'd be happy to merge it immediately, even if the
> Vala bindings still need fixes - it's not blocked.
> 
> > +/* XXX: the Return value line is > 80 chars because of bgo #618569 */
> 
> Does bgo#618569 actually break our bindings, or just make their documentation
> wrong? If it's the latter, we don't need to go to great lengths to avoid it -
> just get it fixed upstream (which has indeed been done) and the documentation
> will become correct eventually. I'm happy to bump the required g-i version.

It kind of mangled our docs (relocated the second line to the doc text for the next element, I believe), but since it's fixed in the version we require, I've snipped this.

> http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=dca6893a95
> > - ... (element-type Tp.ChannelRequest)...
> > + ... (element-type TelepathyGLib.ChannelRequest)...
> 
> Looks good, and is not blocked. Please do a git grep and fix any other
> occurrences too.

I looked for any other instances of "type Tp" (which also gets the "type" keyword), and it only matched the TpConnectionStatus[Reason], which was resolved below.

> > +  PKG_CHECK_MODULES(LIBTEST_VALA_LOWLEVEL,
> > +                    [glib-2.0 >= 2.22,
> > +                     gobject-2.0 >= 2.22,
> > +                     telepathy-glib])

I know I fixed at least one other instance of this - must have missed this one.

> > +VAPIGEN = $(shell pkg-config vala-1.0 --variable="vapigen")
> 
> This should come from configure.ac - ask pkg-config, and AC_SUBST it.

Good idea

> > +++ b/vala/telepathy-vala.pc.in
> 
> There should be an accompanying telepathy-vala-uninstalled.pc.in.

Fixed

> http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=4dc1c9bbbb84c790a
> > Add a few more g-i annotations for (uint -> TpHandle) and (uint -> TpConnectionStatus
> 
> This is two patches, as the commit message indicates :-P
> 
> > -   * @old_status: old #TpAccount:connection-status
> > -   * @new_status: new #TpAccount:connection-status
> > -   * @reason: the #TpAccount:connection-status-reason
> > +   * @old_status: (type TpConnectionStatus): old #TpAccount:connection-status
> > +   * @new_status: (type TpConnectionStatus): new #TpAccount:connection-status
> > +   * @reason: (type TpConnectionStatusReason): the
> > +   *  #TpAccount:connection-status-reason
> 
> Careful; @reason can have values that are outside the range of a
> TpConnectionStatusReason (and in principle, so can the statuses!). Will g-i and
> Vala cope?

Yeah, they'll cope. It'd be nice to stick to the symbolic types, but there's an outstanding Vala code generator problem dealing with Handle, so I already have to sort that out in my application code.

> > - * @handles: (array length=n_handles) (element-type uint): An array of handles
> > - *  of type %TP_HANDLE_TYPE_CONTACT representing the desired contacts
> > + * @handles: (array length=n_handles) (element-type TelepathyGLib.Handle): An
> > + *  array of handles of type %TP_HANDLE_TYPE_CONTACT representing the desired
> > + *  contacts
> 
> Having a handle not be a uint would be a massive compatibility break anyway,
> and TpHandle is just a typedef for guint. Is this a functional change, or just
> for documentation?

The goal was to be able to use Handle everywhere, but it's already got the problem mentioned above. So this isn't a big deal.

> It seems that Danni specifically annotated this to be of element type uint:
> deleting the element-type annotation ought to make it be Handle automatically.
> Ask Danni whether annotating it back to Handle will break Python/JS?

It's fine to have this be a uint.

The net result was I just removed this last patch (just required some trivial changes to libfolks to handle it).

(In reply to comment #11)
> (In reply to comment #10)

> It will be TelepathyGLib.ConnectionStatus etc. -- I don't know whether PyGI/GJS
> does any specific validation on this value. Will have to check.

Oddly enough, gi/Vala translated to this for me (which is why I didn't catch this earlier). And I ended up removing this patch anyhow, so it's a moot point, but I'll be sure to use the TelepathyGLib namespace in the future (for consistency/etc.).
 
> > It seems that Danni specifically annotated this to be of element type uint:
> > deleting the element-type annotation ought to make it be Handle automatically.
> > Ask Danni whether annotating it back to Handle will break Python/JS?
> 
> Yeah, this was added because gjs doesn't handle arrays of non-basic types very
> well.

Is that true for all generics (eg, GLib.HashTable), or just primitive arrays?

We'll see how it all shakes out, but Vala is pretty good about this, so I'd hate to do a bunch of casting around generics in other cases.

> Is that true for all generics (eg, GLib.HashTable), or just primitive arrays?

No hash tables, GLists and GArrays are all fine, iirc. It's just C-allocated arrays that are bodgy. I should do a patch to recursively do conversion via the type system as for other container types.

> +  PKG_CHECK_MODULES(LIBTEST_VALA_LOWLEVEL,
> +                    [glib-2.0 >= 2.22,
> +                     gobject-2.0 >= 2.22])

I still don't see why you need this. Not only does LIBTEST_VALA_LOWLEVEL not exist in the patches you've submitted, but you can just use GLIB_CFLAGS, GLIB_LIBS.

You don't need to include GLib in the check for Vala either (indeed, we check for a strictly newer version further up the configure.ac already).

It seems to be a popular pattern to say, in a project called Foo,

    PKG_CHECK_MODULES([FOO], [bar >= 1, baz >= 2])

but that's not really right - FOO isn't what you're asking for at all. I think the ./configure message

    checking for FOO... yes

illustrates that this isn't the intended usage :-P

Another reason to check for libraries separately (as we do in telepathy-glib), rather than having a single DEPENDENCIES variable and check, is given by the GStreamer module policy:

<http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/random/moving-plugins#n72>
>   - should list libs and cflags in stack order, with lowest in the stack first
>     (so one can link against highest in the stack somewhere else without
>      picking up everything from the somewhere else)
>     e.g. $(GST_PLUGINS_BASE_CFLAGS) \
>          $(GST_BASE_CFLAGS) \
>          $(GST_CFLAGS) $(CAIRO_CFLAGS)

r+ with the additional patch.

After a final fix and confirmation on IRC, I've merged this to mainline.

It's still not quite usable for the Vala bindings themselves, but I can keep working on that without having to keep a separate, bit-rotting branch.

I'll close this bug once the bindings are sufficient for libfolks' needs and I've merged the tests.

Remove two newly-fixed Vala bugs as dependencies.

(In reply to comment #17)
> I'll close this bug once the bindings are sufficient for libfolks' needs and
> I've merged the tests.

Removed from the review queue. Please add 'patch' and a URL when appropriate.

bgo #621834 is fixed in Vala mainline. Removing as dependency.

New blocker bug in bgo: https://bugzilla.gnome.org/show_bug.cgi?id=623254

Add another blocker: https://bugzilla.gnome.org/show_bug.cgi?id=623255

Now that the final Vala bugs are fixed, I've made a few trivial changes to tp-glib to make libfolks work with these g-i based bindings.

Danielle, could you please review as well? -- I don't see any big problem with exposing Capabilities.get_channel_classes(), but I remembered you blacklisted it, and wasn't sure if it caused problems for any of the bindings you work with.

As far as the dbus-type utility functions, we can probably expose the rest of them, but I figured it'd be safer to just start with the getter functions.

(In reply to comment #23)
> Now that the final Vala bugs are fixed, I've made a few trivial changes to
> tp-glib to make libfolks work with these g-i based bindings.
> 
> Danielle, could you please review as well? -- I don't see any big problem with
> exposing Capabilities.get_channel_classes(), but I remembered you blacklisted
> it, and wasn't sure if it caused problems for any of the bindings you work
> with.

It's skipped because we have no way to introspect the contents. It's one of those functions that would be nicer as a GVariant version (e.g. GVariant<a(a{sv}as)>).

> As far as the dbus-type utility functions, we can probably expose the rest of
> them, but I figured it'd be safer to just start with the getter functions.

Again IMO it would be better to start offering alternative API with GHashTable<utf8,GVariant> or GVariant<a{sv}> then exposing all of this C convenience API into the bindings.

Both of these are probably less of an issue in Vala... thoughts?

http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=54062f5e3c0c7ff147c8cb0490bbd3a65c59b50e
> Don't implicitly transfer ownership of Contacts' or Connections' TpCapabilities in their getter functions.

That's not what this patch does (and if it did that, it'd be an ABI break). A better commit message would be "Annotate that TpCapabilities getter functions don't transfer ownership".

> Expose Capabilities.get_channel_classes() to g-i.

Will this break other g-i consumers?

If not, I'm not against it, but the long commit message seems misleading: you can use the higher-level convenience methods on TpCapabilities even if you can't use this one, surely?

> Un-blacklist the tp_asv_get_* utility functions for g-i.

Does @asv need annotating with (element-type)?

I'm not against this, although a parallel GVariant API for a{sv}s would be nice too.

However, please note that the tp_asv_set_* family are not safe for g-i, so stop here rather than continuing :-)

(The tp_asv_set_* family require prior knowledge of the memory allocation model being used for the hash table - are the GValues slice-allocated or g_malloc'd, and are the strings owned or static? - so they can't safely be used from g-i, which doesn't have this information.)

(In reply to comment #24)
> (In reply to comment #23)
> > Now that the final Vala bugs are fixed, I've made a few trivial changes to
> > tp-glib to make libfolks work with these g-i based bindings.
> > 
> > Danielle, could you please review as well? -- I don't see any big problem with
> > exposing Capabilities.get_channel_classes(), but I remembered you blacklisted
> > it, and wasn't sure if it caused problems for any of the bindings you work
> > with.
> 
> It's skipped because we have no way to introspect the contents. It's one of
> those functions that would be nicer as a GVariant version (e.g.
> GVariant<a(a{sv}as)>).

OK, I guess I'll propose some API for that.

> > As far as the dbus-type utility functions, we can probably expose the rest of
> > them, but I figured it'd be safer to just start with the getter functions.
> 
> Again IMO it would be better to start offering alternative API with
> GHashTable<utf8,GVariant> or GVariant<a{sv}> then exposing all of this C
> convenience API into the bindings.
> 
> Both of these are probably less of an issue in Vala... thoughts?

The current API works fine in Vala, but so should your proposal.

(In reply to comment #25)
> http://git.collabora.co.uk/?p=user/treitter/telepathy-glib.git;a=commitdiff;h=54062f5e3c0c7ff147c8cb0490bbd3a65c59b50e
> > Don't implicitly transfer ownership of Contacts' or Connections' TpCapabilities in their getter functions.
> 
> That's not what this patch does (and if it did that, it'd be an ABI break). A
> better commit message would be "Annotate that TpCapabilities getter functions
> don't transfer ownership".

Fixed in gi-fixes3

> > Expose Capabilities.get_channel_classes() to g-i.
> 
> Will this break other g-i consumers?

I'm not sure if it causes build failures or just gets ignored (it depends on whether the parser treats it as an error, warning, or nothing).

Danielle, could you address this?

> If not, I'm not against it, but the long commit message seems misleading: you
> can use the higher-level convenience methods on TpCapabilities even if you
> can't use this one, surely?

Sure, I could use higher-level convenience methods, but they currently only tell us whether the capabilities support 1:1 chat or rooms; we'll need to add a convenience function per flag value to not need to access the channel classes.

> > Un-blacklist the tp_asv_get_* utility functions for g-i.
> 
> Does @asv need annotating with (element-type)?

Fixed in gi-fixes3

> I'm not against this, although a parallel GVariant API for a{sv}s would be nice
> too.
> 
> However, please note that the tp_asv_set_* family are not safe for g-i, so stop
> here rather than continuing :-)

Done.

New branch - gi-fixes4. The only difference between this and gi-fixes3 is that gi-fixes4 doesn't expose Capabilities.get_channel_classes() for g-i.

(In reply to comment #28)
> New branch - gi-fixes4. The only difference between this and gi-fixes3 is that
> gi-fixes4 doesn't expose Capabilities.get_channel_classes() for g-i.

This looks harmless, please merge.

Please also make a one-patch branch that exposes get_channel_classes(), but give Danielle a chance to veto that one.

(In reply to comment #29)
> (In reply to comment #28)
> > New branch - gi-fixes4. The only difference between this and gi-fixes3 is that
> > gi-fixes4 doesn't expose Capabilities.get_channel_classes() for g-i.
> 
> This looks harmless, please merge.

Thanks; merged.

> Please also make a one-patch branch that exposes get_channel_classes(), but
> give Danielle a chance to veto that one.

A new trivial branch for this is in the URL now.

> > Please also make a one-patch branch that exposes get_channel_classes(), but
> > give Danielle a chance to veto that one.
> 
> A new trivial branch for this is in the URL now.

If the typelib compiles, it's fine. This was only skipped because it's opaque to g-i and I wanted to replace it with something more useful in the future.

(In reply to comment #31)
> If the typelib compiles, it's fine.

OK, I've cherry-picked it - thanks. Will be in 0.11.11.

In order for build dependencies to be properly calculated for code which uses the tp-glib Vala bindings, we need to install a deps file alongside the vapi file. The branch below does this (and also cleans up vala/Makefile.am a little).

http://git.collabora.co.uk/?p=user/pwith/telepathy-glib;a=shortlog;h=refs/heads/vala-build