Bug 38248

Here is a spec addition proposed: http://cgit.collabora.com/git/user/xclaesse/telepathy-spec.git/log/?h=channel-contacts

Hum, actually thinking about this twice, and this is either not enough, or useless.

If we go that way, that means we still needs to introspect sets of handles to know if the contacts are member or local/remove-pending. If we have to introspect them anyway, we could just in TpChannel make the union of all those sets and call tp_connection_get_contact_attributes() once.

Also in the case the channel does not have specific handles, it means we would be getting attributes for contacts we probably already have from the connection anyway.

What could really be valuable though, is having a{us} (handle-id maps) instead of au for members properties, like that we can create a TpContact right away. SelfHandleChanged also needs to give the ID. But that's really for telepathy 7.0 :)

Should I close INVALID?

I'm looking into this again, and we really need (handle,id) pairs to create TpContact, IMO.

On Channel main interface we are good because we have (TargetHandle, TargetID) and (InitiatorHandle, InitiatorID) pairs and they are immutable. So the best is probably to not touch that interface at all.

On Group interface though, we have issues because none of those properties gives the corresponding ids: HandleOwners, LocalPendingMembers, Members, RemotePendingMembers and SelfHandle. Also SelfHandleChanged and HandleOwnersChanged does not give ids. The good news is MembersChangedDetailed already gives the ids.

From this, how do we fix Group interface?

1) extend current properties: Add HandleOwnersIds, LocalPendingMembersIds (contains also actors), MembersIds, RemotePendingMembersIds, SelfHandleId properties (with type a{us}). Also add HandleOwnersChangedDetailed and SelfHandleChangedDetailed which gives the ids via a a{us} or even a a{sv} just like the details in MembersChangedDetailed.

2) like 1, but introduce only 1 new property ids:a{us} which gives the mapping for all handles in all properties (including also local-pending actors). HandleOwnersChangedDetailed and SelfHandleChangedDetailed would still be needed.

3) deprecate current properties and create new ones that gives directly the ids. For example Members2 : a{us}. Note that local-pending would also give id for the actor.

4) resurect GetAllMembers but gives contact attributes, just like GetContactListAttributes. Note that would still need a solution for SelfHandle and HandleOwners.

5) suggestions?

if we goes for 3) or 4) and since we already have lots of deprecated stuff, maybe create a new iface SimpleGroup ?

My personal preference is solution 4 (consistent with ContactList iface) together with additional SelfID:s and OwndersID:a{us} properties (consistent with Channel iface) and their respective Detailed signal which gives an a{sv} (consistent with MembersChangedDetailed).

