Bug 28199

This branch fixing some more bad assumptions could be merged already:

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/roster-assumptions

This branch based on that one, when used in conjunction with telepathy-glib from branch smcv/contact-list-fixes, passes tests, although it still needs (many!) tests for the new API:

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/wip-contact-list

On Bug #28200, Guillaume wrote:
> Looks good except
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=ae0c4aed9b8f878806a51f8e14d9a4ed57a65184
> 
> Shouldn't we fix Gabble instead?

Accordingly, I've appended a patch to roster-assumptions to use the self-handle as the actor for changes to the deny list, matching TpBaseContactList's behaviour.

In principle we could do the same for Group changes, but I haven't yet.

For List changes, it's not always obvious who the actor is. For instance, when a contact accepts our subscription request, we can get the roster push from the server before the <presence type='subscribed'/> (assuming test-google-roster.py is realistic), so we can't assume that unsolicited roster pushes are always from one of our own resources.

I think this looks somewhat finished. More tests would obviously be nice, but it demonstrates that TpBaseContactList basically works, passes all the existing tests, and cleans up roster.c in the process:

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/contact-list

(This branch requires telepathy-glib branch contact-list-manager. I'd appreciate review on the branch so far; I don't plan to update it to newer telepathy-glib API until the D-Bus and telepathy-glib API has settled.)

Since the telepathy-glib API has stabilized a bit, I'm rebasing this branch to make sense for the new tp-glib API without having to consider intermediate states, so it's not necessarily suitable for review right now.

I think the current position of contact-list (commit a23615a318870cac7fc7b823467d39f64fba9d23) makes sense to review.

Known problems:

* the test coverage is rather patchy

* contacts can now be removed while not on the list, Unpublish()d while already publish=No, etc., and Gabble will probably do the wrong thing in many of those cases (the Group API wouldn't have allowed this situation to arise)

I propose to fix both of those in parallel, by adding tests for silly situations, fixing the behaviour, and repeating :-)

I've done a first review (patch by patch) of this. Keep in mind that I'm not a Gabble roster expert (I never really hacked on it) so I would have probably missed subtil issues in its logic.

_gabble_roster_item_free() should free item->publish_request

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a07e273d4694040ab4ed287c97bb8a99fcf46b90
I like the idea. Would be good to have somthing similar in glib. Maybe using another result object implementing the GAsyncResult interface so we wouldn't have to abuse the res pointer.

+  /* list of GSimpleAsyncResult */
+  GSList *results;
I'd say if the result objects are owned or borrowed

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=6e66629b4958990d197c15e03020f7c958f2fe25
why not keep both variants and so test the old API as well as you did with other tests?
Same for other tests

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a132acd51f6bf7d9c1cec5c801704f5305fdcf03
if the change didn't stick, shouldn't conn.Contacts.GetContactAttributes() return the "people starting with A" group as well?

I try to use "pyflakes" when adding new tests. For example pyflakes tests/twisted/roster/edit-before-roster.py returns a bunch of trivial warnings that could easily be fixed.
Same for tests/twisted/roster/authorize.py

+  if (priv->pre_authorized != NULL)
+    {
+      tp_handle_set_destroy (priv->pre_authorized);
+      priv->pre_authorized = NULL;
+    }

Why not use your shiny tp_clear_pointer() ? :)

