Bug 26249

The branch danni/js seems to contain some preliminary work on this:

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

I don't think we want the Javascript-specific part right now, just the GObject Introspection for the high-level API, so the commits should be cherry-picked. I'll clone this bug with lower priority for the Javascript part.

> Initial work generating JS bindings for the Telepathy API

Can you shuffle this stuff onto a separate branch, and for the moment, just take the GObject-Introspection bits?

> Rewrite GArray_* and GHashTable_* in the scrubber

Aren't you meant to be able to do this with annotations? I think it's something like:

    (type GArray) (element-type uint)

> proxy-introspectable.h is a dirty hack to make TpProxy method calls work

Similarly, aren't you meant to be able to use (type TpProxy) to force the issue?

(In reply to comment #2)
> > Initial work generating JS bindings for the Telepathy API
> 
> Can you shuffle this stuff onto a separate branch, and for the moment, just
> take the GObject-Introspection bits?

Yes. Was thinking of doing that.

> > Rewrite GArray_* and GHashTable_* in the scrubber
> 
> Aren't you meant to be able to do this with annotations? I think it's something
> like:
> 
>     (type GArray) (element-type uint)

So, from #introspection (type GArray) works -- just undocumented, so I didn't know about it. GArrays don't support (element-type) which I'm going to file as a bug.

> > proxy-introspectable.h is a dirty hack to make TpProxy method calls work
> 
> Similarly, aren't you meant to be able to use (type TpProxy) to force the
> issue?

This doesn't work. Probably could be considered a bug.

(In reply to comment #3)
> (In reply to comment #2)
> > > Initial work generating JS bindings for the Telepathy API
> > 
> > Can you shuffle this stuff onto a separate branch, and for the moment, just
> > take the GObject-Introspection bits?
> 
> Yes. Was thinking of doing that.

http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=shortlog;h=refs/heads/gobject-introspection

> > > Rewrite GArray_* and GHashTable_* in the scrubber
> > 
> > Aren't you meant to be able to do this with annotations? I think it's something
> > like:
> > 
> >     (type GArray) (element-type uint)
> 
> So, from #introspection (type GArray) works -- just undocumented, so I didn't
> know about it. GArrays don't support (element-type) which I'm going to file as
> a bug.

https://bugzilla.gnome.org/show_bug.cgi?id=615231

> > > proxy-introspectable.h is a dirty hack to make TpProxy method calls work
> > 
> > Similarly, aren't you meant to be able to use (type TpProxy) to force the
> > issue?
> 
> This doesn't work. Probably could be considered a bug.

https://bugzilla.gnome.org/show_bug.cgi?id=615233

(In reply to comment #3)

> So, from #introspection (type GArray) works -- just undocumented, so I didn't
> know about it. GArrays don't support (element-type) which I'm going to file as
> a bug.

Also, it was documented. Apparently I just can't read :-/

Useful references: http://live.gnome.org/GObjectIntrospection/Annotations and http://live.gnome.org/GObjectIntrospection/WritingBindingableAPIs

Please say in NEWS in this branch that gtk-doc 1.14 is strongly recommended (or even just increase the dependency). I fixed a bug that would make it truncate annotated doc-comments at the first newline, which broke much of our documentation; 1.14 has that fix.

> +       sed -re 's/gpointer self/TpProxy *self/' < proxy.h > proxy-introspectable.h

Why are you using --regexp-extended? This looks unnecessary (and probably not portable); just use sed -e 's/.../'.

Please put the temporary proxy-introspectable.h in _gen so it's git-ignored.

> +TelepathyGLib-1.0.gir: $(INTROSPECTION_SCANNER) libtelepathy-glib.la

Shouldn't this be called TelepathyGLib-0.12.gir?

> 2138  * tp_account_get_current_presence:
> 2139  * @account: a #TpAccount
> 2140  * @status: (out) (transfer none): return location for the current status
> 2141  * @status_message: (out) (transfer none): return location for the current status message

These strings are (transfer full).

>     * TpChannel::group-members-changed:

Perhaps (skip) this one, and just bind group-members-changed-detailed?

> JS example that retrieves the statuses of valid accounts

Please put this in tests/manual/ or something (if it's intended as a manual regression test), or examples/client/ (if it's good example code).

Is it good JS style to use semicolons or not? You seem to have omitted the semicolon on the import of TelepathyGLib. We should probably pick a Javascript coding convention (i.e. steal the one from either GJS or Seed) and document it on /Style on the wiki (a simple reference like "for Javascript, use <a ...>the GJS coding conventions</a>" would be perfect).

You seem to be passing null as user_data to all the async functions in JS. Could you make this unnecessary by annotating the user-data as (closure)?

>   * tp_account_update_parameters_finish:
...
> + * @reconnect_required: (out) (transfer none): a #GStrv to fill with
> + *  properties that need a reconnect to take effect

This GStrv is (transfer full).

Should the C type of this parameter be GStrv *reconnect_required? Would that produce better bindings by default?

>  * tp_account_manager_get_most_available_presence:
>  * @manager: a #TpAccountManager
>- * @status: a string to fill with the actual status
>- * @message: a string to fill with the actual status message
>+ * @status: (out) (transfer none): a string to fill with the actual status
>+ * @message: (out) (transfer none) :a string to fill with the actual status
>+ *  message

The two strings are (transfer full). Also, the second colon in @message should be swapped with the preceding space.

>   * tp_account_get_requested_presence:
>   * @account: a #TpAccount
> - * @status: return location for the requested status
> - * @status_message: return location for the requested status message
> + * @status: (out) (transfer none): return location for the requested status
> + * @status_message: (out) (transfer none): return location for the requested
> + *  status message

The strings are (transfer full).

>  * tp_asv_size:

I think this could be (skip)ped? It's only there because g_hash_table_size isn't const-correct.

> - * tp_asv_take_object_path:
> + * tp_asv_take_object_path: (skip)
>   * @asv: a #GHashTable created with tp_asv_new()
>   * @key: string key
> - * @value: value
> + * @value: (transfer full): value

I don't think it makes sense to use (transfer full) here? It's really a hypothetical new transfer type, (transfer steal) perhaps, which can't be bound, and the function is (skip)ped anyway.

> 1822  * tp_asv_get_strv:
...
> 1836  * Returns: (transfer none): the %NULL-terminated string-array value of @key,
> 1837  *  or %NULL

I think this should be annotated as (type Strv), or (array zero-terminated=1) (element-type utf8), or however you spell that?

>   * tp_asv_set_strv:
...
> + * @value: (in) (transfer none): a %NULL-terminated string array

Would it be better if we used "GStrv value" as the C declaration?

> +          try {
> +            let params = account.get_parameters();
> +            print("params = " + params);
> +            Tp.asv_dump(params);

I'd be happier about the quality of our introspection if this iterated over the array printing the GValues, however you do that in Javascript (some sort of equivalent of g_strdup_value_contents).

(In reply to comment #6)
> Useful references: http://live.gnome.org/GObjectIntrospection/Annotations and
> http://live.gnome.org/GObjectIntrospection/WritingBindingableAPIs
> 
> Please say in NEWS in this branch that gtk-doc 1.14 is strongly recommended (or
> even just increase the dependency). I fixed a bug that would make it truncate
> annotated doc-comments at the first newline, which broke much of our
> documentation; 1.14 has that fix.

I bumped the dependency. Seems like the right choice.

> > +       sed -re 's/gpointer self/TpProxy *self/' < proxy.h > proxy-introspectable.h
> 
> Why are you using --regexp-extended? This looks unnecessary (and probably not
> portable); just use sed -e 's/.../'.

Habit. Fixed.

> Please put the temporary proxy-introspectable.h in _gen so it's git-ignored.

Fixed.

> > +TelepathyGLib-1.0.gir: $(INTROSPECTION_SCANNER) libtelepathy-glib.la
> 
> Shouldn't this be called TelepathyGLib-0.12.gir?

I don't know TBH. It has a separate version field inside it. The 1.0 seems to refer to an ABI version?

> > 2138  * tp_account_get_current_presence:
> > 2139  * @account: a #TpAccount
> > 2140  * @status: (out) (transfer none): return location for the current status
> > 2141  * @status_message: (out) (transfer none): return location for the current status message
> 
> These strings are (transfer full).

Fixed. Whoops.

> >     * TpChannel::group-members-changed:
> 
> Perhaps (skip) this one, and just bind group-members-changed-detailed?
> 
> > JS example that retrieves the statuses of valid accounts
> 
> Please put this in tests/manual/ or something (if it's intended as a manual
> regression test), or examples/client/ (if it's good example code).

They're not really intended as either at the moment. I'm not entirely sure if we should merge them or not. I suppose I should write proper regression tests instead.

> Is it good JS style to use semicolons or not? You seem to have omitted the
> semicolon on the import of TelepathyGLib. We should probably pick a Javascript
> coding convention (i.e. steal the one from either GJS or Seed) and document it
> on /Style on the wiki (a simple reference like "for Javascript, use <a ...>the
> GJS coding conventions</a>" would be perfect).

This was a mistake that for some reason the interpreter didn't pick up. Fixed.

There doesn't seem to be a strong convention yet. Maybe gnome-shell has a document we can borrow though.

> You seem to be passing null as user_data to all the async functions in JS.
> Could you make this unnecessary by annotating the user-data as (closure)?

Yes probably. I copied this basic format from gio-cat.js in the gjs examples.

> >   * tp_account_update_parameters_finish:
> ...
> > + * @reconnect_required: (out) (transfer none): a #GStrv to fill with
> > + *  properties that need a reconnect to take effect
> 
> This GStrv is (transfer full).

Whoops. I really should have read the source code, instead of the docstring. We should probably make ownership explicit in the docstring. Fixed.

> Should the C type of this parameter be GStrv *reconnect_required? Would that
> produce better bindings by default?

I've forced the type in the annotation for now.

> >  * tp_account_manager_get_most_available_presence:
> >  * @manager: a #TpAccountManager
> >- * @status: a string to fill with the actual status
> >- * @message: a string to fill with the actual status message
> >+ * @status: (out) (transfer none): a string to fill with the actual status
> >+ * @message: (out) (transfer none) :a string to fill with the actual status
> >+ *  message
> 
> The two strings are (transfer full). Also, the second colon in @message should
> be swapped with the preceding space.

Fixed.

> >   * tp_account_get_requested_presence:
> >   * @account: a #TpAccount
> > - * @status: return location for the requested status
> > - * @status_message: return location for the requested status message
> > + * @status: (out) (transfer none): return location for the requested status
> > + * @status_message: (out) (transfer none): return location for the requested
> > + *  status message
> 
> The strings are (transfer full).

Fixed.

> >  * tp_asv_size:
> 
> I think this could be (skip)ped? It's only there because g_hash_table_size
> isn't const-correct.

Perhaps. Need to look into dicts more in general.

> > - * tp_asv_take_object_path:
> > + * tp_asv_take_object_path: (skip)
> >   * @asv: a #GHashTable created with tp_asv_new()
> >   * @key: string key
> > - * @value: value
> > + * @value: (transfer full): value
> 
> I don't think it makes sense to use (transfer full) here? It's really a
> hypothetical new transfer type, (transfer steal) perhaps, which can't be bound,
> and the function is (skip)ped anyway.

That's not meant to be there. Fixed.

> > 1822  * tp_asv_get_strv:
> ...
> > 1836  * Returns: (transfer none): the %NULL-terminated string-array value of @key,
> > 1837  *  or %NULL
> 
> I think this should be annotated as (type Strv), or (array zero-terminated=1)
> (element-type utf8), or however you spell that?

Fixed. (type GLib.Strv)

> >   * tp_asv_set_strv:
> ...
> > + * @value: (in) (transfer none): a %NULL-terminated string array
> 
> Would it be better if we used "GStrv value" as the C declaration?

Also fixed.

> > +          try {
> > +            let params = account.get_parameters();
> > +            print("params = " + params);
> > +            Tp.asv_dump(params);
> 
> I'd be happier about the quality of our introspection if this iterated over the
> array printing the GValues, however you do that in Javascript (some sort of
> equivalent of g_strdup_value_contents).

I haven't yet found a way to iterate through a hash table yet. Certainly this works:

  print("$$ account = " + params["account"]);
  if (params["account"] == "danielle.madeley@collabora.co.uk")
    print("wooooh");

Which implies that our tp_asv_get_* functions are actually unrequired (need to build a test for all data types I suppose).

Ok. This works to iterate:

  for (let key in params) {
     print("key = " + key + ", value = " + params[key]);
  }

Some more open issues:

https://bugzilla.gnome.org/show_bug.cgi?id=615923 -- g-i fails to parse TpContactFeatures enum

https://bugzilla.gnome.org/show_bug.cgi?id=615924 -- lengths required in gjs JS API

https://bugzilla.gnome.org/show_bug.cgi?id=615932 -- can't call Tp.Channel.group_get_members()

Another issue:

https://bugzilla.gnome.org/show_bug.cgi?id=616004 -- chan.borrow_immutable_properties(): Error: No introspection information found for GStrv

(In reply to comment #9)

> https://bugzilla.gnome.org/show_bug.cgi?id=615932 -- can't call
> Tp.Channel.group_get_members()

This was due to a missing boxed type. There may be a few other boxed types required for structs in tp-glib. TpConnectionManagerProtocol for instance.

I've added TpIntSet as a boxed type in git master. If you need more, please add them using a similar patch, or file bugs requesting that they be added.

Another bug in g-i: https://bugzilla.gnome.org/show_bug.cgi?id=616375

This one prevents us from (skip)ing properties that contain things we don't want to expose (specifically DBusGConnection).

Also filed https://bugzilla.gnome.org/show_bug.cgi?id=616376 which would be nice for binding some of our API.

Can we work around GNOME #616375 like this?

* Provide a very small dummy dbus-glib GIR file with exactly the types we need

* Use a Python script (or bring back the XSLT...) to delete the offending references from *our* .gir file before compiling or installing it

(In reply to comment #15)
> Can we work around GNOME #616375 like this?
> 
> * Provide a very small dummy dbus-glib GIR file with exactly the types we need
> 
> * Use a Python script (or bring back the XSLT...) to delete the offending
> references from *our* .gir file before compiling or installing it

Another option might be a:

#ifdef _GOBJECT_INTROSPECTION_
typedef DBusGConnection gpointer
#endif

Or to use the --inject flag to g-ir-scanner.

Will look into this.

Ok. I've now removed the weird DBusGLib-1.0 requirement. The workaround I used in the end was an XSLT identity template.

I've renamed the gir/typelib to Telepathy-0.12, this can be bumped when we break ABI I guess. The other option is Telepathy-0.0.

Review would be awesome.

I don't think we should include the three tests included as separate commits at the end of the branch. Instead I should write some proper unit tests (next goal).

It seems from Danielle and Travis's work that g-i bindings and Vala bindings are currently separate. Let's use this bug for g-i only, and open another for Vala.

Merge blockers
==============

In "Initial work on generating gobject-introspection":

> +       LD_LIBRARY_PATH=.libs$${LD_LIBRARY_PATH:+:$$LD_LIBRARY_PATH} \

If you invoke it using $(top_builddir)/libtool --mode=execute $(INTROSPECTION_COMPILER) ... (like we do for valgrind in the regression tests), you shouldn't need to set LD_LIBRARY_PATH by hand.

>   * tp_asv_set_boolean:

I think it's dangerous to bind these setters into other languages - they make assumptions about the memory allocation model of the GHashTable that are not, in general, true. (Also, we don't bind tp_asv_new, which is the only documented way to create a suitable hash table :-)

> -               --pkg dbus-glib-1 \

Do we want to keep the dbus-glib-1 pkg-config package, even though we've dropped the GObject-Introspection-level dependency? I think that would avoid needing @DBUS_CFLAGS@?

> JS example that retrieves the statuses of valid accounts
> Connection Manager example
> Contact list example that makes a low-level D-Bus call

I don't think we should ship these at all, for now.

In future, examples that are exemplary should go in examples/client/ (or perhaps in examples/client/js/), and non-automated tests should go in tests/manual/ or something, to make it very obvious that unlike the rest of the tests, they are not run automatically.

Not merge blockers
==================

In "Initial work on generating gobject-introspection":

> +       proxy.c _gen/proxy-introspectable.h \
> +       account.c account.h \
> +       account-manager.c account-manager.h \
> +       connection.c connection.h \
> +       connection-handles.c \
> +       connection-manager.c connection-manager.h \
> +       channel.c channel.h \
> +       handle.c handle.h \
> +       dbus-daemon.c dbus-daemon.h \
> +       interfaces.c interfaces.h \
> +       intset.c intset.h \
> +       dbus.c dbus.h \
> +       capabilities.c capabilities.h \
> +       contact.c contact.h \
> +       defs.h \
> +       debug.c debug.h \
> +       _gen/telepathy-enums.h \
> +       _gen/telepathy-interfaces.h

I'd still like a brief status report for each of those files: unchecked / completely OK / only foo and bar checked / whatever.

It might help to look at the generated .gir and see if it looks sane.

>     * TpChannel::group-members-changed:

Why is this already re-annotated in the initial commit? Also, weren't you going to (skip) it?

>    * TpChannel::group-members-changed-detailed:

Likewise, but without the (skip)ping.

> @@ -738,8 +738,8 @@ connection_got_contact_attributes (TpConnection *self,
>   * @user_data: arbitrary user-supplied data

Why is this already re-annotated in the initial commit? Also, user_data could probably be annotated as (closure).

> Annotate TpProxy

Have you checked all of TpProxy's API? If not, "Annotate tp_proxy_prepare_async" would seem a more appropriate commit message.

> Annotate TpChannel
>  342  * tp_channel_borrow_connection:

Shouldn't methods that duplicate a property be (skip)ped? I consider this method to be a "C binding" :-)

> + * Returns: (transfer none): a borrowed reference to the #TpContact:connection

I think this is a "C binding" and so should be (skip)ped? In fact, most of the TpContact stuff falls into that category.

> + * @contacts: (array length=n_contacts): An array of @n_contacts TpContact
> + *  objects (this callback is
>   *  not given a reference to any of these objects, and must call
>   *  g_object_ref() on any that it will keep), or %NULL on unrecoverable errors

Shouldn't this have an explicit (transfer none)? Likewise for requested_ids, failed_id_errors?

> + * tp_get_bus_proxy: (skip)

All other deprecated functions should probably be (skip)ped, too.

I'd quite like to see a straight Javascript port of the existing client examples (inspect-connection and friends) in examples/.

(In reply to comment #19)
> Merge blockers
> ==============
> 
> In "Initial work on generating gobject-introspection":
> 
> > +       LD_LIBRARY_PATH=.libs$${LD_LIBRARY_PATH:+:$$LD_LIBRARY_PATH} \
> 
> If you invoke it using $(top_builddir)/libtool --mode=execute
> $(INTROSPECTION_COMPILER) ... (like we do for valgrind in the regression
> tests), you shouldn't need to set LD_LIBRARY_PATH by hand.

Fixed.

> >   * tp_asv_set_boolean:
> 
> I think it's dangerous to bind these setters into other languages - they make
> assumptions about the memory allocation model of the GHashTable that are not,
> in general, true. (Also, we don't bind tp_asv_new, which is the only documented
> way to create a suitable hash table :-)

Ok. I think we should skip all tp_asv_* functions.

> > -               --pkg dbus-glib-1 \
> 
> Do we want to keep the dbus-glib-1 pkg-config package, even though we've
> dropped the GObject-Introspection-level dependency? I think that would avoid
> needing @DBUS_CFLAGS@?

You're right. Fixed.

> > JS example that retrieves the statuses of valid accounts
> > Connection Manager example
> > Contact list example that makes a low-level D-Bus call
> 
> I don't think we should ship these at all, for now.

Agreed.

> In future, examples that are exemplary should go in examples/client/ (or
> perhaps in examples/client/js/), and non-automated tests should go in
> tests/manual/ or something, to make it very obvious that unlike the rest of the
> tests, they are not run automatically.

Agreed.

> Not merge blockers
> ==================
> 
> In "Initial work on generating gobject-introspection":
> 
> > +       proxy.c _gen/proxy-introspectable.h \
> > +       account.c account.h \
> > +       account-manager.c account-manager.h \
> > +       connection.c connection.h \
> > +       connection-handles.c \
> > +       connection-manager.c connection-manager.h \
> > +       channel.c channel.h \
> > +       handle.c handle.h \
> > +       dbus-daemon.c dbus-daemon.h \
> > +       interfaces.c interfaces.h \
> > +       intset.c intset.h \
> > +       dbus.c dbus.h \
> > +       capabilities.c capabilities.h \
> > +       contact.c contact.h \
> > +       defs.h \
> > +       debug.c debug.h \
> > +       _gen/telepathy-enums.h \
> > +       _gen/telepathy-interfaces.h
> 
> I'd still like a brief status report for each of those files: unchecked /
> completely OK / only foo and bar checked / whatever.

Will build a list.

> It might help to look at the generated .gir and see if it looks sane.
> 
> >     * TpChannel::group-members-changed:
> 
> Why is this already re-annotated in the initial commit? Also, weren't you going
> to (skip) it?

It was annotated to make the original commit compile. Will (skip) instead.

> >    * TpChannel::group-members-changed-detailed:
> 
> Likewise, but without the (skip)ping.

Same reason.

> > @@ -738,8 +738,8 @@ connection_got_contact_attributes (TpConnection *self,
> >   * @user_data: arbitrary user-supplied data
> 
> Why is this already re-annotated in the initial commit? Also, user_data could
> probably be annotated as (closure).

Same reason. This method doesn't actually work anyway.

> > Annotate TpProxy
> 
> Have you checked all of TpProxy's API? If not, "Annotate
> tp_proxy_prepare_async" would seem a more appropriate commit message.

So far everything I've tried is introspected correctly.

> > Annotate TpChannel
> >  342  * tp_channel_borrow_connection:
> 
> Shouldn't methods that duplicate a property be (skip)ped? I consider this
> method to be a "C binding" :-)

I don't know what the G-I policy is here. We should ask.

> > + * Returns: (transfer none): a borrowed reference to the #TpContact:connection
> 
> I think this is a "C binding" and so should be (skip)ped? In fact, most of the
> TpContact stuff falls into that category.
> 
> > + * @contacts: (array length=n_contacts): An array of @n_contacts TpContact
> > + *  objects (this callback is
> >   *  not given a reference to any of these objects, and must call
> >   *  g_object_ref() on any that it will keep), or %NULL on unrecoverable errors
> 
> Shouldn't this have an explicit (transfer none)? Likewise for requested_ids,
> failed_id_errors?

This method doesn't actually work. Hence the bug to replace them with GIO-style API. I'm not sure whether or not to skip currently broken API or not. If it doesn't work it's not really part of our API guarantee at the moment anyway.

> > + * tp_get_bus_proxy: (skip)
> 
> All other deprecated functions should probably be (skip)ped, too.

Ok. There were only 3 functions in debug.c not skipped.

> I'd quite like to see a straight Javascript port of the existing client
> examples (inspect-connection and friends) in examples/.

(In reply to comment #20)
> > >   * tp_asv_set_boolean:
> > 
> > I think it's dangerous to bind these setters into other languages - they make
> > assumptions about the memory allocation model of the GHashTable that are not,
> > in general, true. (Also, we don't bind tp_asv_new, which is the only documented
> > way to create a suitable hash table :-)
> 
> Ok. I think we should skip all tp_asv_* functions.

Hmm, the only one I'm wondering is how people are going to set the object-path variant.

(In reply to comment #19)

> I'd still like a brief status report for each of those files: unchecked /
> completely OK / only foo and bar checked / whatever.

Ok. I've compiled a list of all of the function calls in the introspection http://telepathy.freedesktop.org/wiki/GObjectIntrospection

I'll go through it and write in what's been annotated implicitly/explicitly and what's been explicitly tested. It's also my goal to write unit tests, so we can also document what has unit tests.

I'll make a branch that's your branch with the example/test-things omitted, and see if I can get it merged for the next version so we have some sort of baseline to work with.

(In reply to comment #20)
> Ok. I think we should skip all tp_asv_* functions.

It's not really necessary to skip tp_asv_get_* - those are perfectly safe - but we can always reinstate them later.

> I'm not sure whether or not to skip currently broken API or not. If it
> doesn't work it's not really part of our API guarantee at the moment anyway.

I think we should (skip) anything that's *known* to be broken, with a comment above the doc-comment saying "GObject-Introspection known not to work". Not a merge blocker, though.

(In reply to comment #21)
> Hmm, the only one I'm wondering is how people are going to set the object-path
> variant.

Perhaps one way forward would be to have a struct that wraps a GHashTable-with-known-allocation-model, like so:

    typedef struct { /*<private>*/ GHashTable *asv; } TpAsvBuilder;

    TpAsvBuilder *builder = tp_asv_builder_new ();
    GHashTable *asv;

    tp_asv_builder_set_int64 (builder, "discord", 5);
    tp_asv_builder_set_uint32 (builder, "towel", 42);
    asv = tp_asv_builder_finish (builder);
    tp_asv_builder_destroy (builder);
    do_something_with (asv);
    g_hash_table_unref (asv);

(With hindsight, I should probably have insisted that tp_asv_set_foo work like this...)

(In reply to comment #22)
> I'll go through it and write in what's been annotated implicitly/explicitly and
> what's been explicitly tested. It's also my goal to write unit tests, so we can
> also document what has unit tests.

Brilliant, thank you.

Here's a version of Danielle's branch that I'd be willing to merge.

Unfortunately, I had to work around <https://bugzilla.gnome.org/show_bug.cgi?id=615550> in a rather ugly way. When we can depend on gtk-doc 1.15 (i.e. when it's been released), we can get rid of the workaround.

(In reply to comment #23)

> Perhaps one way forward would be to have a struct that wraps a
> GHashTable-with-known-allocation-model, like so:
> 
>     typedef struct { /*<private>*/ GHashTable *asv; } TpAsvBuilder;
> 
>     TpAsvBuilder *builder = tp_asv_builder_new ();
>     GHashTable *asv;
> 
>     tp_asv_builder_set_int64 (builder, "discord", 5);
>     tp_asv_builder_set_uint32 (builder, "towel", 42);
>     asv = tp_asv_builder_finish (builder);
>     tp_asv_builder_destroy (builder);
>     do_something_with (asv);
>     g_hash_table_unref (asv);
> 
> (With hindsight, I should probably have insisted that tp_asv_set_foo work like
> this...)

I did a test where you have to pass an a{sv} map (create_account_async) and things seem to work perfectly with the map created in JS. I think we can leave out all a{sv} mangling.

N.B. still haven't tested passing an object path in a map, can't think of a place where you have to do this as a client.

Updated my branch with some further annotation fixes.

https://bugzilla.gnome.org/show_bug.cgi?id=616815 --  GStrv not usable as struct field

I've merged as much as we have so far. Repurposing this bug from "add" to "stabilize".

Created attachment 35385 [details] [review]
Adds garray parameter annotations, and skips tp_g_value_slice_* functions

(In reply to comment #29)
> Created an attachment (id=35385) [details]
> Adds garray parameter annotations, and skips tp_g_value_slice_* functions

Now that GObject-Introspection 0.6.11 is available to be depended on, I've merged this; it'll be released in tp-glib 0.11.6.

For future reference, here's my review of that patch:

* technically correct
* several lines had trailing whitespace, which I've removed (git can be
  configured to complain about "whitespace errors", which is recommended)
* in future, please err on the side of having more, smaller patches: the
  (skip) bits could have been merged already if you hadn't committed them in
  the same patch as the GArray bits

Comment on attachment 35385 [details] [review]
Adds garray parameter annotations, and skips tp_g_value_slice_* functions

I can't see anything needing review here (Morten's patch has been applied), so I've removed the patch keyword. Please re-add it, or file a bug that blocks this one, if more g-i changes are needed.

I don't think having this bug open benefits anyone: individual g-i bugfixes can be individual bugs.