Bug 29298

+static gboolean
+dummy_caps_set_predicate (const GabbleCapabilitySet *set,
+    gconstpointer user_data)
+{
+  return TRUE;
+}

Why not just make gabble_presence_pick_resource_by_caps() accept a NULL predicate?

Why can't the method and contact attribute implementations share code?

+  /* Find all identity nodes in the return. */

Do you mean ‘result’?

+      const gchar *type;
+
+      /* Now get the client type */
+      if ((type = wocky_node_get_attribute (identity, "type")) == NULL)
+        continue;

This is perverse. Assign directly to type when it's defined?

+              WockyNode *query = wocky_node_tree_get_top_node (cached_query_rep
+              GPtrArray *types;
+
+              if ((types = client_types_from_message (handle, query)) != NULL)

Ditto.

+gboolean
+gabble_presence_cache_disco_in_progress (GabblePresenceCache *cache,
+    TpHandle handle,
+    const gchar *resource)

I think maybe some of the Jingle code could do with being made to use this function?


       /* trump existing status & message if it's more present
        * 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))

You should update the comment.


+      if (!tp_strdiff (type, "bot"))
+        res->client_type |= GABBLE_CLIENT_TYPE_BOT;
+
+      if (!tp_strdiff (type, "console"))
+        res->client_type |= GABBLE_CLIENT_TYPE_CONSOLE;
+
+      if (!tp_strdiff (type, "game"))
+        res->client_type |= GABBLE_CLIENT_TYPE_GAME;
+
+      if (!tp_strdiff (type, "handheld"))
+        res->client_type |= GABBLE_CLIENT_TYPE_HANDHELD;
+
+      if (!tp_strdiff (type, "pc"))
+        res->client_type |= GABBLE_CLIENT_TYPE_PC;
+
+      if (!tp_strdiff (type, "phone"))
+        res->client_type |= GABBLE_CLIENT_TYPE_PHONE;
+
+      if (!tp_strdiff (type, "web"))
+        res->client_type |= GABBLE_CLIENT_TYPE_WEB;
+
+      if (!tp_strdiff (type, "sms"))
+        res->client_type |= GABBLE_CLIENT_TYPE_SMS;

If you generate a GEnum for the GabbleClientType enum, you can use wocky_enum_from_nick() to remove all this code. Oh hmm, I guess it'd be GFlags... but something similar presumably applies. The next function which does the reverse is harder because the flags aren't mutually exclusive. Something similar should be possible... get the GFlagsClass and iterate its ->values array.

Sigh, I already replied to this once. Let's do it again!

(In reply to comment #1)
> Why not just make gabble_presence_pick_resource_by_caps() accept a NULL
> predicate?

OK done.

> Why can't the method and contact attribute implementations share code?

They can now!

> Do you mean ‘result’?

I do.

> This is perverse. Assign directly to type when it's defined?

OK

> You should update the comment.

Done.

> If you generate a GEnum for the GabbleClientType enum, you can use
> wocky_enum_from_nick() to remove all this code. Oh hmm, I guess it'd be
> GFlags... but something similar presumably applies. The next function which
> does the reverse is harder because the flags aren't mutually exclusive.
> Something similar should be possible... get the GFlagsClass and iterate its
> ->values array.

Yeah that's better. Remind me to file a glib bug to merge all these helper functions for GFlags/GEnum.

Now, let's try that again...

       const gchar *type;
 
       /* Now get the client type */
-      if ((type = wocky_node_get_attribute (identity, "type")) == NULL)
+      type = wocky_node_get_attribute (identity, "type");

You could assign to 'type' when you declare it.

+      if(type == NULL)

Style: space before paren.

Ah I see you made both of these comments unnecessarily later.

+      if (g_ptr_array_index (types, 0) != NULL)

Is this secret code for (types->len > 0)? If not, could you explain why not? It's not obvious to me.

The Android special case is pretty annoying, but it seems like a reasonable compromise. We should also nag Google to do the right thing.

(In reply to comment #3)
> +      if (g_ptr_array_index (types, 0) != NULL)
> 
> Is this secret code for (types->len > 0)? If not, could you explain why not?
> It's not obvious to me.

Ah yes, that is now wrong. See this for a clarification:

http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=commitdiff;h=10fd7b2e

> The Android special case is pretty annoying, but it seems like a reasonable
> compromise. We should also nag Google to do the right thing.

I'll email them now.

merged, thanks.