Bug 32139

This shouldn't happen. I just had a little look at the client-types.py test and it appears to be testing exactly what you're describing. Could you take a look at it and let me know what that's missing or something like that? Cheers.

Quite a few big changes here. I've added another test for this bug too.

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

Well, I took over this branch, fixed most of my review comments, found and fixed some bugs, and did some cleanup.

(In reply to comment #3)
> -  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'.)

Fixed in my branch.

> +  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?

It turned out not to be very useful to include the client types, due mainly to the idle hack.

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

This actually wasn't true, but I refactored this code to make it more (obviously) right.

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

I refactored this all to make the GPtrArrays an implementation detail of gabble_presence_get_client_types_array().

> 
> +  /* 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.

I weak-reffed the connection, and improved this comment to explain why it's so difficult to fix things so this idle is not necessary.

> 
> +  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 did the latter.

+ gchar ***types)

**************************

- *types = empty_array;
+ *types = g_strdupv (empty);

Not your code, but perhaps you should check whether types != NULL before setting *types

Seems alright otherwise.

I pushed a patch to add a g_return_val_if_fail() check.

…and just merged the thing: http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/