(In reply to comment #3)
> My personal preference is solution 4 (consistent with ContactList iface)
> together with additional SelfID:s and OwndersID:a{us} properties (consistent
> with Channel iface) and their respective Detailed signal which gives an a{sv}
> (consistent with MembersChangedDetailed).

Oh, forgot that I've actually had something against that:

(In reply to comment #2)
> Also in the case the channel does not have specific handles, it means we would
> be getting attributes for contacts we probably already have from the connection
> anyway.


Maybe the less intrusive change would just to add a new property identifiers:a{us} giving the mapping from handles to id for all handles on the group interface...

(In reply to comment #4)
> (In reply to comment #3)
> > My personal preference is solution 4 (consistent with ContactList iface)
> > together with additional SelfID:s and OwndersID:a{us} properties (consistent
> > with Channel iface) and their respective Detailed signal which gives an a{sv}
> > (consistent with MembersChangedDetailed).
> 
> Oh, forgot that I've actually had something against that:
> 
> (In reply to comment #2)
> > Also in the case the channel does not have specific handles, it means we would
> > be getting attributes for contacts we probably already have from the connection
> > anyway.
> 
> 
> Maybe the less intrusive change would just to add a new property
> identifiers:a{us} giving the mapping from handles to id for all handles on the
> group interface...

I think adding a new property, Group.MemberIdentifiers: a{us}y, containing the handle→identifier map of all members of any kind on the channel, would be enough? I guess additions to this could be signalled before any other signal that introduces a handle, such as Group.SelfHandleChanged.

It might also be nice if, when you call GetContactAttributes() for a channel-specific handle, you get an attribute mapping them to their real handle. This could be exposed as another TpContact in tp-glib. Maybe also GetContactAttributes would return all the attributes for the handle's owner, too?

Ok, made a patch around that idea: http://cgit.collabora.com/git/user/xclaesse/telepathy-spec.git/log/?h=channel-ids

comments?

And here is a branch for tp-glib to implement new spec in group-mixin.c and create TpContact objects on TpChannel: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=channel-ids

spec branch updated as per smcv's comments

(In reply to comment #7)
> And here is a branch for tp-glib to implement new spec in group-mixin.c and
> create TpContact objects on TpChannel:
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=channel-ids

Branch rebased on master, now using the factory to create TpContact. It has an issue making some unit tests fail:

TpChannel constructor gets actor handle/id from immutable-properties, so it creates a TpContact right away. Though if the TpConnection was not prepared before, it does not yet know it has immutable-handles => can't create the TpContact from handle/id pair.

To avoid races, TpChannel will need to delay contact creation until TpConnection has CORE prepared.

Another question: atm all TpContact objects are created as part of TpChannel's CORE feature because we can create them without extra round-trip. Though, I would like to add a feature TP_CHANNEL_FEATURE_CONTACTS that delay exposing the contacts until all desired contact features set on the factory are prepared on them. Or should I only create TpContact objects if that feature is prepared?

(In reply to comment #9)
> (In reply to comment #7)
> > And here is a branch for tp-glib to implement new spec in group-mixin.c and
> > create TpContact objects on TpChannel:
> > http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=channel-ids
> 
> Branch rebased on master, now using the factory to create TpContact. It has an
> issue making some unit tests fail:
> 
> TpChannel constructor gets actor handle/id from immutable-properties, so it
> creates a TpContact right away. Though if the TpConnection was not prepared
> before, it does not yet know it has immutable-handles => can't create the
> TpContact from handle/id pair.
> 
> To avoid races, TpChannel will need to delay contact creation until
> TpConnection has CORE prepared.

Now fixed

*** Bug 26171 has been marked as a duplicate of this bug. ***

tp-glib branch now updated with new feature TP_CHANNEL_FEATURE_CONTACTS that delays signals until contact features are prepared.

First we should merge the spec branch (set in URL) then reassign (or open new bug?) to tp-glib for implementation both service and client side.

Everything is ready for review :)

Spec looks good. But how is a client supposed to know whether or not the new signals will be emitted? In your tp-glib branch you're emitting the new ones after the old ones, so… yeah, it's just “some properties are randomly NULL”.

I hate to say it, but at least throw in a new group flag so that Empathy or whatever can just say “no, I refuse to deal with this old CM” or at least spit something in its debug log. Hey, you could make TP_CHANNEL_FEATURE_CONTACTS fail to prepare in this situation.

+++ b/telepathy-glib/channel-contacts.c
@@ -0,0 +1,546 @@
+/*
+ * channel.c - proxy for a Telepathy channel (contacts feature)

That's not what the file is called.

+  /**
+   * TpChannel:target-contact:
+   *
+   * Same as #TpChannelIface:handle but gives a #TpContact instead of
+   * handle if #TpChannelIface:handle-type is %TP_HANDLE_TYPE_CONTACT, %NULL
+   * otherwise.
+   *
+   * If the Connection Manager is too old, could return %NULL.
+   *
+   * Since: 0.UNRELEASED

This documentation is really unhelpful: it doesn't tell you what the property actually means, it just links to an even less helpfully documented property on some random interface! How about:

  If this channel is for communication with a single contact (that is, #TpChannelIface:handle-type is %TP_HANDLE_TYPE_CONTACT), then a #TpContact representing the remote contact. For chat rooms, contact search channels and other channels without a single remote contact, %NULL.

  If TP_CHANNEL_FEATURE_CONTACTS is not prepared, this property will be %NULL.

You can remove the stuff about the CM being too old, and add to the documentation for TP_CHANNEL_FEATURE_CONTACTS:

  On older connection managers, this feature may fail to prepare.

I haven't finished reviewing this branch. I'll pick it back up on Monday.

(In reply to comment #13)
> Spec looks good. But how is a client supposed to know whether or not the new
> signals will be emitted? In your tp-glib branch you're emitting the new ones
> after the old ones, so… yeah, it's just “some properties are randomly NULL”.

I guess tp-glib could detect how modern the CM is by the presence or absence of the MemberIdentifiers property in the GetAll reply.

(In reply to comment #14)
> (In reply to comment #13)
> > Spec looks good. But how is a client supposed to know whether or not the new
> > signals will be emitted? In your tp-glib branch you're emitting the new ones
> > after the old ones, so… yeah, it's just “some properties are randomly NULL”.
> 
> I guess tp-glib could detect how modern the CM is by the presence or absence of
> the MemberIdentifiers property in the GetAll reply.

Yep, that's already what my code does, see the comment in tp_channel_got_group_properties_cb()

+      /* Removed handles are not in identifiers table because we are supposed
+       * to already know them. Search the contact in our tables */
+      contact = g_hash_table_lookup (self->priv->group_members_contacts, key);
+      if (contact == NULL)
+          contact = g_hash_table_lookup (
+              self->priv->group_local_pending_contacts, key);
+      if (contact == NULL)
+          contact = g_hash_table_lookup (
+              self->priv->group_remote_pending_contacts, key);
+
+      if (contact == NULL)
+        continue;

This is a bit suspect: this means that contacts just won't be signalled as being removed. You could just change TpGroupMixin to always include IDs in the signal. It's not like any other CMs implement contact-ids.

I guess this is common to this whole function though: if the CM is broken, contacts will get randomly omitted from signals.


+      if (actor_contact != NULL)
+        {
+          info = g_hash_table_lookup (self->priv->group_local_pending_info, key);
+          info->actor_contact = g_object_ref (actor_contact);
+        }

Why is info guaranteed to be non-NULL?

My comment about useless docstrings applies equally to all the other documentation that begins “Same [h]as foo”.

+
+              if (info->message != NULL)
+                m = info->message;

The check is redundant.

tp_channel_group_get_contact_owner() is not documented to be able to return NULL. Is your intention that it should ever return NULL?

+      G_PARAM_READABLE | G_PARAM_STATIC_NAME | G_PARAM_STATIC_BLURB |
+      G_PARAM_STATIC_NICK);

G_PARAM_STATIC_STRINGS.

+  TpHandle handle;
+  GArray handles = { (gchar *) &handle, 1 };

What on earth? It would be so much clearer to build the GArray properly, right after initializing 'handle' and just before passing the GArray to tp_cli_channel_interface_group_call_add_members().

The title of “Add unit tests for TpChannel contacts” made me optimistic that there would be maybe more than one test. It should test more than just a single contact in the 'added' list. Oh, and the very next commit removes most of that test!

First:

@@ -575,6 +575,56 @@ test_join_room (Test *test,
   g_assert_no_error (test->error);
 }
 
+static void
+group_contacts_changed_cb (TpChannel *self,
+    GPtrArray *added,
+    GPtrArray *removed,
+    GPtrArray *local_pending,
+    GPtrArray *remote_pending,
+    TpContact *actor,
+    GHashTable *details,
+    Test *test)
+{
+  g_assert (added->len == 1);
+  g_assert_cmpstr (tp_contact_get_identifier (g_ptr_array_index (added, 0)),
+      ==, "badger");
+  g_assert (removed->len == 0);
+  g_assert (local_pending->len == 0);
+  g_assert (remote_pending->len == 0);
+  g_assert_cmpstr (tp_contact_get_identifier (actor), ==, "me@test.com");
+
+  test->wait--;
+  if (test->wait <= 0)
+    g_main_loop_quit (test->mainloop);
+}

Followed by:

@@ -585,13 +585,7 @@ group_contacts_changed_cb (TpChannel *self,
     GHashTable *details,
     Test *test)
 {
-  g_assert (added->len == 1);
-  g_assert_cmpstr (tp_contact_get_identifier (g_ptr_array_index (added, 0)),
-      ==, "badger");
-  g_assert (removed->len == 0);
-  g_assert (local_pending->len == 0);
-  g_assert (remote_pending->len == 0);
-  g_assert_cmpstr (tp_contact_get_identifier (actor), ==, "me@test.com");
+  g_print ("contact added: %s\n", tp_contact_get_identifier (g_ptr_array_index (added, 0)));

+    GQueue *contacts_changed_queue;

Any particular reason for using a GQueue * and not a GQueue? NBD, just wondered.

+static void
+contacts_upgraded_cb (TpConnection *connection,
…
+  g_simple_async_result_complete (result);
+}

I think you're missing an unref.

What happens if a new message arrives from a contact while we're still preparing their TpContact? Does the message get signalled before the member change notification? If so, that's a bug.

Relatedly, can we expect the result of tp_signalled_message_get_sender() to be prepared? Consider getting a message from someone who's not currently in the chat room.

+  /* If we are still preparing the initial set of contacts,
+   * wait until its done */

it's.

+  /* If no contacts needs to be upgraded, item is ready */

No contacts need to be.

+  /* Collect all TpContact we have for this channel */

“Collect all the TpContacts we have for this channel”.

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Spec looks good. But how is a client supposed to know whether or not the new
> > > signals will be emitted? In your tp-glib branch you're emitting the new ones
> > > after the old ones, so… yeah, it's just “some properties are randomly NULL”.
> > 
> > I guess tp-glib could detect how modern the CM is by the presence or absence of
> > the MemberIdentifiers property in the GetAll reply.
> 
> Yep, that's already what my code does, see the comment in
> tp_channel_got_group_properties_cb()

So to be clear, I'm suggesting:
• if the CM is too old, TP_CHANNEL_FEATURE_CONTACTS should fail to prepare;
• this should be documented;
• the properties and methods relating to it should be documented to return NULL/FALSE/whatever until/unless CONTACTS is prepared.

(In reply to comment #16)
> +      /* Removed handles are not in identifiers table because we are supposed
> +       * to already know them. Search the contact in our tables */
> +      contact = g_hash_table_lookup (self->priv->group_members_contacts, key);
> +      if (contact == NULL)
> +          contact = g_hash_table_lookup (
> +              self->priv->group_local_pending_contacts, key);
> +      if (contact == NULL)
> +          contact = g_hash_table_lookup (
> +              self->priv->group_remote_pending_contacts, key);
> +
> +      if (contact == NULL)
> +        continue;
> 
> This is a bit suspect: this means that contacts just won't be signalled as
> being removed. You could just change TpGroupMixin to always include IDs in the
> signal. It's not like any other CMs implement contact-ids.
> 
> I guess this is common to this whole function though: if the CM is broken,
> contacts will get randomly omitted from signals.

Spec explicitly says they can be omitted, with a rational. This can only happen on broken CMs anyway so I'll just add a DEBUG() telling it.

> 
> +      if (actor_contact != NULL)
> +        {
> +          info = g_hash_table_lookup (self->priv->group_local_pending_info,
> key);
> +          info->actor_contact = g_object_ref (actor_contact);
> +        }
> 
> Why is info guaranteed to be non-NULL?

because it should have been created in handle_members_changed() just before. But you're right that if another signal that removes the local-pending handle happened while we were still preparing the local-pending contact, then it could have been removed from the table already... I'll add a if (info!=NULL) to be safe.

> +
> +              if (info->message != NULL)
> +                m = info->message; 
> 
> The check is redundant.

Why is it redundant? if info->message is NULL it should leave m to "". This is copy/paste from tp_channel_group_get_local_pending_info().

> tp_channel_group_get_contact_owner() is not documented to be able to return
> NULL. Is your intention that it should ever return NULL?

As per the spec: "Handles which are channel-specific, but for which the owner is unknown, MUST appear in this mapping with 0 as owner." In that case, it will return NULL. Needs to be documented, indeed.

> +  TpHandle handle;
> +  GArray handles = { (gchar *) &handle, 1 };
> 
> What on earth? It would be so much clearer to build the GArray properly, right
> after initializing 'handle' and just before passing the GArray to
> tp_cli_channel_interface_group_call_add_members().

Oh right, there are still places doing that even in tp-glib code. I'll change.

> +    GQueue *contacts_changed_queue;
> 
> Any particular reason for using a GQueue * and not a GQueue? NBD, just
> wondered.

No particular reason, just weird doing &self->priv->contacts_changed_queue.

> +static void
> +contacts_upgraded_cb (TpConnection *connection,
> …
> +  g_simple_async_result_complete (result);
> +}
> 
> I think you're missing an unref.

Unref is passed to tp_connection_upgrade_contacts().

> What happens if a new message arrives from a contact while we're still
> preparing their TpContact? Does the message get signalled before the member
> change notification? If so, that's a bug.

> Relatedly, can we expect the result of tp_signalled_message_get_sender() to be
> prepared? Consider getting a message from someone who's not currently in the
> chat room.

This is not about message contact. I do not give any guarantee on message sender, yet. In practice, if sender is part of GROUP iface (or is target handle) it will _most probably_ be ready, but not guaranteed. Indeed there is the case the contact joined the channel and send message right away, so we could still be preparing it. Also the case the sender is not part of the channel.

Later I would like to also guarantee message sender to be prepared, and probably at that point TpTextChannel will have too hook into contacts_changed_queue to make sure to not reorder added member and received message.

ContactsChangedItem (elements in that queue) is already flexible enough and TpTextChannel can easily make such item with the sender contact and put inside the queue. Just need to move some API into channel-internal.h.

Branch updated with everything fixed.

I've made the implementation and doc closer to what we have for GROUP feature: values are undefined until CONTACTS feature is prepared. I've actually copy/pasted the doc for the most part.

Since the spec part is already merged, I reassign this bug to tp-glib for service and client side implementation.

Reworked the queue in my branch to fit the needs of bug #40903

Branch ready for review, afaik.

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=channel-contacts&id=2a5d07781b72e19d89e6601036650149a933705b

Not mergeable. We should do a spec release and sync tp-glib.


tp_svc_channel_interface_group_emit_handle_owners_changed_detailed (
    obj, empty_hash_table, arr_owners_removed, empty_hash_table);
According to the spec we MAY include the identifier of the removed handles;
any reason to not to?

dup_handle_array(): can't we use g_array_ref? Doesn't dbus-glib play nice with
GArray now?

dup_owners_table(): you don't check if dup_contact returned NULL.

contacts_queue_item_set_contacts(): add a "g_assert (item->contacts == NULL);"
to make sure we don't leak contacts?

tp_channel_group_get_contact_owner:
* - if @self does not have flags that include
 *   %TP_CHANNEL_GROUP_FLAG_PROPERTIES,
 *   result is undefined;
Did you mean TP_CHANNEL_GROUP_FLAG_CHANNEL_SPECIFIC_HANDLES?

_tp_channel_contacts_prepare_async
          "The Connection Manager does not implement the required telepathy "
          "specification to prepare TP_CHANNEL_FEATURE_CONTACTS");
Include the spec version number?

+   * TpChannel::group-contacts-changed:
+   * @added: (type GLib.PtrArray) (element-type TelepathyGLib.Contact):
+   *  a #GList of #TpContact containing the full members added
It's not a GList.
ditto for the other args.
Say that those contacts have been prepared if possible?


http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=channel-contacts&id=bad617f34e09084ef977fa11fa2f1ae1af5cffbe

+  g_print ("contact added: %s\n", tp_contact_get_identifier (g_ptr_array_index (added, 0)));
should be removed.

(In reply to comment #22)
> tp_svc_channel_interface_group_emit_handle_owners_changed_detailed (
>     obj, empty_hash_table, arr_owners_removed, empty_hash_table);
> According to the spec we MAY include the identifier of the removed handles;
> any reason to not to?

Same rational than for contact-ids in MembersChangedDetailed. This makes message size smaller since we don't need them anyway.

> dup_handle_array(): can't we use g_array_ref? Doesn't dbus-glib play nice with
> GArray now?

ahah! no :(

I've checked dbus-glib code and only GHashTable are safe to ref, arrays are destroyed.

> dup_owners_table(): you don't check if dup_contact returned NULL.

We actually want to map to NULL in that case, meaning it is channel-specific but mapping is unknown. If we don't include in the map it would mean it is globally valid contact. See in tp_channel_group_get_contact_owner() we use g_hash_table_lookup_extended() to make distinction between not in table and mapped to NULL.

> contacts_queue_item_set_contacts(): add a "g_assert (item->contacts == NULL);"
> to make sure we don't leak contacts?

can't hurt indeed. done.

> tp_channel_group_get_contact_owner:
> * - if @self does not have flags that include
>  *   %TP_CHANNEL_GROUP_FLAG_PROPERTIES,
>  *   result is undefined;
> Did you mean TP_CHANNEL_GROUP_FLAG_CHANNEL_SPECIFIC_HANDLES?

That's copy/pasted from tp_channel_group_get_handle_owner. I think it is correct because that flag tells if GetAll works. that's totally old stuff where group handles were not fetched via dbus properties. In the case we don't have TP_CHANNEL_GROUP_FLAG_CHANNEL_SPECIFIC_HANDLES, the result is defined since we know all contacts are globally valid so it will always return @self.

> _tp_channel_contacts_prepare_async
>           "The Connection Manager does not implement the required telepathy "
>           "specification to prepare TP_CHANNEL_FEATURE_CONTACTS");
> Include the spec version number?

right, done.

> +   * TpChannel::group-contacts-changed:
> +   * @added: (type GLib.PtrArray) (element-type TelepathyGLib.Contact):
> +   *  a #GList of #TpContact containing the full members added
> It's not a GList.
> ditto for the other args.
> Say that those contacts have been prepared if possible?

I've added 
   * This is not guaranteed to be emitted until tp_proxy_prepare_async() has
   * finished preparing %TP_CHANNEL_FEATURE_CONTACTS; until then, it may be
   * %NULL.

Since preparing that would catch too old CMs. And the doc of that feature says it is guaranteed that contacts are always prepared. It's the same text copied from other properties.

> 
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=channel-contacts&id=bad617f34e09084ef977fa11fa2f1ae1af5cffbe
> 
> +  g_print ("contact added: %s\n", tp_contact_get_identifier (g_ptr_array_index
> (added, 0)));
> should be removed.

Oops, removed.


Pushed an extra commit to fix review comments, I'll squash if it's OK.

(In reply to comment #23)
> 
> I've added 
>    * This is not guaranteed to be emitted until tp_proxy_prepare_async() has
>    * finished preparing %TP_CHANNEL_FEATURE_CONTACTS; until then, it may be
>    * %NULL.

Ops:

   * This is not guaranteed to be emitted until tp_proxy_prepare_async() has
   * finished preparing %TP_CHANNEL_FEATURE_CONTACTS; until then, it may be
   * omitted.

(In reply to comment #23)
> > dup_handle_array(): can't we use g_array_ref? Doesn't dbus-glib play nice with
> > GArray now?
> 
> ahah! no :(
> 
> I've checked dbus-glib code and only GHashTable are safe to ref, arrays are
> destroyed.

Bouh :(
Open a dbus-glib while we're on it?

> Pushed an extra commit to fix review comments, I'll squash if it's OK.

Looks good to me. Feel free to merge once you have done the spec release and
synced tp-glib (don't forget to drop the old commit).

This is now merged. Bug fixed \o/