[Bug 35341] Use Connection.Interface.ContactBlocking in Tp::ContactManager

[bugzilla-daemon at freedesktop.org]

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

--- Comment #13 from Olli Salli <ollisal at gmail.com> 2011-04-22 06:48:49 PDT ---
If we're going to keep the blocked contacts in allKnownContacts(), as we
agreed, we might as well remove the blockedContacts() accessor for now as well.
It conveys no additional information if the blocked contacts can be discovered
from allKnownContacts() as well, and would need a change notification signal of
its own, which'd complicate the internals even more.

We can re-add it if/when we add full Contact filtering, at which point it'd be
frustrating to have a method called blockedContacts() already when we want to
add a new one with the same name, but using the new Contact filtering machinery
and returning a ContactSet or somesuch.

+    Contacts allChangedContacts = groupMembersAdded;
+    allChangedContacts.unite(groupMembersRemoved);
+    computeKnownContactsChanges(allChangedContacts, Contacts(),
+            Contacts(), Contacts(), Channel::GroupMemberChangeDetails());

You're adding to allKnownContacts even the contacts which were removed from the
lists. If allKnownContacts is intended to be kept as it is now, an union of the
contacts in all server-side stored contact lists, that won't do. When people
who weren't in the roster (pub/sub/known) cease to be blocked, they're not
stored server-side anymore. Your code would keep them in allKnownContacts(),
but after a reconnect they wouldn't be there.

So please augment the tests by checking that a contact who is not in the
roster, and has been unblocked, is no longer in allKnownContacts() either.

However, if you changed this to pass the removed contacts as the second
parameter, I think you'd hit the fact that computeKnownContactsChanges is
secretly a part of the fallback code-path, and only checks the deprecated
ContactList Channels for the contact having been removed completely, not just
from one list:

    // Check if realRemoved have been _really_ removed from all lists
    foreach (const ChannelInfo &contactListChannel, contactListChannels) {
        ChannelPtr channel = contactListChannel.channel;
        if (!channel) {
            continue;
        }
        realRemoved.subtract(channel->groupContacts());
        realRemoved.subtract(channel->groupLocalPendingContacts());
        realRemoved.subtract(channel->groupRemotePendingContacts());
    }

The result would be that even contacts who are still in the roster (e.g.
subscribed to) are removed from allKnownContacts when they are unblocked. So
also check in the test that they remain.

To get that to pass, I guess you need three sets:

1) cachedAllKnownContacts (mostly to cache the union of old-school ContactList
Channels and the sets from 2) and 3), and thus prevent emitting duplicate
signals while remaining efficient)
2) contactListContacts (the contacts from Conn.I.ContactList API, empty if
using pub/sub/known channels, with their member sets used instead)
3) blockedContacts (the contacts from Conn.I.ContactBlocking API, empty if
using the deny channel, with its member sets used instead)

Then, you can check the contactListContacts and blockedContacts sets in
computeKnownContactsChanges in addition to just checking the ContactList
Channels which we don't necessarily have.

Sigh... given the degree of how misleading and fragile it is, the entire roster
code could use a big rewrite. We'll definitely do that at least when we can
remove support for the ContactList Channels completely (telepathy 1.0 ?).

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