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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #12 from Olli Salli <ollisal at gmail.com> 2011-04-22 05:32:31 PDT ---
(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 at example.com and blocktest2 at 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...

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