(When putting things up for review, please add the patch keyword, and put the branch in the URL field (or attach git-format-patch output if you prefer).)

If you're going to (skip) methods to avoid having to think about whether/how they should be introspected, we should have a way to keep track of which ones are deliberately (skip)'d, and which ones we just haven't thought about yet. I suggest opening one bug for each class visible to introspection.am that's partly (or not at all) annotated.

For instance, the properties on TpBaseConnection should be pretty uncontroversial.

I don't see how you can use TpBaseConnectionManager correctly without calling its register() method, or indeed, without knowing enough about its properties to construct one. Is it actually needed?

You (skip) all of TpBaseProtocol, but I'm reasonably sure it's annotated correctly (or at least, I wrote it after learning about annotations).

>  /**
> - * connection_shutdown_finished_cb:
> + * connection_shutdown_finished_cb: (skip)

This isn't API, so the right fix here would have been to change the /** to /*. Same for list_channel_factory_foreach_one, list_channel_manager_foreach_one.

> - * TP_INTERNAL_CONNECTION_STATUS_NEW:
> + * TP_INTERNAL_CONNECTION_STATUS_NEW: (skip)

This is pretty much necessary to use a TpBaseConnection correctly, so skipping it seems suspect.

>      <method name="destroy" c:identifier="tp_handle_set_destroy">
>        <return-value transfer-ownership="none">
>          <type name="none" c:type="void"/>
>        </return-value>
>      </method>

Should this be in the GIR at all? It's the destructor, so technically it's (transfer full) or (transfer into-a-big-chemical-fire) or something :-)

>       <constructor name="new_from_array" c:identifier="tp_handle_set_new_from_array">
> ...
>           <parameter name="array" transfer-ownership="none">
>             <array name="GLib.Array" c:type="GArray*">
>             </array>

With (element-type uint) a la tp_intset_to_array, this could be annotated correctly.

>      <method name="peek" c:identifier="tp_handle_set_peek">
>        <return-value transfer-ownership="full">
>          <type name="IntSet" c:type="TpIntSet*"/>
>        </return-value>
>      </method>

This doesn't transfer ownership. It needs (transfer none).

>       <method name="foreach" c:identifier="tp_handle_set_foreach">

I'd skip this in favour of peeking at the underlying TpIntSet and iterating it, tbh. Or, if you want it visible, please make the (scope call) explicit. The userdata is meant to be marked as (closure), isn't it?

>      <method name="to_array" c:identifier="tp_handle_set_to_array">
>        <return-value transfer-ownership="full">
>          <array name="GLib.Array" c:type="GArray*">

This should be annotated as (element-type uint) like tp_intset_to_array().

(In reply to comment #1)
> (When putting things up for review, please add the patch keyword, and put the
> branch in the URL field (or attach git-format-patch output if you prefer).)

Sorry.

Updated branch here: http://git.collabora.co.uk/?p=user/pwith/telepathy-glib;a=shortlog;h=refs/heads/test-suite-gir-bindings2

> If you're going to (skip) methods to avoid having to think about whether/how
> they should be introspected, we should have a way to keep track of which ones
> are deliberately (skip)'d, and which ones we just haven't thought about yet. I
> suggest opening one bug for each class visible to introspection.am that's
> partly (or not at all) annotated.
> 
> For instance, the properties on TpBaseConnection should be pretty
> uncontroversial.

Bugs should be filed for the following classes/interfaces then:
 * TpBaseConnection (base-connection.h)
 * TpHandleRepo (handle-set.h)
 * TpHandleSet (handle-set.h)

> I don't see how you can use TpBaseConnectionManager correctly without calling
> its register() method, or indeed, without knowing enough about its properties
> to construct one. Is it actually needed?

I found that one of the classes I was building for the libfolks test suite was entirely redundant. Dropping it means that TpBaseConnectionManager and TpBaseProtocol don't need to be added to tp-glib's Vala bindings.

> You (skip) all of TpBaseProtocol, but I'm reasonably sure it's annotated
> correctly (or at least, I wrote it after learning about annotations).
> 
> >  /**
> > - * connection_shutdown_finished_cb:
> > + * connection_shutdown_finished_cb: (skip)
> 
> This isn't API, so the right fix here would have been to change the /** to /*.
> Same for list_channel_factory_foreach_one, list_channel_manager_foreach_one.

This is all irrelevant now. I've cut the changes out of my branch.

> > - * TP_INTERNAL_CONNECTION_STATUS_NEW:
> > + * TP_INTERNAL_CONNECTION_STATUS_NEW: (skip)
> 
> This is pretty much necessary to use a TpBaseConnection correctly, so skipping
> it seems suspect.

The Connection class we're using has been pinched from tp-glib/examples/cm/contactlist/conn.c, and goes straight to CONNECTED.

> >      <method name="destroy" c:identifier="tp_handle_set_destroy">
> >        <return-value transfer-ownership="none">
> >          <type name="none" c:type="void"/>
> >        </return-value>
> >      </method>
> 
> Should this be in the GIR at all? It's the destructor, so technically it's
> (transfer full) or (transfer into-a-big-chemical-fire) or something :-)

I didn't notice that g-ir-scanner had snuck the TpHandleSet methods into the GIR file. They're all now (skip)ped in the updated branch.

> >       <constructor name="new_from_array" c:identifier="tp_handle_set_new_from_array">
> > ...
> >           <parameter name="array" transfer-ownership="none">
> >             <array name="GLib.Array" c:type="GArray*">
> >             </array>
> 
> With (element-type uint) a la tp_intset_to_array, this could be annotated
> correctly.

Even though this is now (skip)ped, I added the annotation anyway, so as not to waste valuable review time. :-)

> >      <method name="peek" c:identifier="tp_handle_set_peek">
> >        <return-value transfer-ownership="full">
> >          <type name="IntSet" c:type="TpIntSet*"/>
> >        </return-value>
> >      </method>
> 
> This doesn't transfer ownership. It needs (transfer none).

Same.

> >       <method name="foreach" c:identifier="tp_handle_set_foreach">
> 
> I'd skip this in favour of peeking at the underlying TpIntSet and iterating it,
> tbh. Or, if you want it visible, please make the (scope call) explicit. The
> userdata is meant to be marked as (closure), isn't it?

(skip)ped and annotated.

> >      <method name="to_array" c:identifier="tp_handle_set_to_array">
> >        <return-value transfer-ownership="full">
> >          <array name="GLib.Array" c:type="GArray*">
> 
> This should be annotated as (element-type uint) like tp_intset_to_array().

Same.

Note that the members of the TpBaseConnection and TpBaseConnectionClass have made their way into the GIR file, including the private members. I don't think there's any way to avoid this until https://bugzilla.gnome.org/show_bug.cgi?id=590274 is fixed.

TpHandleSetMemberFunc should probably be (skip)'d, because it's only used by a (skip)'d method.

TpBaseConnectionCreateHandleReposImpl isn't introspectable (its second argument isn't parsed correctly - it's an array of TpHandleRepoIface pointers, of length NUM_TP_HANDLE_TYPES, but in the .gir it's a single TpHandleRepoIface pointer). Can it be skipped or treated as opaque or something?

Non-blockers:

You've added (skip) to a lot of static functions which mistakenly had /** */ doc-comments, which should never be necessary, but is harmless I suppose.

I had a go at fixing this branch myself, since I'd like to get a release out soon:

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

(In reply to comment #5)
> I had a go at fixing this branch myself, since I'd like to get a release out
> soon:
> 
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/pwith-gir

Danielle reviewed this and approved, so I've merged to master. Fixed in 0.11.11