[Bug 32139] Shows phone clienttype when the remote user is logged in with both a phone and a pc

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Will Thompson <will.thompson at collabora.co.uk> 2011-02-07 02:57:19 PST ---
-  for (l = waiters; l != NULL; l = l->next)
+  for (l = waiter_list; l != NULL; l = l->next)
     {
-      DiscoWaiter *w = l->data;
+      GList *j;

-      if (w != NULL && w->handle == handle && !tp_strdiff (w->resource,
resource))
+      for (j = l->data; j != NULL; j = j->next)
         {
-          out = TRUE;
-          break;
+          DiscoWaiter *w = j->data;
+
+          if (w != NULL && w->handle == handle && !tp_strdiff (w->resource,
resource))
+            {
+              out = TRUE;
+              break;
+            }
         }
     }

Your 'break' only jumps out of the inner loop, not out of the outer one. This
code is still functionally correct, but wasteful. You could either use a goto,
or (and I would prefer this) you could include '!out' in both loops' condition.
(Also I would slightly like a better name than 'out'.)

+  signals[CLIENT_TYPES_UPDATED] = g_signal_new (
+    "client-types-updated",
+    G_TYPE_FROM_CLASS (klass),
+    G_SIGNAL_RUN_LAST,
+    0,
+    NULL, NULL,
+    g_cclosure_marshal_VOID__UINT, G_TYPE_NONE, 1, TP_TYPE_HANDLE);

Shouldn't this signal include the list/bitfield of client types?

+      if (best == NULL)
+        best = r;

       /* trump existing status & message if it's more present
        * or has the same presence and a more recent last activity
        * or has the same presence and a higher priority */
-      if (r->status > presence->status ||
-          (r->status == presence->status && r->last_activity > activity) ||
-          (r->status == presence->status && r->priority > prio))
+      if (r->status > best->status ||
+          (r->status == best->status && r->last_activity > activity) ||
+          (r->status == best->status && r->priority > prio) ||
+          (r->client_type & GABBLE_CLIENT_TYPE_PC
+              && !(best->client_type & GABBLE_CLIENT_TYPE_PC)))

The second if block should be else if, because it's a no-op if r == best.


-  if (client_types != NULL)
-    {
-      g_ptr_array_unref (client_types);
-      _signal_presences_updated (cache, handle);
-    }
+  if (out_types != NULL)
+    *out_types = client_types;

So yeah, I think this means you're leaking client_types if out_types != NULL. I
generally am not overly confident about the GPtrArray refcounting in cf067b1.

+  /* Do this in an idle because the presence cache can make disco
+   * requests after dealing with the incoming presence stanza, so we
+   * can reach this point before the disco request has been made and
+   * disco_in_progress will return FALSE and we will signal with no
+   * client types which is a bit annoying. If we do this bit in an
+   * idle then the disco request will have been made by the time the
+   * idle source function is actually called. */

Eww.

You should take a ref on at least the connection, if not the cache too. I
really don't like this though.

+  if (array != empty_array)
+    g_ptr_array_unref (array);

This is silly. gabble_presence_get_client_types_array() should return a ref to
a GPtrArray and then the cleanup path can be the same either way. Or,
gabble_presence_get_client_types_array() could be guaranteed to return
non-NULL.

I know this is existing code, but.

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