[Bug 28199] port Gabble to TpBaseContactList

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=28199

--- Comment #11 from David Laban <david.laban at collabora.co.uk> 2010-09-20 06:13:54 PDT ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.