[Bug 27792] add Vala bindings

Thu Jun 17 13:18:36 CEST 2010

https://bugs.freedesktop.org/show_bug.cgi?id=27792

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review- but worth merging
                   |                            |fairly soon
           Keywords|                            |patch

--- Comment #10 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-06-17 04:18:35 PDT ---
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?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.