The branch in URL implements initial support for it. Review will follow.

So first things first. Let me explain a bit on how tp-qt4 deal with new new D-Bus interfaces additions.

The files in the spec/ directory are the same files found in telepathy-spec-<version>/spec for a given tp-spec version. In order to add new files released in a new spec, we first add all files from that spec, generate low-level bindings for all new interfaces (including adding pretty headers) and only then we use the new low-level bindings.

Said that I would like some changes in your branch scheme:
- First create a branch called spec-unreleased that adds the file spec/Connection_Interface_Contact_Blocking.xml, generates low-level bindings for it (including adding the pretty header). (no change in existing code here)
- After that edit your first patch at your blocking branch, remove the parts you already did in the branch spec-unreleased (above) and rebase the blocking branch against spec-unreleased.

This way we can integrate your branch easily when we merge the branch to update to the new spec (not your spec-unreleased branch, but a correct one adding everything from the new spec when that is released) and no conflict (probably) will happen from your blocking branch and current tp-qt4 master (which will include the new spec additions).

Also when doing this, please rebase both of your branches on top of current master.

Now for the code itself:
- In ContactManager::Roster::introspect() you don't want to introspect both the deny channel and the ContactBlocking iface, use the ContactBlocking iface if supported and the Deny Channel as fallback. You also want to call requestBlockedContacts _async_ here and only finish FeatureRoster introspection once you get the initial blocked contacts.

In resume:
Call introspectContactBlocking (adding the support to get the initial blocked contacts) in ::introspect not in ::onContactListChannelReady() if ContactBlocking is supported and do not call the deny channel introspection code in this case. Once ContactBlocking finishes introspection, call introspectContactList and you should be fine. Check the other code path for the deny channel, you may need some other changes.

- Coding style. Add space after ",", if, etc, check HACKING
+        return queuedFinishVoid(iface->BlockContacts(handles,reportAbuse));
Also here:
+        if(hasContactBlockingInterface) {


- _never_ call blocking D-Bus code inside tp-qt4 as done in:
 +        UIntList handles = iface->RequestBlockedContacts();

- _never_ change public API signature as you did in ContactManager::blockContacts, instead add a new overload method for this, without any default arg.

- Either remove ContactManager::blockedContacts() or make it return a cached list of blocked contacts (no D-Bus call).
  for i in allcontacts: if contact->isBlocked() ret << contact

That is it for now, I will check again once the branch is updated.

I've made the requested changes. 
You can find the blocking branch at http://git.collabora.co.uk/?p=user/asoliver/telepathy-qt4/.git;a=shortlog;h=refs/heads/blocking

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.

Oh crap. I ignored the WIP commit because I thought you said you amended the
earlier commits to fix issues :) I see you already spotted the need for the
async contact buildup and the changes from the latest spec. Some things I spot
in the WIP commit though:

In gotContactBlockingBlockedContacts:

+    Q_ASSERT(!cachedBlockedContacts.isEmpty());

Surely there can be no blocked contacts?

-    uint contactBlockingCaps = 
qdbus_cast<uint>(props[QLatin1String("ContactBlockingCapabilities")]);
-    canReportAbusive =  contactBlockingCaps &
ContactBlockingCapabilityCanReportAbusive;
+    uint contactBlockingCaps =  qdbus_cast<uint>(
+            props[QLatin1String("ContactBlockingCapabilities")]);

You could use requestPropertyContactBlockingCapabilities instead of the
allProperties method as you only need a single property. In addition to being
marginally more efficient, that way you can catch the error of the property not
being correctly implemented (using All it's just missing from the map and
you'll get a default-constructed value from qdbus_cast).

Most of the issues from my previous comment remain, though.

Making this block the spec update metabug.

(In reply to comment #4)
> 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.
> 

I see we already have similarly crappy API without the report abuse param. Please deprecate that, and have blockContacts(contacts, reportAbuse) and unblockContacts().

Updated branch with improvements according to review comments:

http://git.collabora.co.uk/?p=user/gkiagia/telepathy-qt4.git;a=shortlog;h=refs/heads/blocking

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.

I have updated again my branch to address Olli's comments.

Notes:

1) The test now depends on Will's telepathy-glib branch here:
http://git.collabora.co.uk/gitweb/?p=user/wjt/telepathy-glib.git;a=shortlog;h=refs/heads/blocking

I will update the tp-glib version requirement as soon as a new tp-glib version with this branch included has been released.

2) I have also tested the code path used with the old deny lists by locally reverting commit f736a0398a826afef0442c597681de4bfb194c0f (contactlist2 cm: Implement the ContactBlocking interface) and it works the same as the new code path (the test passes). Unfortunately I don't know of any better way to test the old code path (except from building the CM and the test twice, with some preprocessor directive to control if ContactBlocking will be enabled... should I do that?)