(In reply to comment #7)
> I've done a first review (patch by patch) of this. Keep in mind that I'm not a
> Gabble roster expert (I never really hacked on it) so I would have probably
> missed subtil issues in its logic.
> 
> _gabble_roster_item_free() should free item->publish_request

Fixed, and reviewed on IRC. I've merged the fixes approved so far into contact-list and kept the rest in contact-list-fixes.

> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a07e273d4694040ab4ed287c97bb8a99fcf46b90
> I like the idea. Would be good to have somthing similar in glib. Maybe using
> another result object implementing the GAsyncResult interface so we wouldn't
> have to abuse the res pointer.

Yes, but I'd like to to defer this. I think landing ContactList before it bit-rots any further is more important than GAsyncResult infrastructure :-)

> +  /* list of GSimpleAsyncResult */
> +  GSList *results;
> I'd say if the result objects are owned or borrowed

It's a bit weird: it's "borrowed from itself" (they ref themselves for as long as they have a nonzero counter). I've changed that to an explicit reference, and also fixed a leak of the GSList links themselves.

> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=6e66629b4958990d197c15e03020f7c958f2fe25
> why not keep both variants and so test the old API as well as you did with
> other tests?
> Same for other tests

Done for roster/removed-from-rp-subscribe (which increases it to 8 sub-tests). I don't think I deleted any other old-test variants.

roster/authorize.py is new and doesn't exercise the old API; I could test the old API as well, I suppose.

> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a132acd51f6bf7d9c1cec5c801704f5305fdcf03
> if the change didn't stick, shouldn't conn.Contacts.GetContactAttributes()
> return the "people starting with A" group as well?

tbd

> I try to use "pyflakes" when adding new tests. For example pyflakes
> tests/twisted/roster/edit-before-roster.py returns a bunch of trivial warnings
> that could easily be fixed.
> Same for tests/twisted/roster/authorize.py

tbd

> Why not use your shiny tp_clear_pointer() ? :)

It didn't exist when I initially wrote this code in mid June; I did grep for g_object_unref to add some calls to tp_clear_object, but good places for tp_clear_pointer are harder to spot automatically. I've added some.

> roster/authorize.py is new and doesn't exercise the old API; I could test the
> old API as well, I suppose.

Done.

> > http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=a132acd51f6bf7d9c1cec5c801704f5305fdcf03
> > if the change didn't stick, shouldn't conn.Contacts.GetContactAttributes()
> > return the "people starting with A" group as well?

This was in fact right: GCLA() wasn't called until after the case where it didn't stick, *and* the case where it did. I've added another call to GCLA() to make it more clearly correct.

> > I try to use "pyflakes" when adding new tests. For example pyflakes
> > tests/twisted/roster/edit-before-roster.py returns a bunch of trivial warnings
> > that could easily be fixed.
> > Same for tests/twisted/roster/authorize.py

Done, but I'm really not convinced that removing unused imports and variables from regression tests is very productive...

