Bug 36058

Summary: Available presence seen for a bit when switching from dnd to away
Product: Telepathy Reporter: Marco Barisione <marco.barisione>
Component: gabbleAssignee: Marco Barisione <marco.barisione>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/bari/telepathy-gabble.git;a=shortlog;h=refs/heads/shared-presence
Whiteboard:
i915 platform: i915 features:

Description Marco Barisione 2011-04-07 10:53:22 UTC
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/>.
Comment 1 Marco Barisione 2011-04-07 10:56:47 UTC
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.
Comment 2 Will Thompson 2011-04-12 11:44:12 UTC
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.
Comment 3 Will Thompson 2011-04-12 11:54:58 UTC
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.
Comment 4 Marco Barisione 2011-04-12 13:25:41 UTC
(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.
Comment 5 Will Thompson 2011-04-14 03:25:27 UTC
(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
Comment 6 Marco Barisione 2011-04-15 10:07:40 UTC
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?
Comment 7 Marco Barisione 2011-04-18 10:39:22 UTC
I pushed 6 new patches to the branch. Can you please review them?
Comment 8 Will Thompson 2011-04-29 08:21:07 UTC
(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
Comment 9 Marco Barisione 2011-05-04 05:24:26 UTC
Updated. I think I addressed all of the issues you found.
Comment 10 Will Thompson 2011-05-05 04:14:51 UTC
(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.)

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.