Bug 28200 (TpBaseContactList)

This is ready for a first round of review, I think.

Errata:

* It should probably be called TpBaseContactList

* Everything that takes a GStrv or a const gchar * const * should be using the array + length pattern, but with a signed (gssize) length, with -1 meaning NULL-terminated - this is "the best of both worlds"

* Some of the handle sets would probably be nicer as int sets

Review to f56db201f69bc10768fc365128025a21157acf04 (included):

+  _tp_dynamic_handle_repo_set_normalization_data ((TpHandleRepoIface *) obj,
+      NULL, NULL);
Maybe add a comment saying that's to call the GDestroyNotify on the existing data?

there are 2 FIXME:
+      /* FIXME: borrowed reference */

+  self->priv->groups = g_hash_table_new_full (g_direct_hash, g_direct_equal,
+      NULL, g_object_unref);
+  self->priv->queued_requests = g_hash_table_new_full (g_direct_hash,
+      g_direct_equal, NULL, NULL);
+}

I heard than passing NULL as hash_func and key_equal_func but is more optimized.


I'll continue the review on Monday.

Didn't see anything obvioulsy wrong in the latest commits. The design and implementation is pretty clear; nice work!
Did you already ported a CM to it?

2 small nipticks:

tp_contact_list_manager_groups_created
+              g_ptr_array_add (pa, (gchar *) tp_handle_inspect (
+                    self->priv->group_repo, handle));
+
+            }

Useless \n

+    /* string borrowed from priv->all_tags => itself */
I had to check what the "itself" actually meant (the self pointer or something?) but it seemed obvious once I read the code. :)


I've been pleased to see that Géraldine and me are in this example :)

(In reply to comment #2)
> Review to f56db201f69bc10768fc365128025a21157acf04 (included):
> 
> +  _tp_dynamic_handle_repo_set_normalization_data ((TpHandleRepoIface *) obj,
> +      NULL, NULL);
> Maybe add a comment saying that's to call the GDestroyNotify on the existing
> data?

That's not why I call it: the existing data is a borrowed cyclic ref to the contact list manager, so that GROUP handles can be normalized by calling its virtual method.

When the contact list manager becomes unusable (either when disposed, or when the parent TpBaseConnection disconnects), that dangling pointer needs to be released; the normalization function should no longer be called anyway, but if it is, it'll fall back to just passing group names through as-is.

I've added brief comments.

(In this code I'm trying very hard to avoid the problems Gabble has, with assumptions about objects' lifetimes all over the place.)

> there are 2 FIXME:
> +      /* FIXME: borrowed reference */

The channels assumed that they wouldn't survive longer than the connection and their manager. This is in fact true, but I think it's better style to not have these borrowed references. I've added logic to have circular references for the lifetime of the channels, which are cleaned up when the channels are closed.

> I heard than passing NULL as hash_func and key_equal_func but is more
> optimized.

Fixed.

(In reply to comment #3)
> Didn't see anything obvioulsy wrong in the latest commits. The design and
> implementation is pretty clear; nice work!
> Did you already ported a CM to it?

No, I haven't done that yet. I should port at least one to prove the design before we merge this; I should also demonstrate (on another branch) that it can implement the planned new D-Bus API.

> +                    self->priv->group_repo, handle));
> +
> +            }
> 
> Useless \n

Fixed

> +    /* string borrowed from priv->all_tags => itself */
> I had to check what the "itself" actually meant (the self pointer or
> something?) but it seemed obvious once I read the code. :)

I changed those comments to read "the same pointer" instead of "itself". Is that better?

