[Bug 43599] Add high-level API for Conn.I.Addressing interface

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Olli Salli <ollisal at gmail.com> 2011-12-11 12:28:19 PST ---
+        ListTypeId,

Mmm. I totally didn't understand what is a "list type" at first - had to go
through the entire code, to realize that it means that contacts are being built
from a list containing contact IDs. The type of the list passed in, which was
the first thing my brain tries to interpret this as, is QStringList - that
doesn't change. Maybe AddressTypeId, AddressTypeVCardField, AddressTypeUri
would be more readable?

BUT: Do you actually need both that enum AND
PendingContacts::Private::RequestType? They seem awfully similar. Both are
internal identifier types - private API - so not an issue even if all values
(Upgrade) doesn't make sense with the string list taking constructor.

Another option which might be about as readable: have the PendingContacts
string list constructor do the common part, but have static PendingContacts
*PendingContacts::forVCardAddresses() etc functions which do the right call.

In any case, you could have a single QStringList in PendingContacts::Private
representing the ids/vcard addresses/uris, as there's never but one of the
three at most in a single PendingContacts. The public accessors being separate
makes sense though. Dropping the "ListType" external-only enum and merging the
lists, you can drop all code from the PendingContacts::Private ctor you added
here too.

Similarly, you could merge the (in)validVCardAddresses private variables with
(in)validUris. There's again only one or the other.

In a way, I like the idea of splitting the Addressing contact request starting
and result receiving to a separate PendingOperation from PendingContacts, which
was getting quite crowded. However:

In PendingAGC:

+            connAddressingIface->GetContactsByURI(uris, QStringList()));

and then in PendingContacts using it:

+    PendingAddressingGetContacts *pa =
qobject_cast<PendingAddressingGetContacts *>(operation);
+    mPriv->nested = manager()->contactsForHandles(pa->validHandles(),
features());

i.e. you make a GetContactsByURI D-Bus round trip with no contact attribute
interfaces, ignore the resulting attributes (IDs, addresses, vcard fields only
though irrespective of features), and then make another D-Bus round trip to get
the contact attributes which were requested.

While this is unavoidable for contactsForIdentifiers(), as there is no
Conn.I.Contacts.GetContactAttributesForIDs or alike, and hence a nested
contactsForHandles() operation must be used, here this is directly causing
twice the latency which would be readily possible with the way the D-Bus
interface is designed - because it's from addresses to attributes directly.

So, you should pass the feature-specific interfaces to
GetContactsByURI/VCardField, and use the resulting contact attributes - there
is no need to make further calls. This requires you to be able to pass the
features  and/or interfaces down to the code making the GetContactsBy* calls,
and the resulting identifier -> (handle, attributes) mapping back for
PendingContacts result accessors to read. (The handle being principally needed
to associate the results with the correct existing contact from ContactManager
if one exists). This is an awful lot of data to shuffle through a PendingAGC
API - so I suspect it might be cleaner to just start the calls and process the
results in PendingContacts directly - with the data in the member variables.
See what works best, please.

The tests eventually need to stress a few peculiar things in addition to the
basics:
 * that existing contacts e.g. from contactsForIdentifiers or another
contactsForVCardFields/Uris (maybe with an another field / uri scheme) are
correctly looked up when doing a contactsForVCardFields/Uris with some of the
addresses referring to the existing contact (same Contact object is returned)
 * that requests where some addresses are invalid, some are valid work sensibly
 * requests for no contacts at all work as well consistently with other
contactsFor, upgradeContacts operations (while in general useless, this
shouldn't e.g. cause crashes or infinitely lasting operations or anything like
that because it's easy to do in code which can work on 0...* Contacts at a
time)

+void Contact::receiveAddressing(const QMap<QString, QString> &addresses,
+        const QStringList &uris)

Both the vcard addresses and uris are addresses for the contact. Note: all the
receive* methods are named like receiveX == "receive data X describing the
contact". Hence Contact::receiveAddresses(vcardAddresses, uris)? Ditto the
Feature could be called FeatureAddresses - the user is interested in seeing
addresses of the contact, not the info pertaining to the Addressing interface.

+    if (!mPriv->requestedFeatures.contains(FeatureAddressing)) {
+        return;
+    }
+

Doesn't need this because the addresses are immutable (no change notification)
- this is present in some other receive* functions to avoid emitting change
notification signals for features which haven't been requested.

The documentation when it appears needs to explain the relationship between the
ProtocolInfo addressing bits, ContactManager addressing operations and Contact
address attributes. In my understanding, this is something like:

if ProtocolInfo would normalize address X to Y, a successful ContactManager
contact request for address X would result in a contact with Y reported as an
address that can identify it. (With "address" being a vCard field value and an
URI as applicable).

Is this correct? Anyway, such an explanation would make nice documentation AND
also link up functions in the three APIs nicely.

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