Bug 30310

Probably need a docstring there. Coming soon.

commit 49bed1c04d79dd9449d1357e52d2a94fafe30a11
Author: Eitan Isaacson <eitan@monotonous.org>
Date:   Tue Sep 21 10:25:35 2010 -0700

    Added docstring to tp_contacts_mixin_get_contacts_attributes.
    Suffixed DBus method with _impl.

My long-running contact-lists branch has a more hackish version of this as library-internal API, but yours seems sufficiently good to be real API, which is nice.

> 266 tp_contacts_mixin_get_contacts_attributes (GObject *obj,

contact_attributes not contacts_attributes, please.

It should also g_return_val_if_fail that obj is a TpBaseConnection with a TpContactsMixin.

Please decide on a policy for how this function deals with disconnected connections, and document it. Possibilities are:

- raise a GError and document that it's OK to call it disconnected, but it will fail
- g_return_val_if_fail and document that the connection must still be connected

If you make it raise a GError, you could also not ignore the error from tp_handles_client_hold (which can't happen when telepathy-glib calls it, but could happen from another caller).

I think it's worth having an extra, nullable assumed_interface argument, which ContactList and Addressing can use to avoid having to copy their caller-supplied interfaces list in order to always include the relevant interface.

> And non-valid handles will be

Any invalid handles will be

> + * will be held by this client.

"held on behalf of this client, as if via tp_handles_client_hold()" would be better?

> + * Provide attributs for all requested

attributes

(In reply to comment #1)
> Probably need a docstring there. Coming soon.

... where by "probably need" you mean "to pass 'make check && make distcheck', it will need"; it'll also need adding to the -sections.txt to pass checks. In general, every public API function should be documented (and added to -sections.txt) by the same patch that introduces it.

> +tp_contacts_mixin_get_contacts_attributes (GObject *obj,
> +    const GArray *handles, const gchar **interfaces, const gchar *sender)

One argument per line, please (I know the existing code had this wrong).

The newly public method either needs the (skip) annotation, or correct GObject-Introspection annotations, or both. I'd be inclined to (skip) it as not useful for introspection, but annotations are often useful documentation anyway. I think it's something like:

    @handles: (element-type guint): ...
    @interfaces: (array zero-terminated=1): ...
    @sender: (allow-none): ...

    Returns: (element-type guint GLib.HashTable): ...

(In reply to comment #3)
> My long-running contact-lists branch has a more hackish version of this as
> library-internal API, but yours seems sufficiently good to be real API, which
> is nice.
> 
> > 266 tp_contacts_mixin_get_contacts_attributes (GObject *obj,
> 
> contact_attributes not contacts_attributes, please.
> 

6861220

> It should also g_return_val_if_fail that obj is a TpBaseConnection with a
> TpContactsMixin.
> 

ba4d915
My brain stopped working, it's too late here. Not sure if I am checking for the mixin correctly. Looked like the most straightforward way.

> Please decide on a policy for how this function deals with disconnected
> connections, and document it. Possibilities are:
> 
> - raise a GError and document that it's OK to call it disconnected, but it will
> fail
> - g_return_val_if_fail and document that the connection must still be connected
> 
> If you make it raise a GError, you could also not ignore the error from
> tp_handles_client_hold (which can't happen when telepathy-glib calls it, but
> could happen from another caller).

Let's just assume it's connected.
72ac056

> 
> I think it's worth having an extra, nullable assumed_interface argument, which
> ContactList and Addressing can use to avoid having to copy their
> caller-supplied interfaces list in order to always include the relevant
> interface.

ccab5f4

> 
> > And non-valid handles will be
> 
> Any invalid handles will be
> 
> > + * will be held by this client.
> 
> "held on behalf of this client, as if via tp_handles_client_hold()" would be
> better?
> 
> > + * Provide attributs for all requested
> 
> attributes
> 

92707b9

> (In reply to comment #1)
> > Probably need a docstring there. Coming soon.
> 
> ... where by "probably need" you mean "to pass 'make check && make distcheck',
> it will need"; it'll also need adding to the -sections.txt to pass checks. In
> general, every public API function should be documented (and added to
> -sections.txt) by the same patch that introduces it.
> 

e10fad6

I'll squash all these changes into one patch with a good commit message before merging.

> > +tp_contacts_mixin_get_contacts_attributes (GObject *obj,
> > +    const GArray *handles, const gchar **interfaces, const gchar *sender)
> 
> One argument per line, please (I know the existing code had this wrong).
> 

ccab5f4

> The newly public method either needs the (skip) annotation, or correct
> GObject-Introspection annotations, or both. I'd be inclined to (skip) it as not
> useful for introspection, but annotations are often useful documentation
> anyway. I think it's something like:
> 
>     @handles: (element-type guint): ...
>     @interfaces: (array zero-terminated=1): ...
>     @sender: (allow-none): ...
> 
>     Returns: (element-type guint GLib.HashTable): ...

e10fad6

review+, thanks. I'll probably merge this myself, since TpBaseContactList should use it.

Merge, thanks!

(In reply to comment #6)
> Merge, thanks!

If that meant "Please merge", please don't mark bugs RESOLVED until the bug is actually fixed in master.

If that meant "I merged it", you didn't :-)

Some non-trivial merging later, see referenced URL.

(In reply to comment #7)
> (In reply to comment #6)
> > Merge, thanks!
> 
> If that meant "Please merge", please don't mark bugs RESOLVED until the bug is
> actually fixed in master.
> 
> If that meant "I merged it", you didn't :-)

Sorry! I changed this bug when I meant to change bug 30094.
That was sloppy.

Fixed in git, will be in 0.13.0 shortly.