Bug 36058

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