[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

Andre Moreira Magalhaes <andrunko at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |43598

--- Comment #2 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-12-14 18:31:51 UTC ---
First of all, I rebased the branch on top of proto-addressing (bug #43598) so
that I can properly link the documentation to ProtocolInfo newly added methods.

(In reply to comment #1)
> +        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.
Done. Updated the code to use RequestType instead (now private to
PendingContacts).
I didn't use it at first cause, as you said, some values doesn't make sense for
that constructor, but as this is internal, we can make sure the proper values
are passed.

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

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

> 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)
Done

> +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.
Done, renamed to receiveAddresses/FeatureAddresses.

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

> 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:
Done

> 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.
Done, added to the docs in ProtocolInfo::normalize*

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