[Bug 38248] Get handle identifiers for contacts related to a channel

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=38248

--- Comment #18 from Xavier Claessens <xclaesse at gmail.com> 2011-09-13 06:43:31 PDT ---
(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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.