3) I am not so sure about the allKnownContacts() changes. To me, the method name implies that it contains *every* contact ever known to tp-qt4. Anyway, leaving that aside, I added some code that makes sure blocked contacts never appear in allKnownContacts(), however, I am not sure if re-adding unblocked contacts to allKnownContacts is the right thing to do. For example, in the test, blocktest1@example.com and blocktest2@example.com, which are both *not* in the contact list are blocked and no changes happen to allKnownContacts(); but if I unblock them, they will be added to allKnownContacts() and that doesn't feel right.

4) I am wondering whether we should fake the signature of ContactManager::blockContacts() and Contact::block() in the documentation to make it less confusing for the users. i.e. let doxygen believe that we have:

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

instead of:

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

what do you think?

So, after a quick irc discussion, we decided to let allKnownContacts() contain blocked contacts, so I adjusted the branch accordingly. Please review :)

(In reply to comment #10)
> I have updated again my branch to address Olli's comments.
> 
> Notes:
> 
> 1) The test now depends on Will's telepathy-glib branch here:
> http://git.collabora.co.uk/gitweb/?p=user/wjt/telepathy-glib.git;a=shortlog;h=refs/heads/blocking
> 
> I will update the tp-glib version requirement as soon as a new tp-glib version
> with this branch included has been released.

Makes sense. All the more reason to have this in a new 0.7 branch which requires a 0.15.x tp-glib as well, though.

> 
> 2) I have also tested the code path used with the old deny lists by locally
> reverting commit f736a0398a826afef0442c597681de4bfb194c0f (contactlist2 cm:
> Implement the ContactBlocking interface) and it works the same as the new code
> path (the test passes). Unfortunately I don't know of any better way to test
> the old code path (except from building the CM and the test twice, with some
> preprocessor directive to control if ContactBlocking will be enabled... should
> I do that?)

We have a conn-roster-legacy test, which tests the old subscribe/publish/stored lists instead of the newer Conn.I.ContactList interface. Make the contactlist (not 2) test CM implement a "block" channel if it doesn't already, and add the tests for the old code path to the legacy test?

I wouldn't object if you renamed contactlist to contactlist-legacy and contactlist2 to contactlist, btw...

> 
> 3) I am not so sure about the allKnownContacts() changes. To me, the method
> name implies that it contains *every* contact ever known to tp-qt4. Anyway,
> leaving that aside, I added some code that makes sure blocked contacts never
> appear in allKnownContacts(), however, I am not sure if re-adding unblocked
> contacts to allKnownContacts is the right thing to do. For example, in the
> test, blocktest1@example.com and blocktest2@example.com, which are both *not*
> in the contact list are blocked and no changes happen to allKnownContacts();
> but if I unblock them, they will be added to allKnownContacts() and that
> doesn't feel right.
> 

Yeah this was a bong idea. Sorry about that. However, it's never "all contacts ever known to tp-qt4", just the contacts stored in the server-side roster or similar. For example Contacts you see randomly in Jabber MUCs or IRC channels won't appear there.

> 4) I am wondering whether we should fake the signature of
> ContactManager::blockContacts() and Contact::block() in the documentation to
> make it less confusing for the users. i.e. let doxygen believe that we have:
> 
> TELEPATHY_QT4_DEPRECATED PendingOperation *blockContacts(
>     const QList<ContactPtr> &contacts, bool value);
> PendingOperation *blockContacts(
>     const QList<ContactPtr> &contacts);
> 
> instead of:
> 
> PendingOperation *blockContacts(
>     const QList<ContactPtr> &contacts, bool value = true);
> 
> what do you think?

You can actually have the first option there in reality, not just in doxygen, while remaining backwards compatible enough.

DEPRECATED and C++ default parameters are compile-time features, not a runtime ones, so calls like blockContacts(contacts) which previously compiled to invoking the now-deprecated function with a default parameter will invoke the single-parameter one with no warning, while blockContacts(contacts, false) will trigger the deprecation warning as expected.

