Bug 37283

Summary: Don't leave "away" when the shared status is remotely changed
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 CC: will
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.collabora.co.uk/git/user/bari/telepathy-gabble.git/log/?h=shared-presence
Whiteboard: review+
i915 platform: i915 features:

Description Marco Barisione 2011-05-17 05:05:38 UTC
Away status when using GTalk's shared status is per connection and means idleness. We should not leave the away status when another client changes its status.
Comment 1 Marco Barisione 2011-05-17 05:08:06 UTC
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 ?
Comment 2 Will Thompson 2011-05-17 06:31:24 UTC
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?
Comment 3 Simon McVittie 2011-05-17 06:33:52 UTC
(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?
Comment 4 Marco Barisione 2011-05-17 07:03:09 UTC
(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.
Comment 5 Marco Barisione 2011-05-17 08:16:08 UTC
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).
Comment 6 Will Thompson 2011-05-17 08:26:06 UTC
Looks good!
Comment 7 Marco Barisione 2011-05-17 08:49:50 UTC
Merged and pushed to telepathy-gabble-0.12 and master.

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.