[Bug 35341] Bind Connection.ContactBlocking

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Fri Mar 25 12:29:12 CET 2011


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/asoliver/telepathy- |?p=user/andrunko/telepathy-
                   |qt4/.git;a=shortlog;h=refs/ |qt4.git;a=shortlog;h=refs/h
                   |heads/blocking              |eads/blocking
             Status|NEW                         |ASSIGNED

--- Comment #4 from Olli Salli <ollisal at gmail.com> 2011-03-25 04:29:11 PDT ---
Andre further updated the branch. Some quick review comments:


+    PendingOperation *blockContacts(const QList<ContactPtr> &contacts, bool
value, bool reportAbuse);

I find a single method called "block" the "value" param of which changes it to
actually unblock exceedingly confusing, especially considering the reportAbuse
parameter doesn't make sense with "value = false".

So separate to blockContacts and unblockContacts, please.

+void ContactManager::Roster::onBlockedContactsChanged(Tp::UIntList added,
Tp::UIntList removed)
+{
+    Tp::Contacts addedContacts;
+    foreach (uint handle, added) {
+        Tp::ContactPtr contact =
contactManager->lookupContactByHandle(handle);
+        addedContacts.insert(contact);
+    }
+

This'll just insert a bunch of NULLs if the contacts being signaled as blocked
didn't happen to be already built elsewhere, and still referenced. Well, quite
often they exist otherwise, because they are (or were) previously in the
roster, but not necessarily - especially because we queue the other contact
list changes, so the contact appearing in the roster might actually be
processed (from the queue) later than it being blocked already!

Thus, the change events here must be put to the common roster event change
queue to preserve event order, and also must have their contacts built using
contactsForHandles just as for all other events.

Besides, it seems the spec here hasn't been updated to the final version which
carries the IDs in the signals (and doesn't have the signal this slot is being
connected to at all!). Be sure to update this code accordingly with the changes
from the stable spec in 0.22.0, injecting the IDs.

Also, shouldn't this slot eventually result into calling
updateContactsBlockState so the changes would be signaled to the application?
Currently it just updates cachedBlockedContacts.

The test:

+    // test if can block contacts
+    if (mConn->contactManager()->canBlockContacts()) {

and

+        // test the report abuse method
+        if (mConn->contactManager()->canReportAbuse()) {

You KNOW (from the service implementation), whether contact blocking is
supported or not - thus, you should QVERIFY the blocking and abuse reporting
capability, and then (if the service supports it, in fact) unconditionally
execute the blocking and abuse reporting tests.

Here, I believe, the test CM might not actually support blocking at all: thus
your entire test will be skipped. I actually believe this to be the case,
because the test tries to verify that blocking and unblocking contacts changes
their state, even though the state change signal doesn't seem to be connected
to the method which updates the contact state correctly, as previously
mentioned.

If the test was, in fact, executed, and the other changes to queue the contact
building for the signals correctly were made, it could also hit a race where
although the BlockContacts call is finished, the corresponding signal hasn't
yet been processed and the contact state has been changed. To fix that (and the
API semantics in general), the BlockContacts/UnblockContacts D-Bus call must be
signaled finished from the same queue too, like the rest of the roster methods.

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



More information about the telepathy-bugs mailing list