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

[bugzilla-daemon at freedesktop.org]

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

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://git.collabora.co.uk/ |http://git.collabora.co.uk/
                   |?p=user/andrunko/telepathy- |?p=user/gkiagia/telepathy-q
                   |qt4.git;a=shortlog;h=refs/h |t4.git;a=shortlog;h=refs/he
                   |eads/blocking               |ads/blocking
  Status Whiteboard|                            |review-

--- Comment #9 from Olli Salli <ollisal at gmail.com> 2011-04-15 06:44:01 PDT ---
OK so this became a really long review. But bear with me please, and try to
address all of the issues I've identified here.

     HandleIdentifierMap contactsIds = reply.value();
+    HandleIdentifierMap::const_iterator begin = contactsIds.constBegin();

Still, contactIDs, not the double-plural form please.

+        ContactPtr contact = contactManager->ensureContact(bareHandle,
+                id, conn->contactFactory()->features());
+        contact->setBlocked(true);
+    }

Won't these Contacts just get destroyed at that point if ensureContact created
them, making this a no-op? And created will they be, as this is part of the
initial introspection chain: there might be no other contact objects at that
point except for maybe the self contact.

You can't in general create contacts like this without going the normal contact
buildup route, because you might be on a service which doesn't have immortal
handles. For those services, if you don't hold the handles (by using
contactsForHandles() / contactsForIdentifiers()), the actual contact
represented by the handle can change arbitrarily as the handle is recycled.

-    // FIXME build contacts async, update contacts blocked state and
-    //       cachedBlockedContacts. Call introspectContactList when
-    //       finished
-    */

Yeah, so do what Andre's FIXME says, and don't just remove it :) You should
additionally inject the ids like you're doing in the change notification slot,
though.

This is perhaps most easily accomplished by queuing a "fake" blocked contacts
change event with the initial contacts in added and none in removed. That is,
if there are in fact some initial contacts - otherwise just skip that step.

You should include a test for having initial contacts as blocked in the service
(use tp-glib service API to set them as blocked) before starting making
FeatureRoster ready, and check that those contacts are correctly reported as
blocked. This would currently fail, I believe, if this codepath is used, and
the Contact objects didn't somehow exist already (e.g. by creating them
beforehand to block them using tp-qt4 API in the first place).

+void
ContactManager::Roster::onContactListBlockedContactsConstructed(Tp::PendingOperation
*op)

Similarly, you don't take refs to the contacts in this method. Hence, if the
contacts didn't exist otherwise, they will be released as soon as the
PendingContacts destroys itself in the next mainloop iteration, and you'll lose
the fact that they were blocked. This is what the "cachedBlockedContacts"
variable was for. It's a slightly bad name though, I'd use just
blockedContacts. The "cached" in the cachedAllKnownContacts var refers to how
it works as a cache of the union of the sub/pub/known lists when using
old-school separate ContactList channels.

The test (if it is even executed, see below) wouldn't hit this oversight,
because it just blocks contacts which are also in allKnownContacts (the "list
of friends"). You should also test blocking some completely unrelated contact,
and then later checking that that person is indicated as being blocked (by
releasing the test's reference to the contact in between, and re-requesting the
contact afterwards, checking if they're still reported as being blocked at that
stage.

You should then add a new blockedContacts() method to ContactManager, which'd
return the currently blocked contacts (just that set when using the new API,
the members of the "deny" group channel when on the old API, if there is one.
AND, you should fix the allKnownContacts() method for the old API to not
include the contacts from the "deny" list unless they're also on some other
list (i.e. ignore the deny list). Otherwise, removeContacts() doesn't make
sense: either it won't remove contacts from allKnownContacts() in all cases (if
the contacts were also blocked) or it will also have to remove their block
status (clearly wrong, if you're removing somebody you previously had blocked
as a friend, you don't want to unblock them).

Adding blockedContacts() is additionally justified by the fact that to unblock
people sensibly, you have to know who the blocked people are in the first
place. And if the contacts aren't necessarily in allKnownContacts(), like they
shouldn't be, it isn't guaranteed you could discover them by iterating over
allKnownContacts() either.

Of course, the above changes to allKnownContacts() are semantically backwards
incompatible, even if they remain ABI-compatible. This is not an issue however,
as we're going to branch out a stable 0.6 branch when your work is finished,
and   merge your branch to a new master, starting the 0.7 release series which
can subtly break semantics. Given the HORRIBLE semantics of the old API, I
estimate that not too many people even used it, though, so this is even less of
an issue.

Did you verify if the test for blocking is actually run, which it previously
didn't appear to be? As I originally commented, having such conditionally
executed blocks in tests is always wrong. Ideally you would test both cases,
but as the very minimum you have to QVERIFY that the service IS reported to
support blocking and then unconditionally execute the rest of the test - and
the same for reporting abuse later on in the test.

Once you've made sure the blocking test is actually run, you should check the
test coverage by building with ENABLE_COMPILER_COVERAGE and running make
lcov-check. Especially important is that the old "deny" channel codepath is
still tested for regressions. As much as possible of your new codepath should
be tested too, of course.

-- 
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.