[Bug 28726] Support Google's Shared Status extension

[bugzilla-daemon at freedesktop.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review-, nothing major,
                   |                            |just trivia

--- Comment #7 from Will Thompson <will.thompson at collabora.co.uk> 2010-10-26 10:45:21 PDT ---
Why did get_bare_jid() move to util.h? Why not make it
gabble_connection_get_bare_jid() in connection.h? It's conceptually a method on
the connection.

+#define g_simple_async_result_complete_in_idle g_simple_async_result_complete

http://uploads.siteduzero.com/files/123001_124000/123393.jpg !

+      priv->iq_shared_status_cb = lm_message_handler_new (
+          iq_shared_status_changed_cb, self, NULL);
+
+      priv->invisibility_method = INVISIBILITY_METHOD_SHARED_STATUS;
+
+      lm_connection_register_message_handler (self->lmconn,
+          priv->iq_shared_status_cb, LM_MESSAGE_TYPE_IQ,
+          LM_HANDLER_PRIORITY_NORMAL);

You could use wocky_porter_register_handler(). You can tell it what shape of
stanza you're expecting, too.

+  WockyStanza *iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+      WOCKY_STANZA_SUB_TYPE_SET, NULL, bare_jid,
+        '(', "query",
+          ':', NS_GOOGLE_SHARED_STATUS,
+        ')',
+      NULL);
+  WockyNode *query_node = wocky_node_get_child_ns (
+      wocky_stanza_get_top_node (iq), "query",
+      NS_GOOGLE_SHARED_STATUS);
+
+  wocky_node_add_child_with_content (query_node, "status",
presence->status_message);
+  wocky_node_add_child_with_content (query_node, "show",
+      presence->status == GABBLE_PRESENCE_DND ? "dnd" : "default");

You can replace this with:

+  WockyNode *query_node = NULL;
+  WockyStanza *iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+      WOCKY_STANZA_SUB_TYPE_SET, NULL, bare_jid,
+        '(', "query",
+          ':', NS_GOOGLE_SHARED_STATUS,
+          '*', &query_node,
+          '@', "status", presence->status_message,
+          '(', "show",
+            '$', presence->status == GABBLE_PRESENCE_DND ? "dnd" : "default",
+          ')',
+        ')',
+      NULL);

Also adding the "invisible" node could be folded into this. I could be
convinced that this is less clear than spelling it out, maybe...

+  g_hash_table_foreach (priv->shared_statuses, (GHFunc)
add_shared_status_list,
+      query_node);

I'm more a fan of g_hash_table_iter, but this is no big deal.


+  if (!conn_util_send_iq_finish (self, res, NULL, &error) ||
+      !conn_presence_signal_own_presence (self, NULL, NULL))
+    {
+      g_simple_async_result_set_error (result,
+          CONN_PRESENCE_ERROR, CONN_PRESENCE_ERROR_SET_SHARED_STATUS,
+          "error setting Google shared status: %s", error->message);
+    }
+
+      g_simple_async_result_complete (result);
+      g_object_unref (result);

Couldn't 'error' be unset if conn_util_send_iq_finish() is successful, but
conn_presence_signal_own_presence() fails? And the indentation of the
complete/unref lines is misleading. You fixed this later. Could you squash the
“fixed invisibility” commit into the first commit, maybe?

I don't really see the point of making set_shared_status_async() a thin wrapper
around another function.

+  DEBUG ("%d", priv->shared_status_compat);

Log a more useful debug message. Maybe if (priv->shared_status_compat && status
== HIDDEN) { DEBUG ("a connected client doesn't support invisibility; falling
back to DND"); ... } ?

+      WockyNode *query_node = wocky_node_get_child_ns
(wocky_stanza_get_top_node (iq),
+          "query", NS_GOOGLE_SHARED_STATUS);
+
+      store_shared_statuses (self, query_node);

This will make store_shared_statuses crash if the <query/> node is missing.


@@ -715,6 +1024,7 @@ get_existing_privacy_lists_async (GabbleConnection *self,
       WOCKY_STANZA_SUB_TYPE_GET, NULL, NULL,
         '(', "query",
           ':', NS_PRIVACY,
+          '@', "version", GOOGLE_SHARED_STATUS_VERSION,
         ')',

Whu? Why set the shared status version on the privacy list query?

Oh, you fixed it in a later commit. Could you rebase that out of existence? :)

+  DEBUG ("%d",
+      self->features & GABBLE_CONNECTION_FEATURES_GOOGLE_SHARED_STATUS);

More useful message plz.

+          const gchar *show = wocky_node_get_attribute(node, "show");

The coding style checker should have screamed at you for the lack of space
before the (.

+            {
+              g_ptr_array_add (statuses,
+                  (gpointer) g_strdup (list_item->content));
+            }

You don't need to cast to gpointer. (I guess you originally didn't strdup and
were casting the const away?)

The tests look good! It'd be nice to put a reference to
http://code.google.com/apis/talk/jep_extensions/shared_status.html into the
relevant test's header. And I think this:

+presence_types = {'available' : 2,
+                  'away'      : 3,
+                  'hidden'    : 5,
+                  'dnd'       : 6}

ought to be using magic numbers from constants.py.

Also, I changed my GMail theme. ;-)

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.