| Summary: | Support Google's Shared Status extension | ||
|---|---|---|---|
| Product: | Telepathy | Reporter: | Will Thompson <will> |
| Component: | gabble | Assignee: | Will Thompson <will> |
| Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
| Severity: | enhancement | ||
| Priority: | low | CC: | 84yelo3, eitan.isaacson, mehmet.giritli, om26er, smcv, steko, tgpraveen89, will |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| URL: | http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=shortlog;h=refs/heads/shared-status | ||
| Whiteboard: | merged | ||
| i915 platform: | i915 features: | ||
| Bug Depends on: | 20768 | ||
| Bug Blocks: | |||
| Attachments: | unsightly error message in gmail | ||
http://code.google.com/apis/talk/jep_extensions/shared_status.html is the extension's documentation. *** Bug 29724 has been marked as a duplicate of this bug. *** Ok, taking this. This will be more than invisibility as this extension is impossible to implement in a piecemeal fashion. Will, your gmail theme is horrible and offensive. I'm intentionally not adding the 'patch' keyword. Want to do one more round of testing. I few caveats: 1. This patch does not add support for the shared status *list*, it perpetuates whatever list it last retrieved, but does not add our current status to the list. Google's server does the right thing, and appends it for us. Does telepathy have a stored status iface? I didn't see it, but if we did, then this would be a useful feature later on. 2. There is a dilemma how to set the status when first connected. Google's docs don't have any pointers for this. Do we use the shared status, or the locally requested status? Or do we decide on it in descending order of restrictiveness (hidden, dnd, etc.). I went with local status. If the user intentionally sets the status when connecting, they are responsible for it. Also, the current protocol does not give any easy way of knowing if other resources are currently connected, so chances are you are getting a phantom status from the last time you connected with the same client. Ok, this is ready for review. Disregard item 1 above, we support stored shared statuses now, although it is not exposed to the UI. Taking this bug to review the branch. 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. ;-) (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 ! > > + 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. ;-) Did you mean to add some text with your reply? :)
> Assert that stanza has query element before using.
This is a stanza that the server just sent us. If the server messes up, we shouldn't crash. The code should treat the <query/> element being missing as equivalent to getting an error back.
Also I noticed that get_shared_status_cb() contains:
938 DEBUG ("Error getting privacy lists: %s", error->message);
which is not quite right.
You didn't address my question about why get_bare_self_jid() was moved to util.h, rather than living in a connection-related header.
(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. (In reply to comment #9) > Did you mean to add some text with your reply? :) > > > Assert that stanza has query element before using. > > This is a stanza that the server just sent us. If the server messes up, we > shouldn't crash. The code should treat the <query/> element being missing as > equivalent to getting an error back. > > Also I noticed that get_shared_status_cb() contains: > > 938 DEBUG ("Error getting privacy lists: %s", error->message); > > which is not quite right. > > You didn't address my question about why get_bare_self_jid() was moved to > util.h, rather than living in a connection-related header. Right! I'll get to work on this momentarily. (In reply to comment #9) > Did you mean to add some text with your reply? :) > Yes, I did. All the revs above don't exist anymore after a recent rebase. > > Assert that stanza has query element before using. > > This is a stanza that the server just sent us. If the server messes up, we > shouldn't crash. The code should treat the <query/> element being missing as > equivalent to getting an error back. Yes, this was stupid. 6fca8f3 > > Also I noticed that get_shared_status_cb() contains: > > 938 DEBUG ("Error getting privacy lists: %s", error->message); > > which is not quite right. bf32c6c > > You didn't address my question about why get_bare_self_jid() was moved to > util.h, rather than living in a connection-related header. I rebased the branch on a patch that puts this in conn-util.c. 9911a07 Ace. Ship it :) (In reply to comment #13) > Ace. Ship it :) Blah, I should really do regression tests before submitting stuff. Could you review to top of my branch? (In reply to comment #14) > (In reply to comment #13) > > Ace. Ship it :) > > Blah, I should really do regression tests before submitting stuff. Could you > review to top of my branch? Doh! Ship that too. :) Merged Hi, Is this not a part of gabble-0.10.4? (In reply to comment #17) > Is this not a part of gabble-0.10.4? No, it was merged to the development branch, so it'll be in 0.11.0. (Eitan: when you merge stuff, please say which version will be the first to have it :-) |
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.
Created attachment 36455 [details] unsightly error message in gmail This is needed for Gabble to support becoming invisible on Google Talk connections. Also not supporting it leads to unsightly error messages in GMail if you do try to become invisible using GMail while also logged in with Gabble.