Bug 32139

Summary: Shows phone clienttype when the remote user is logged in with both a phone and a pc
Product: Telepathy Reporter: Sjoerd Simons <sjoerd>
Component: gabbleAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.co.uk/git/user/wjt/telepathy-gabble-wjt.git/log/?h=client-types
Whiteboard:
i915 platform: i915 features:

Description Sjoerd Simons 2010-12-06 08:48:33 UTC
If there are multiple clients with the same availability, but different client types, then Gabble seems to (sometimes?, always?) use phone as the client type. 

These days Empathy shows a phone icon for contacts using their phone (such as the N900),  so this is mildly annoying. Like we did for availability we should probably have an ordering for the different client types, such that gabble can first sort by availability and then by most capable client type.
Comment 1 Jonny Lamb 2011-01-11 08:01:30 UTC
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.
Comment 2 Jonny Lamb 2011-01-20 02:34:44 UTC
Quite a few big changes here. I've added another test for this bug too.
Comment 3 Will Thompson 2011-02-07 02:57:19 UTC
-  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.
Comment 4 Will Thompson 2011-04-08 10:01:28 UTC
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.
Comment 5 Jonny Lamb 2011-04-11 03:46:02 UTC
+ 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.
Comment 6 Will Thompson 2011-04-11 08:08:26 UTC
I pushed a patch to add a g_return_val_if_fail() check.
Comment 7 Will Thompson 2011-04-18 11:30:16 UTC
…and just merged the thing: http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.