1. Connect to GTalk 2. Set your presence to dnd 3. Set your presence to away Your contacts will see dnd -> available -> away. This is actually due to 2 different bugs in our shared presence implementation. The first issue is that we send both the <iq/> for shared presence and the <presence/> stanza; the <iq/> for setting away actually say to set the presence to available. This is because shared presence is not supported for away (and xa). Being away in GTalk is mapped to being idle and is per connection, so it should be handled with <presence/>.
This branch fixes both issues, an indentation problem and updates the tests. Note that with this branch we lose support for the “chat” status when using GTalk. Not that anybody uses it probably... I think this branch should not break caps (that are sent in <presence/>) or invisibility and the tests keep working, but please tell me if I'm wrong.
The thing that most concerns me about this branch is that I can't see any way that our initial capabilities will necessarily be sent. Interestingly, as far as I can tell, conn_presence_set_initial_presence_async() does not actually set our initial presence on the Shared Status code path. Also shared_status_setup_cb() calls get_existing_privacy_lists_async(), which makes no sense at all: if we have Shared Status, we shouldn't be looking at privacy lists……… OHHHHHH! That's how it works. privacy_lists_loaded_cb() (the callback for get_existing_privacy_lists_async()) is called when receiving the privacy lists from the Google server fails (as it inevitably will), and that function calls toggle_presence_visibility_async(). And this works in the test suite because BaseXmlStream (and hence SharedStatusStream) responds to privacy list queries in the negative by default. I don't suppose you fancy tidying that up so that Gabble doesn't check for privacy list support if the server does indeed support shared status, and just calls toggle_presence_visibility_async() directly from shared_status_setup_cb() in the success path? And also fix the shared status test case to forbid privacy list queries (and set handle_privacy_lists to False in SharedStatusStream)? On presence/shared-status.py: test the correct behaviour with away and xa: + #e = q.expect('dbus-signal', signal='PresenceUpdate') + #print e.args + #return O rly. + "chat" : ['Please talk with me!', + 'Work is overrated, let\'s chat']} I thought you said the 'chat' status stops working as a result of this branch; but your test tests it. The other patches look fine, modulo my concern above.
I would love your reciprocal feedback on the trivial patches at http://cgit.collabora.co.uk/git/user/wjt/telepathy-gabble-wjt.git/log/?h=misc that I committed while reviewing this and noticing bugs.
(In reply to comment #2) > OHHHHHH! That's how it works. privacy_lists_loaded_cb() (the callback for > get_existing_privacy_lists_async()) is called when receiving the privacy lists > from the Google server fails (as it inevitably will), and that function calls > toggle_presence_visibility_async(). And this works in the test suite because > BaseXmlStream (and hence SharedStatusStream) responds to privacy list queries > in the negative by default. Oh, I thought conn_presence_set_initial_presence_async() was called. > I don't suppose you fancy tidying that up so that Gabble doesn't Can try, not sure the results will be good at the first attempt :) (and hey, this code is entangled!) > On presence/shared-status.py: test the correct behaviour with away and xa: > > + #e = q.expect('dbus-signal', signal='PresenceUpdate') > + #print e.args > + #return > > O rly. Ops, yes. > + "chat" : ['Please talk with me!', > + 'Work is overrated, let\'s chat']} > > I thought you said the 'chat' status stops working as a result of this branch; > but your test tests it. Yes, but look at presence_types: we expect PRESENCE_AVAILABLE when "chat" is used. Of course this would need a comment.
(In reply to comment #4) > (In reply to comment #2) > > OHHHHHH! That's how it works. privacy_lists_loaded_cb() (the callback for > > get_existing_privacy_lists_async()) is called when receiving the privacy lists > > from the Google server fails (as it inevitably will), and that function calls > > toggle_presence_visibility_async(). And this works in the test suite because > > BaseXmlStream (and hence SharedStatusStream) responds to privacy list queries > > in the negative by default. > > Oh, I thought conn_presence_set_initial_presence_async() was called. > > > I don't suppose you fancy tidying that up so that Gabble doesn't > > Can try, not sure the results will be good at the first attempt :) (and hey, > this code is entangled!) 11:25 wjt_ | barisione: re. your last comment on https://bugs.freedesktop.org/show_bug.cgi?id=36058, conn_presence_set_initial_presence_async() *is* called 11:25 wjt_ | barisione: but i couldn't see how calling it would end up actually setting your initial presence in the shared status case 11:26 wjt_ | barisione: and it turns out to only do so by accident, because the shared status code path inexplicably calls into the privacy list code path 11:26 barisione | ah ok 11:26 barisione | now it's clear 11:26 wjt_ | so the tidying up i'm after isn't very big 11:27 wjt_ | it's just making it not try to get privacy lists once it's verified that the server supports SS
I'm a bit confused about the proper fix. My code was actually working (by accident) and sending the initial caps. The caps were not sent when we were initially connected (i.e. conn_presence_signal_own_presence() is not called), but we send the caps as a consequence of connection_got_self_initial_avatar_cb() (that calls update_own_avatar_sha1() that calls conn_presence_signal_own_presence()). How should this work together with shared status?
I pushed 6 new patches to the branch. Can you please review them?
(In reply to comment #2) > And also fix the shared status test case to forbid privacy > list queries (and set handle_privacy_lists to False in SharedStatusStream)? You don't seem to have made this change. From IRC: 16:13 wjt | barisione: http://cgit.collabora.co.uk/git/user/bari/telepathy-gabble.git/commit/?id=1ce86d06e740f827ffaf5f7e76bab123865c7824 is wrong 16:13 wjt | barisione: "chat" always has type Available 16:13 wjt | there's no type Chat 16:16 wjt | barisione: in <http://cgit.collabora.co.uk/git/user/bari/telepathy-gabble.git/commit/?id=2e4059c105cf0755c58fdcf149f2f604cc2dfa97>, don't do that 16:16 wjt | instead change the status_available_cb() to return FALSE for "chat" if Shared Status is in use 16:17 wjt | that way, tp-glib will ensure that you can't use 'chat' 16:18 wjt | oh, but you may need to make the code just after you've discovered that shared status is available patch the self presence
Updated. I think I addressed all of the issues you found.
(In reply to comment #9) > Updated. I think I addressed all of the issues you found. This looks good. Can you rebase it atop the telepathy-gabble-0.12 branch I've just created it, and merge it to that branch? (master will become 0.13.0, but this bug fix deserves to go into the stable branch.)
http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/?id=5d2927d9155d2388145406c189fdf9d9f8dfcac6 woop woop
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.