[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 #16 from Will Thompson <will.thompson at collabora.co.uk> 2011-09-12 07:50:07 PDT ---
+      /* 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 at 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 at 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”.

-- 
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.