[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

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://git.collabora.co.uk/ |http://cgit.collabora.co.uk
                   |?p=user/jonny/telepathy-gab |/git/user/wjt/telepathy-gab
                   |ble.git;a=shortlog;h=refs/h |ble-wjt.git/log/?h=client-t
                   |eads/client-types           |ypes

--- Comment #4 from Will Thompson <will.thompson at collabora.co.uk> 2011-04-08 10:01:28 PDT ---
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.

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