(In reply to comment #6)
> * the test coverage is rather patchy
> 
> * contacts can now be removed while not on the list, Unpublish()d while already
> publish=No, etc., and Gabble will probably do the wrong thing in many of those
> cases (the Group API wouldn't have allowed this situation to arise)

I now have regression tests for many no-op changes, and have fixed some bad assumptions.

I've also made the roster code distinguish between people who we *treat as* on the roster, and people who *are* on the roster (a subset), which is a bit scary but more correct.

(People from whom we have received a publish request are treated as if on the roster, without actually being there. Technically, that means they shouldn't be on 'stored', but in practice we never did that and I don't think anyone really wants that semantic.)

I hate it when all I have is really pedantic review comments, but:

    GabbleRoster: track whether items are actually on our roster
>    Now, NONE means the item is on our server-side roster with
>    subscription='none', and REMOVE means the item is not on our server-side
>    roster. The function _gabble_roster_item_maybe_remove will avoid removing
>    items from our ContactList if they have another reason to live.

REMOVE is a verb. Would something like "NOT_YET_STORED" be more descriptive, or am I missing some subtlety here?

Also, I looked and saw that GabbleRosterSubscription is a set of flags, but you're testing it as if it was a normal enum. Is there a case where subscription == GABBLE_ROSTER_SUBSCRIPTION_REMOVE | GABBLE_ROSTER_SUBSCRIPTION_INVALID or something, and do we behave correctly in this case?

-  if (item->subscription != GABBLE_ROSTER_SUBSCRIPTION_NONE)
+  if (item->subscription != GABBLE_ROSTER_SUBSCRIPTION_NONE &&
+      item->subscription != GABBLE_ROSTER_SUBSCRIPTION_REMOVE)

I could probably verify this for myself by wrapping my head around all of the logic involved, but I think I'll just ask you and let you assert that you're doing it right. 

I've also not checked the entirety of the subscription state machine (only the bits that appear in the patch). How thorough do you like your reviews to be?

Other than that slight uncertainty, I think everything else looks fine.

(In reply to comment #11)
> REMOVE is a verb. Would something like "NOT_YET_STORED" be more descriptive, or
> am I missing some subtlety here?

It's REMOVE because it corresponds to <item subscription="remove">. It is indeed a verb (state transition) in XMPP too, but appears in an attribute that is normally an adjective; the "default subscription" is of course "not on the roster", so you only see "remove" in XMPP for the state transition from present to absent.

I could change it to REMOVED, but I think it might be clearer to stick to the XMPP phrasing.

> Also, I looked and saw that GabbleRosterSubscription is a set of flags, but
> you're testing it as if it was a normal enum. Is there a case where
> subscription == GABBLE_ROSTER_SUBSCRIPTION_REMOVE |
> GABBLE_ROSTER_SUBSCRIPTION_INVALID or something, and do we behave correctly in
> this case?

As far as I can see, the only states that can ever exist in a GabbleRosterItem are NONE (= 0), FROM, TO, FROM|TO (= BOTH) and REMOVE; additionally, the new_subscription in a GabbleRosterItemEdit can be INVALID. The only bitfielding that goes on should be to combine FROM and TO.

_subscription_to_string() and process_roster() assert this, _parse_item_subscription() can only return these, and I've just checked that no other states are assigned.

> -  if (item->subscription != GABBLE_ROSTER_SUBSCRIPTION_NONE)
> +  if (item->subscription != GABBLE_ROSTER_SUBSCRIPTION_NONE &&
> +      item->subscription != GABBLE_ROSTER_SUBSCRIPTION_REMOVE)

I'm reasonably sure this is right: "no subscription and present on the contact list" and "no subscription and absent from the contact list" were previously conflated as NONE, so we now want to treat REMOVE much like NONE.

Further changes for review:

My contact-list-fixes branch might have some extra commits at the end that you haven't seen yet. I think it's probably just one, "constants: update for stable telepathy-spec".

Following that, contact-list-merge does a non-trivial merge of master into contact-list-fixes.

Following *that*:

(In reply to comment #12)
> > Also, I looked and saw that GabbleRosterSubscription is a set of flags, but
> > you're testing it as if it was a normal enum.

... my really-enum branch turns it into a real enum, since changes I made on master mean that it can safely be localized into roster.c.

I'd appreciate it if we could get this merged for 0.11 relatively soon; I'm also going to work on implementations for other connection managers.

(In reply to comment #13)
> Further changes for review:
> 
> My contact-list-fixes branch might have some extra commits at the end that you
> haven't seen yet. I think it's probably just one, "constants: update for stable
> telepathy-spec".
++
> 
> Following that, contact-list-merge does a non-trivial merge of master into
> contact-list-fixes.
I ended up doing a naive "pull everything in from both sides" merge, and viewing the diff between that and your merge commit. Seems it's just the old definition of interfaces_always_present that's different between the two, so ++

I kind-of wish there was a tool to do this. Shouldn't be hard to write, right?

> ... my really-enum branch turns it into a real enum, since changes I made on
> master mean that it can safely be localized into roster.c.
++

(In reply to comment #12)
> I could change it to REMOVED, but I think it might be clearer to stick to the
> XMPP phrasing.
...
> _subscription_to_string() and process_roster() assert this,
> _parse_item_subscription() can only return these, and I've just checked that no
> other states are assigned.
...
> I'm reasonably sure this is right: "no subscription and present on the contact
> list" and "no subscription and absent from the contact list" were previously
> conflated as NONE, so we now want to treat REMOVE much like NONE.

Are you sufficiently convinced by these to consider this branch to be ready for merge?

Fixed in git for 0.11.0 \o/