The only questionable side effect is if somebody called blockContacts(contacts, true) explicitly, which will also get compiled as deprecated usage as a result: this is fine, we are going to remove the entire two-parameter function eventually, so we should be deprecating it fully, not just deprecating passing false to it. We aren't going to keep blockContacts(contacts) and blockContacts(contacts, true) as two valid means to achieve the exact same thing after an ABI break.

To reiterate, the fact that previously compiled code still invokes the two-parameter version even for calls like blockContacts(contacts) employing the default parameter is fine: it's going to be dynamically linked to our (deprecated) two-parameter wrapper function with no warnings.

More review input follows...

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

Just to let you know that my spec-0.22.0 branch (which includes autogen for ContactBlocking) is now merged in master and George's branch should be rebased on top of it.

So, branch updated again.

I rebased on top of master, as Andre suggested, and with that opportunity I edited and removed some commits.

1st edit is in:

1456eca366ca1ab907c375ede1117b6001580368 - ContactManager::Roster: Finish the implementation of Conn.I.ContactBlocking introspection

where I simply did not remove the blockedContacts variable just to re-add it later.


2nd edit is in:

4011e716dbfb7d0f1e78a20aa43af80994b760bb - Contact/ContactManager: Improve the public interface for blocking/unblocking contacts.

where I adjusted the public interface to have two methods:

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

instead of one with a default (and deprecated) argument.


Then, I removed the commit that added the blockedContacts() public method, as you suggested.


Real changes begin from:

8c0bd125263e87b00ab95bb0edf48eec79fa0789 - ContactManager::Roster: Introduce a new contactListContacts set to keep track of the Conn.I.ContactList contacts

We now have set up the 0.6 stable branch, so we have an unstable master to merge this work to. Please rebase on top of current master accordingly.

Other than that, observations from the current branch:

Your branch now seems to get the "removed from blocked, but still in other lists" case right. However, removing from other lists but still in blocked would still seem to be bugged and result in the contact being removed from allKnownContacts although it's still blocked, as ContactManager::Roster::onContactListNewContactsConstructed does this directly for all removed contacts:

>        cachedAllKnownContacts.remove(contact);
>        contactListContacts.remove(contact);
>        removed << contact;

Please add a test for this case (e.g. contact is subscribed to, but also blocked, and the subscription is then removed).

Fixing it would be IMO conceptually cleanest by making the aforementioned method use computeKnownContactsChanges for committing and signaling the changes, and making that utility function also check the blocked contacts set (so that it then truly checks all contact lists, both old- and new-style) for the contact still being present. cachedAllKnownContacts should then never have contacts directly removed, always through that utility function which checks the other contact sets.

Otherwise looks fine to me now.

Branch updated.

Still in void ContactManager::Roster::onContactListNewContactsConstructed:

        if (!cachedAllKnownContacts.contains(contact)) {
            warning() << "Contact" << contact->id() << "removed from ContactList but not cached, "
                "ignoring.";
            continue;
        }

For clarity, now that we have the separate contactListContacts set, shouldn't we change the above to check that set instead (as that's what should be in sync with the service's list)? The warning message could be turned to a more descriptive one accordingly, like "removed from contact list, but wasn't present".

This won't have a difference unless there is a change signal with the same contact in both added and removed sets, but would be cleaner and also have better performance (for contacts which are still blocked, so still in all known) for filtering out redundant contact removals emitted by services (like Gabble seems to be doing in bug 35352).

Do you have Telepathy commit access? If yes, please merge after the above change, otherwise ping me and I'll merge for you.

This is done, and reviewed, but the tp-glib dependency still hasn't been merged. Still waiting for that.

(In reply to comment #18)
> Do you have Telepathy commit access? If yes, please merge after the above
> change, otherwise ping me and I'll merge for you.

Reiterating this question because the tp-glib is now merged and will be in tp-glib 0.15.1.

George?

(In reply to comment #20)
> (In reply to comment #18)
> > Do you have Telepathy commit access? If yes, please merge after the above
> > change, otherwise ping me and I'll merge for you.
> 
> Reiterating this question because the tp-glib is now merged and will be in
> tp-glib 0.15.1.
> 
> George?

No, I don't have commit access. Please merge when you can.

Thanks for the information, and your work on this branch. I'll merge it as soon as the corresponding tp-glib release has been made.

Now that tp-glib-0.15.1 has been released, I merged this to tp-qt4 master. Will be in the 0.7.0 release once that happens.