[Bug 28726] Support Google's Shared Status extension

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-10-27 06:47:11 PDT ---
(In reply to comment #7)
> 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 !
> 

Dorks, that was a desperate moment nobody was supposed to see. It's been
squished out.

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

I knew there was a wocky alternative, didn't look hard enough. 05d1569

> +  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);
> 

I should try to understand the _build magic, and not just copy others. 8bb8593

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

Like I said above, I don't know the magic. I would like to keep it the last
element because that is how google does it, and I am paranoid.

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

I will heed to your emotions. 70c3540

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

squashed.

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

I originally had set_shared_status() take a NULL GASyncResult, but the NULL
checking got cumbersome. Also, it's nice to have a callback for when other
things are needed (besides emit_presences_changed_for_self() that is called
from it now).


> +  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"); ... } ?
> 

Debug messages cleaned up. 19331a4

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

Added a g_assert. f2c6fb5

> 
> @@ -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? :)
> 

squashed.

> +  DEBUG ("%d",
> +      self->features & GABBLE_CONNECTION_FEATURES_GOOGLE_SHARED_STATUS);
> 
> More useful message plz.

19331a4

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

c343aeb

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

c343aeb

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

62c3765

> Also, I changed my GMail theme. ;-)

In that case, mark it RESOLVED.

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