(In reply to comment #1)
> * It should probably be called TpBaseContactList

I plan to make this change, but I haven't yet, because it's a huge-but-trivial change; I'll go through and do this when you're otherwise happy with it.

> * Everything that takes a GStrv or a const gchar * const * should be using the
> array + length pattern, but with a signed (gssize) length, with -1 meaning
> NULL-terminated - this is "the best of both worlds"

Done; this produces more natural-looking code in the example CM, so I think it's worth the extra code in the library.

> * Some of the handle sets would probably be nicer as int sets

I don't think this is worth it. The major awkwardness of handle sets is that you have to use tp_handle_set_peek() to iterate over them, which is no big deal really.

Your latest patches look good to me.

In the process of implementing the new D-Bus API, I made some fixes to the base class.

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

The history is a little confusing because I had to merge some recent additions from master. It's probably easiest to review these from smcv/contact-list-manager upwards. I think you've reviewed up to and including smcv/contact-list-manager, although you might not have seen the last couple of commits on that branch.

From smcv/contact-list-manager up to smcv/contact-list-merge-master is just a merge from master, because I needed some API I added on master, so you can skip past that (or have a look yourself if you don't trust Zdra ;-)

From smcv/contact-list-merge-master up to smcv/contact-list-fixes is the rename to TpBaseContactList, then some additional cleanup.

I'll base subsequent branches on smcv/contact-list-fixes.

Reviewed to f607d36745b62979e4bc449f397b63859b250f00 : no specific comment.

(In reply to comment #7)
> Reviewed to f607d36745b62979e4bc449f397b63859b250f00 : no specific comment.

Thanks, I've renamed the branch that ended at that commit to contact-list-reviewed. More commits soon...

I've pushed more fixes to smcv/contact-list-manager.

If you're feeling really enthusiastic about reviewing, smcv/contact-list-new-signals and smcv/contact-list-new-methods additionally implement the new APIs.

"""
AuthorizePublication (au: Contacts) → nothing

For contacts with publish=No, [...] it merely records the fact that presence publication to those contacts is allowed; if any of those contacts ask to receive the local user's presence later [...] the connection SHOULD immediately allow them to do so, changing their publish attribute directly from No to Yes.
"""

Note to self: I haven't implemented those semantics in the example yet. When I do, it should probably be rebased in ahead of the new signals and methods.

Other things still to do:

The various accessor methods are inconsistent about whether virtual methods can raise an error. I think they should all never fail, tbh.

It might be good to move various "compliance levels" of groups of virtual methods onto GInterfaces:

  * can list contacts (basic, required functionality - put in the class?)
  * can alter ContactList (all methods mandatory)
  * can list groups (mandatory getters; optional disjoint_groups)
  * can alter ContactGroups (mandatory set/add/remove and get_group_storage; optional rename)
  * can alter Blocking (there seems no point in having a separate read-only version)

That would mean we wouldn't necessarily need the various _implement_...() methods, and tomeu would be happy.

contact-list-manager looks good (reviewed to 9912266df5b75071a2272cf5843a0022134cf346).

I'll probably look at the other branches later.

smcv/contact-list-new-signals
------------------------------

+  gboolean svc_contact_list;
+  gboolean svc_contact_groups;

Add a comment explaining the semantic of these bool and how/why we use it? It's pretty straightforward when reading the branch but that's uncommon enough to be worth a comment I think.

(In reply to comment #9)
> I've pushed more fixes to smcv/contact-list-manager.
> 
> If you're feeling really enthusiastic about reviewing,
> smcv/contact-list-new-signals and smcv/contact-list-new-methods additionally
> implement the new APIs.

I've rewound smcv/contact-list-new-methods a bit, so it only includes the bits that I think will rebase cleanly onto the changes I'm making to contact-list-manager to implement the errata in Comment #9.

(In reply to comment #11)
> smcv/contact-list-new-signals
> ------------------------------
> 
> +  gboolean svc_contact_list;
> +  gboolean svc_contact_groups;
> 
> Add a comment explaining the semantic of these bool and how/why we use it? It's
> pretty straightforward when reading the branch but that's uncommon enough to be
> worth a comment I think.

Fixed, I think.

(In reply to comment #12)
> I've rewound smcv/contact-list-new-methods a bit, so it only includes the bits
> that I think will rebase cleanly onto the changes I'm making to
> contact-list-manager to implement the errata in Comment #9.

Thinking about it, I'll need to modify the new-methods branch significantly, so it's not useful to review yet.

(In reply to comment #9)
> AuthorizePublication (au: Contacts) → nothing
> 
> For contacts with publish=No, [...] it merely records the fact that presence
> publication to those contacts is allowed; if any of those contacts ask to
> receive the local user's presence later [...] the connection SHOULD immediately
> allow them to do so, changing their publish attribute directly from No to Yes.

I've implemented this in smcv/contact-list-manager. Thinking about it, I don't think the contact's state should visibly change when pre-approving like this, so I didn't implement that bit.

> The various accessor methods are inconsistent about whether virtual methods can
> raise an error. I think they should all never fail, tbh.

Fixed in smcv/contact-list-manager.

> It might be good to move various "compliance levels" of groups of virtual
> methods onto GInterfaces:

Fixed in smcv/contact-list-manager.

I reviewed contact-list-new-methods to a0598385b48505be4ecc0579d19d96c4bd7f503b:

One comment:


tp_connection_request_contact_list_attributes's doc says:
"* Return (via a callback) any number of attributes of the given handles, and
  if they are valid and @hold is TRUE, hold a reference to them. "

But we don't pass any handles to this function.

(In reply to comment #13)
> I've implemented this in smcv/contact-list-manager. Thinking about it, I don't
> think the contact's state should visibly change when pre-approving like this,
> so I didn't implement that bit.

... but that's OK, because the spec didn't say that I should either :-)

I've rebased smcv/contact-list-new-signals onto the new API. The only non-automatic changes were to resolve a conflict in this commit:

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=676a5d18a9c2b351649df76dd929336ad9714302

and to assert about ContactsChanged signals in /contact-lists/add-to-publish/pre-approve in this commit:

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=1402e88674e70177de4d2ae5432773bbe2026d4b

May I consider smcv/contact-list-new-signals to have been reviewed, conditional on getting the spec merged? If so, I'll use that as my new baseline for changes.

wip-contact-list-new-methods still needs rebasing, which will be easy but time-consuming; then I can port Gabble to this API, and fix errata in the spec.

(In reply to comment #14)
> I reviewed contact-list-new-methods to
> a0598385b48505be4ecc0579d19d96c4bd7f503b:
> 
> tp_connection_request_contact_list_attributes's doc says:
> "* Return (via a callback) any number of attributes of the given handles, and
>   if they are valid and @hold is TRUE, hold a reference to them. "
> 
> But we don't pass any handles to this function.

Good catch, I'll fix that. I suggest not reviewing further for now - I'll need to fix up that code for the underlying API changes.

(In reply to comment #15)
> (In reply to comment #13)
> > I've implemented this in smcv/contact-list-manager. Thinking about it, I don't
> > think the contact's state should visibly change when pre-approving like this,
> > so I didn't implement that bit.
> 
> ... but that's OK, because the spec didn't say that I should either :-)
> 
> I've rebased smcv/contact-list-new-signals onto the new API. The only
> non-automatic changes were to resolve a conflict in this commit:
> 
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=676a5d18a9c2b351649df76dd929336ad9714302
> 
> and to assert about ContactsChanged signals in
> /contact-lists/add-to-publish/pre-approve in this commit:
> 
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=1402e88674e70177de4d2ae5432773bbe2026d4b
> 
> May I consider smcv/contact-list-new-signals to have been reviewed, conditional
> on getting the spec merged? If so, I'll use that as my new baseline for
> changes.

Yeah that's fine with me.

Let me know when there is more code to review.

I made some changes to the new-methods stuff you already looked at while rebasing:

b650f02441d3fdfaf0c67408a768ce2be6be21d5 needed some minor changes as described in the commit message.

82f04c4909821298ecbea278d239f6ccdf10904c is new.

7804cabfea7b4dbb477e3a95df47f0d577f9dab2 is new.

570465310a06171e986a92bd6bd49e9d8bf9389f needed changes: I amended the last paragraph.

8d5d7baa7603f0b31456c94d566bf86a8d32b26c needed minor merging at the beginning of setup() (tp_tests_dbus_daemon_dup_or_die has a new name since I merged from master), and a different expectation at the end of test_add_to_publish_pre_approve (which makes more changes to the ninja's state now).

ab402ac69352712dc6609fb3849a9406d1caf145 needed changes as noted in the commit message.

e08da21e93093c5089e70d928a3a62b9ce638b90 likewise.

8903924c919b1019bec22b0cc7c86dcac28d9f49 needed a merge in the test.

f88d7731ea56b1ff6e9d37b461f4a6f719a0c689 needed changes as noted in the commit message.

0ad4e102e01872d3f8328361d49bbd48dae193f1 is new.

0d34a29cf18cd04155e35c6b8fee538b233eca73 needed changes as noted in the commit message.

All subsequent commits haven't been looked at yet; they include correcting your complaint from Comment #14.

Sorry, the branch wasn't linked. It's smcv/contact-list-manager.

(In reply to comment #18)
> I made some changes to the new-methods stuff you already looked at while
> rebasing:
> 
> b650f02441d3fdfaf0c67408a768ce2be6be21d5 needed some minor changes as described
> in the commit message.
> 
> 82f04c4909821298ecbea278d239f6ccdf10904c is new.
> 
> 7804cabfea7b4dbb477e3a95df47f0d577f9dab2 is new.
> 
> 570465310a06171e986a92bd6bd49e9d8bf9389f needed changes: I amended the last
> paragraph.
> 
> 8d5d7baa7603f0b31456c94d566bf86a8d32b26c needed minor merging at the beginning
> of setup() (tp_tests_dbus_daemon_dup_or_die has a new name since I merged from
> master), and a different expectation at the end of
> test_add_to_publish_pre_approve (which makes more changes to the ninja's state
> now).
> 
> ab402ac69352712dc6609fb3849a9406d1caf145 needed changes as noted in the commit
> message.
> 
> e08da21e93093c5089e70d928a3a62b9ce638b90 likewise.
> 
> 8903924c919b1019bec22b0cc7c86dcac28d9f49 needed a merge in the test.
> 
> f88d7731ea56b1ff6e9d37b461f4a6f719a0c689 needed changes as noted in the commit
> message.
> 
> 0ad4e102e01872d3f8328361d49bbd48dae193f1 is new.
> 
> 0d34a29cf18cd04155e35c6b8fee538b233eca73 needed changes as noted in the commit
> message.
> 
> All subsequent commits haven't been looked at yet; they include correcting your
> complaint from Comment #14.

I checked these commits and they looked fine to me.

Any reason to return TP_CONTACT_METADATA_STORAGE_TYPE_ANYONE by default rather than NONE?

Didn't see anything wrong in this branch and I'm happy with the comment change you've made to fix my comment.

(In reply to comment #21)
> Any reason to return TP_CONTACT_METADATA_STORAGE_TYPE_ANYONE by default rather
> than NONE?

Connections that can never alter contacts' groups are expected to not implement TP_TYPE_MUTABLE_CONTACT_GROUP_LIST, which results in GetGroupStorage = NONE anyway.

If the default implementation of the virtual method also returned NONE, then implementing TP_TYPE_MUTABLE_CONTACT_GROUP_LIST without also reimplementing get_group_storage would be meaningless - it'd be the same as not implementing TP_TYPE_MUTABLE_CONTACT_GROUP_LIST, but with more boilerplate.

By choosing a default, I reduce the amount of boilerplate slightly for connections where that default is appropriate; I expect ANYONE to be the most common, and it's also the least astonishing.

(There's a similar situation for CanChangeSubscriptions - if you don't implement TP_TYPE_MUTABLE_CONTACT_LIST it's always False, but if you do implement TP_TYPE_MUTABLE_CONTACT_LIST, the default implementation of can_change_subscriptions always returns True.)

Testing with Gabble reveals that TpBaseContactList didn't deal with channel requests correctly; see branch smcv/contact-list-fixes.

Further thoughts on the design based on Gabble: we either need to:

* add async success/error-reporting to all of TpBaseContactList's virtual methods; or

* change Gabble and all other CMs to have an overlay of requested state changes, pretend those changes had taken place for signalling purposes, and revert them if they turn out to fail; or

* change the spec draft to say that the methods "succeed" instantly, changes are signalled later, and failure isn't explicitly signalled.

Looks good except http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=ae0c4aed9b8f878806a51f8e14d9a4ed57a65184

Shouldn't we fix Gabble instead?

The third solution seems the easiest one and would be enough for Empathy which ignores errors  in this cases I think. But ideally I guess it would be good to be able to tell user that the action failed :\

(In reply to comment #24)
> Looks good except
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=ae0c4aed9b8f878806a51f8e14d9a4ed57a65184
> 
> Shouldn't we fix Gabble instead?

Yeah, you're right. I've rebased that patch out of existence, and merged the rest into contact-list-reviewed.

> The third solution seems the easiest one and would be enough for Empathy which
> ignores errors  in this cases I think. But ideally I guess it would be good to
> be able to tell user that the action failed :\

I think the first is the most correct. I'll try implementing it and see how I get on...

Any thoughts on the spec errata, and possible tristate -> 5-state change, in Bug #21787? If you like the look of those changes, I'll also update TpBaseContactList to use them.

(In reply to comment #25)
> Any thoughts on the spec errata, and possible tristate -> 5-state change, in
> Bug #21787?

smcv/contact-list-manager integrates the errata, but not the change from tristate to 5-state.

Changes awaiting review in smcv/contact-list-manager:

* new utility method: tp_simple_async_report_success_in_idle
* updated spec incorporating the RequestContactList rename and other errata
* API change: all "write" virtual methods are now asynchronous
* API change: new mandatory get_group_members, set_group_members_async methods

At some point we could relax either set_group_members_async or the (add_to_group_async, remove_from_group_async) pair to be optional-to-implement with a default implementation in terms of the other, but I don't think requiring subclasses to implement both is onerous: in Gabble it took 91 lines (including comments etc.) to implement both get_group_members and set_group_members.

It's much easier for a subclass to iterate over a concrete data model than it is for TpBaseContactList to fake it.

Possible extra work on this implementation:

* If we mandate that during the call to tp_base_contact_list_groups_removed(), get_group_members() still reflects the previous state, then we could avoid peeking into the TpGroupMixin. This needs minor changes to Gabble and the example CM.

* Similar for tp_base_contact_list_groups_renamed().

* Similar for tp_base_contact_list_emulate_rename_group().

(In reply to comment #27)
> * If we mandate that during the call to tp_base_contact_list_groups_removed(),
> get_group_members() still reflects the previous state, then we could avoid
> peeking into the TpGroupMixin. This needs minor changes to Gabble and the
> example CM.
> 
> * Similar for tp_base_contact_list_groups_renamed().
> 
> * Similar for tp_base_contact_list_emulate_rename_group().

I've now done all of these, and made minor changes to the Gabble branch from Bug #28199 to keep up.

Why did you change those?
-              g_timeout_add_full (G_PRIORITY_DEFAULT,
+              g_timeout_add_full (G_PRIORITY_LOW,


Some functions document that they can't be called before the contact list has been retrieved but doesn't
  g_return_val_if_fail (self->priv->had_contact_list, NULL);
This include:

tp_base_contact_list_get_group_members
tp_base_contact_list_get_blocked_contacts
tp_base_contact_list_get_groups
tp_base_contact_list_get_contact_groups

From Bug #21787:

> (Having said that, TpBaseContactList doesn't yet allow subclasses to signal
> that retrieval of the contact list failed, except by disconnecting the
> Connection; I'll put that on my to-do list for telepathy-glib, but it's not
> directly relevant to the spec.)

(In reply to comment #29)
> Why did you change those?
> -              g_timeout_add_full (G_PRIORITY_DEFAULT,
> +              g_timeout_add_full (G_PRIORITY_LOW,

Those timeouts emulate network activity; I dropped their priority to get deterministic results when they race with the default-priority idle used by the GSimpleAsyncResult. In reality, network activity would be considerably slower.

(In reply to comment #29)
> Some functions document that they can't be called before the contact list has
> been retrieved but doesn't
>   g_return_val_if_fail (self->priv->had_contact_list, NULL);
> This include:
> 
> tp_base_contact_list_get_group_members
> tp_base_contact_list_get_blocked_contacts
> tp_base_contact_list_get_groups
> tp_base_contact_list_get_contact_groups

Fixed in my contact-list-manager branch.

smcv/contact-list-draft3 additionally updates to my current spec proposal, and implements what I said in comment #30.

To do:

The contactlist example CM isn't particularly realistic: it behaves as if incoming publish requests cause the contact to appear on our roster. It would be more realistic to have a separate list of requests, and have the Telepathy contact list be the union of the protocol roster and the requests list.

Calling Unpublish() on a pending request would remove the contact from the requests list, and therefore from the Telepathy contact list (assuming we hadn't done anything that would cause them to be on our roster, like setting an alias or putting them in a group).

Also to do:

Methods that create groups as a side-effect (like adding members to a nonexistent group) don't call the create_groups_async virtual method. I think that's OK - for robustness, the subclass ought to auto-create groups anyway - but would appreciate confirmation from reviewers.

The create_groups_async virtual method and remove_group_async virtual methods are asymmetric (plural vs. singular). create_groups_async is never actually called except when CreateChannel or EnsureChannel is called for a GROUP ContactList channel, so we could make it singular.

Alternatively, CreateChannel and EnsureChannel could call set_group_members_async(group, []) or add_to_group_async(group, []) (which the subclass has to support anyway, because they can be called from D-Bus), removing the need for an explicit create method.

> To do:
>
> The contactlist example CM isn't particularly realistic: it behaves as if
> incoming publish requests cause the contact to appear on our roster. It would
> be more realistic to have a separate list of requests, and have the Telepathy
> contact list be the union of the protocol roster and the requests list.
>
> Calling Unpublish() on a pending request would remove the contact from the
> requests list, and therefore from the Telepathy contact list (assuming we
> hadn't done anything that would cause them to be on our roster, like setting an
> alias or putting them in a group).

> Methods that create groups as a side-effect (like adding members to a
> nonexistent group) don't call the create_groups_async virtual method. I think
> that's OK - for robustness, the subclass ought to auto-create groups anyway -
> but would appreciate confirmation from reviewers.
I guess I'll have to try using it before I can really comment on this.
My initial thoughts are that on protocols where creating empty groups is a no-op, calling create_groups_async() is just adding complication to the bindings. Also, if tp-glib includes auto-create groups logic, it might be expected to have auto-remove groups logic too, which is probably more difficult to get right in a generic way?

> The create_groups_async virtual method and remove_group_async virtual methods
> are asymmetric (plural vs. singular). create_groups_async is never actually
> called except when CreateChannel or EnsureChannel is called for a GROUP
> ContactList channel, so we could make it singular.
>
> Alternatively, CreateChannel and EnsureChannel could call
> set_group_members_async(group, []) or add_to_group_async(group, []) (which the
> subclass has to support anyway, because they can be called from D-Bus),
> removing the need for an explicit create method.
Is the create method something that's going to disappear in telepathy-1.0 or something? If so, avoiding it entirely sounds like the way forwards. Again, I think implementing a CM or two and finding the edge cases is going to be more useful than lots of discussion here.


In other news: Trivial things that made me have to think when reviewing your code:
d979ef8b54643fecc0717467aa6a75db0c968809
+      GPtrArray *pa;
doesn't really tell me what you're about to do with it. s/pa/new_interfaces/ or something? You use that variable name in a few places in base-contact-list.c, but nowhere else in tp-glib.

Otherwise, contact-list-manager^^..contact-list-draft3 looks good. Will start actually trying to use it tomorrow.

Branch updated as far as d9902722b7794fc35007fc10328cf83b55f0313d. I'll update my Gabble branch (Bug #28199) to match.

(In reply to comment #35)
> > The contactlist example CM isn't particularly realistic: it behaves as if
> > incoming publish requests cause the contact to appear on our roster. It would
> > be more realistic to have a separate list of requests, and have the Telepathy
> > contact list be the union of the protocol roster and the requests list.
> >
> > Calling Unpublish() on a pending request would remove the contact from the
> > requests list, and therefore from the Telepathy contact list (assuming we
> > hadn't done anything that would cause them to be on our roster, like setting an
> > alias or putting them in a group).

I've made this change, and updated the test's assertions to match.

> > Methods that create groups as a side-effect (like adding members to a
> > nonexistent group) don't call the create_groups_async virtual method. I think
> > that's OK - for robustness, the subclass ought to auto-create groups anyway -
> > but would appreciate confirmation from reviewers.
> I guess I'll have to try using it before I can really comment on this.
> My initial thoughts are that on protocols where creating empty groups is a
> no-op, calling create_groups_async() is just adding complication to the
> bindings.

Right, in practice it was easier to have it be part of AddToGroup, SetGroupMembers and SetContactGroups; in protocols that need to do actual work to create a group, these methods can all call a helper function. I've made this change.

> Also, if tp-glib includes auto-create groups logic, it might be
> expected to have auto-remove groups logic too, which is probably more difficult
> to get right in a generic way?

Auto-removing groups doesn't make sense: it is often possible to have a group with no members, and on protocols where it isn't, we should emulate it within the lifetime of the session to make UIs' lives easier.

(Consider: I have Alice in group A and Bob in group B. I want to exchange them, in a UI with drag-and-drop. I drag Alice to group B, and group A is empty for a moment, then I drag Bob to group A. If deletion of group A had been signalled to the UI, there'd be nowhere to drop Bob.)

> > Alternatively, CreateChannel and EnsureChannel could call
> > set_group_members_async(group, []) or add_to_group_async(group, []) (which the
> > subclass has to support anyway, because they can be called from D-Bus),
> > removing the need for an explicit create method.
> Is the create method something that's going to disappear in telepathy-1.0 or
> something? If so, avoiding it entirely sounds like the way forwards. Again, I
> think implementing a CM or two and finding the edge cases is going to be more
> useful than lots of discussion here.

I've implemented this behaviour (I used add_to_group_async, but documented both the methods I mentioned as having to create groups automatically), and deleted create_groups_async.

> In other news: Trivial things that made me have to think when reviewing your
> code:
> d979ef8b54643fecc0717467aa6a75db0c968809
> +      GPtrArray *pa;
> doesn't really tell me what you're about to do with it. s/pa/new_interfaces/ or
> something? You use that variable name in a few places in base-contact-list.c,
> but nowhere else in tp-glib.

I've replaced all uses of this variable name.

(In reply to comment #36)
> > > Alternatively, CreateChannel and EnsureChannel could call
> > > set_group_members_async(group, []) or add_to_group_async(group, []) (which the
> > > subclass has to support anyway, because they can be called from D-Bus),
> > > removing the need for an explicit create method.
> > Is the create method something that's going to disappear in telepathy-1.0 or
> > something? If so, avoiding it entirely sounds like the way forwards.

It's currently necessary to be able to say to an implementation "create this group, with no other side-effects", to implement CreateChannel and EnsureChannel for GROUP channels. However, when we remove the old Chan.T.ContactList channels, and just use these new Conn.I.ContactList and Conn.I.ContactGroups interfaces, the need to do that will mostly go away.

There's currently no Conn.I.ContactGroups.CreateGroup(s: Group) in the D-Bus API, because I don't think anyone will call it, and if someone does need to create a group without members, it's trivial to call AddToGroup(group, []) or SetGroupMembers(group, []) (depending on the behaviour they want if they race with someone else creating the same group). See also the 42nd comment in Bug #21787.

I believe David has reviewed as far as 5c18a27e9200bf1874cc15d807c3d009050b3f3b, with no criticisms except for this:

(In reply to comment #35)
> +      GPtrArray *pa;
> doesn't really tell me what you're about to do with it. s/pa/new_interfaces/ or
> something? You use that variable name in a few places in base-contact-list.c,
> but nowhere else in tp-glib.

which is fixed in a later patch, and IMO is very minor. Accordingly, I've moved contact-list-reviewed to point to that commit.

Subsequent branches for review are contact-list-draft3 and contact-list-the-revenge, each of which breaks API to keep up with changes I propose to make in telepathy-spec.

Okay: thoughts while implementing TpMutableContactListInterface->request_subscription_async

Needing to handle multiple contacts at the same time is a pain in the ass, if
you have to do everything async *before* calling the callback, as the API asks
you to (requires looping and counting the number of async calls you make). What
is the use-case for adding multiple contacts with the same message? (to be
honest, it's not really that much work, because you probably need to do this
for modifying groups etc too. It's just that I implemented this method first,
so I didn't have the helper-functions in place for it.)

Also, doing it GAsyncResult-based means that your function has two pointers
that get passed into it. If you immediately convert that into a
GSimpleAsyncResult, it brings it down to one, but I can't help thinking it
would be simpler to be passed in an opaque token for the request (which could
easily be a pointer to a GSimpleAsyncResult, with the agreement that you will
call tp_mutable_contact_list_request_subscription_finished (self, token); when
you're done.

Also, I can't think of a use-case for implementing a finish function,
especially if the whole point of the GAsyncResult API is that it doesn't
guarantee that it will ever be called. If you want to do cleanup, you can
either finish not in an idle, or do it in the function you pass into
set_op_res_gpointer().

Was going to wait until I'd implemented all of TpMutableContactListInterface
before commenting, but it's probably best to give feedback as I go along in
reality. More to come later.


While I was here, I added myself to the CC list and reviewed up to 4fd1fd62354a1abb12143792993ce66bd540e69a (contact-list-the-revenge).

Looks good.

(In reply to comment #39)
> Needing to handle multiple contacts at the same time is a pain in the ass, if
> you have to do everything async *before* calling the callback, as the API asks
> you to (requires looping and counting the number of async calls you make). What
> is the use-case for adding multiple contacts with the same message?

At the D-Bus level, the "change" methods are:

* RequestSubscription: being plural is only useful if you're in a protocol like SIP/SIMPLE where the contact list is transient. Empathy could store the contact list locally (in libfolks?), and drop it into both AuthorizePublication and RequestSubscription every time you connect. On the other hand, perhaps we want to declare that connection managers for SIMPLE should store your contact list in the XDG_DATA_HOME anyway?

* AuthorizePublication: same as RequestSubscription, but being plural is also useful if you want to apply an "auto-authorize people we're subscribed to" UI policy. Initially you GetContactListAttributes, filter for people we're subscribed to, and drop them back into AuthorizePublication; on changes, you filter and send back in the same way. If that's your UI's policy, then the "pre-authorization" semantics of AuthorizePublication make it work exactly like you want it to :-)

* RemoveContacts: being plural is nonessential

* Unsubscribe, Unpublish: should be rare anyway

* SetContactGroups: not plural for contacts

* SetGroupMembers: has to be plural to be useful!

* AddToGroup, RemoveFromGroup: nonessential

* RemoveGroup, RenameGroup: not plural, but when implemented on a protocol where they're not an atomic network operation, the effects are necessarily plural

So to implement SetGroupMembers, RemoveGroup and RenameGroup, you need operation-counting anyway.

For each change, we can classify protocols into two sets:

1. the change is done per-contact

2. the change is done atomically

XMPP is in set 1 for all operations, but other protocols might not be.

If we don't implement methods as plural at both the D-Bus level and the C level, we lose the opportunity to make changes atomically - we'd essentially make all protocols look like set 1, even if they could be in set 2.

Perhaps it'd be worth making the list (but not group) operations singular in C, I don't know. If we do, we should probably make them singular on D-Bus too?

I don't think it's really all that onerous to count changes, though - Gabble now has gabble_simple_async_counter_new(), which we could provide in telepathy-glib if people like it enough.

> Also, doing it GAsyncResult-based means that your function has two pointers
> that get passed into it.

Not our bug; this is how GAsyncResult works, and being consistent with other GObject libraries (potentially gaining "free" CM bindings later) is important.

> If you immediately convert that into a
> GSimpleAsyncResult, it brings it down to one, but I can't help thinking it
> would be simpler to be passed in an opaque token for the request (which could
> easily be a pointer to a GSimpleAsyncResult, with the agreement that you will
> call tp_mutable_contact_list_request_subscription_finished (self, token); when
> you're done.

Part of the point of GAsyncResult is that not all async results need to be Simple - you can subclass GSimpleAsyncResult to encapsulate extra state if you need to, or make your own implementation of GAsyncResult that encapsulates all the extra state you're tracking. gabble_simple_async_counter_new() is a simplistic version of this (but it abuses the result pointer, which it happens to not need, to be the counter).

> Also, I can't think of a use-case for implementing a finish function,
> especially if the whole point of the GAsyncResult API is that it doesn't
> guarantee that it will ever be called.

The finish function pulls out the "return" from the async result. It's necessary to support non-Simple async results, and is conventional; bindings can presumably safely require it.

In our case, the finish function is very simple, because in the successful case, we "return void". The same's true for (for instance) GAsyncInitable, though.

> If you want to do cleanup, you can
> either finish not in an idle

No, one of the simplifications made by GAsyncResult is that callbacks are always called from the (calling thread's) main loop, never synchronously. The only place where it's acceptable to finish not-in-an-idle is if you know for sure that you're being called from the calling thread's main loop (which basically means you're finishing a GAsyncResult from another GAsyncResult's callback).

> or do it in the function you pass into
> set_op_res_gpointer().

... or in the dispose/finalize of your GAsyncResult implementation, if you reimplemented it.

> https://bugs.freedesktop.org/show_bug.cgi?id=28200
> 
> --- Comment #40 from Simon McVittie <simon.mcvittie@collabora.co.uk>
> 2010-08-04 03:02:12 PDT --- (In reply to comment #39)
> 
> > Needing to handle multiple contacts at the same time is a pain in the
> > ass, if you have to do everything async *before* calling the callback,
> > as the API asks you to (requires looping and counting the number of
> > async calls you make). What is the use-case for adding multiple contacts
> > with the same message?
> 
> At the D-Bus level, the "change" methods are:
> 
> * RequestSubscription: being plural is only useful if you're in a protocol
> like SIP/SIMPLE where the contact list is transient. Empathy could store
> the contact list locally (in libfolks?), and drop it into both
> AuthorizePublication and RequestSubscription every time you connect. On
> the other hand, perhaps we want to declare that connection managers for
> SIMPLE should store your contact list in the XDG_DATA_HOME anyway?
 
Ah, okay. That makes sense now. Thanks.

> Perhaps it'd be worth making the list (but not group) operations singular
> in C, I don't know. If we do, we should probably make them singular on
> D-Bus too?
> 
> I don't think it's really all that onerous to count changes, though -
> Gabble now has gabble_simple_async_counter_new(), which we could provide
> in telepathy-glib if people like it enough.

Oh, it's not a problem. I guess it's just a case of making sure there is a 
use-case for it not being as simple as I thought it could be.

> > Also, doing it GAsyncResult-based means that your function has two
> > pointers that get passed into it.
> 
> Not our bug; this is how GAsyncResult works, and being consistent with
> other GObject libraries (potentially gaining "free" CM bindings later) is
> important.

I understand the thing of getting "free" GAsyncResult based bindings on the 
client side. I don't understand how this would help you write a tp-glib-based 
CM in (for example) python. Are there also plans to help you implement 
GAsyncResult-based interfaces in other languages? I'd assumed there wouldn't 
be (it seems like a less trivial task, with fewer use-cases). 

If there are plans to help with this too, then ++ times a million on using 
GAsyncResult. Otherwise, it seems a little bit more complicated than an api of 
the form
tp_mutable_contact_list_request_subscription_finished (self, token)/
tp_mutable_contact_list_request_subscription_failed (self, token, error)
with no real gain. That's the only point I was trying to make.

Something I didn't spot, but the guy reviewing my code did:

    get_contacts() returns a copy of the handle set,
    but according to the telepathy coding style get, "get" methods should not
    copy the returned value.  Methods that return a copy should be named "dup".

(In reply to comment #42)
> Something I didn't spot, but the guy reviewing my code did:
> 
>     get_contacts() returns a copy of the handle set,
>     but according to the telepathy coding style get, "get" methods should not
>     copy the returned value.  Methods that return a copy should be named "dup".

Well spotted, I'll fix that. I think I wrote that part of the coding style :-/

(In reply to comment #39)
> While I was here, I added myself to the CC list and reviewed up to
> 4fd1fd62354a1abb12143792993ce66bd540e69a (contact-list-the-revenge).

I've merged that into contact-list-reviewed.

(In reply to comment #42)
>     get_contacts() returns a copy of the handle set,
>     but according to the telepathy coding style get, "get" methods should not
>     copy the returned value.

I renamed all these to _dup_ in new patches in contact-list-the-revenge.

commits since contact-list-reviewed look good.

I've added one more commit, which fixes some documentation cross-references.

... and another which ports to TpBaseChannel. 47 insertions(+), 292 deletions(-) :-)

(It would have been an even better diffstat, but contact list channels are weird and need to be able to raise errors from Close().)

Having got Gabble to a reviewable state again with decent test coverage, without further alterations to the base class, I'm pretty happy with this API.

(In reply to comment #47)
> ... and another which ports to TpBaseChannel. 47 insertions(+), 292
> deletions(-) :-)
> 
> (It would have been an even better diffstat, but contact list channels are
> weird and need to be able to raise errors from Close().)

This comment confused me until I saw the one that you removed further down the file.
+/* We don't use this: #TpBaseChannelClass.close doesn't allow us to fail to
+ * close, which is a quirk of the old ContactList design. */

-  /* close is implemented in subclasses, so don't IMPLEMENT (close); */

How about:

/* We don't use this: #TpBaseChannelClass.close doesn't allow us to fail to
 * close, which is a quirk of the old ContactList design, so subclasses
 * IMPLEMENT (close) manually. */

[/pedantry]

But yeah: looks good. Ship it.

I've made your suggested change in contact-list-reviewed. This is now only blocked by undrafting the spec.

Fixed in git, will be in 0.13.0 shortly.