With the introduction of Conn.I.Addressing.GetContacts* we need to emulate GetContactAttributes behavior for retrieving contacts in different ways than just handles. This branch exposes the internal implementation so we could wrap it with different contact retrieval methods.
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.