Bug 37283

Will, could you please review the last 2 patches in http://cgit.collabora.co.uk/git/user/bari/telepathy-gabble.git/log/?h=shared-presence ?

The code looks fine, as does the behaviour change; I have a couple of nitpicks on the test. Thinking about it, the complexity here is because we don't have a concept of shared status in the Telepathy API, so we're trying to squish two concepts into one. :)

+    list_attrs['status'] = list_attrs.get('status') or msg

I think this means the same as list_attrs.get('status', msg) which I find clearer.

Hmm, I wonder if we could have test API like this:

class ForbiddenDBusEvents(object):
    def __init__(self, q, *patterns):
        self.q = q
        self.patterns = patterns

    def enter(self):
        self.q.forbid_events(self.patterns)

    def exit(self):
        sync_dbus(q, ...)
        self.q.unforbid_events(self.patterns)

and then we could use:

    with ForbiddenDBusEvents(q, EventPattern(...), ...):
        do some testing

+    # Test that status doesn't change when we are away and the status is
+    # changed from another client.

I think maybe this deserves a little more explanation of what's being tested: specifically, “Test that our own status as exposed over D-Bus doesn't change when we are away and other clients change the shared status”, maybe?

(In reply to comment #2)
> Hmm, I wonder if we could have test API like this:
...
>     with ForbiddenDBusEvents(q, EventPattern(...), ...):
>         do some testing

I added push_forbidden() and pop_forbidden() (or something) for the MC tests, perhaps that's close enough?

(In reply to comment #2)
> The code looks fine, as does the behaviour change; I have a couple of nitpicks
> on the test. Thinking about it, the complexity here is because we don't have a
> concept of shared status in the Telepathy API, so we're trying to squish two
> concepts into one. :)

Yep.

> +    list_attrs['status'] = list_attrs.get('status') or msg
> 
> I think this means the same as list_attrs.get('status', msg) which I find
> clearer.

Erm... yes. I copied that code from the other test. In theory the 2 things are different, but in this case they mean the same.

> Hmm, I wonder if we could have test API like this:
> [...]
> and then we could use:
> 
>     with ForbiddenDBusEvents(q, EventPattern(...), ...):
>         do some testing

Nope, we cannot use "with" with our minimum requirement for Python.

> +    # Test that status doesn't change when we are away and the status is
> +    # changed from another client.
>
> I think maybe this deserves a little more explanation of what's being tested:
> specifically, “Test that our own status as exposed over D-Bus doesn't change
> when we are away and other clients change the shared status”, maybe?

Sure.

I reordered the patches and added “presence/shared-status.py: don't use a weird and avoidable "or" construct” and “fixup! presence/shared-status.py: test that remote presence doesn't override ...” to fix the things you asked me to fix (of course I will squash the latter before merging).

Looks good!

Merged and pushed to telepathy-gabble-0.12 and master.