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

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*

Mmm, I wonder

> cmake: Fix FindDBusGLib.cmake to properly search for dbus-glib-1 instead of dbus-glib.

How did this work before? Didn't we use it at all or something? Or is there now some chance that an installation where it worked before, is now broken?cmake: 

> Also export DBUS_GLIB_LOWLEVEL_INCLUDE_DIR so one can include <dbus/dbus-glib-lowlevel.h>.

Uhm, usually library include dir variables are per-library, not per-header. Is there some chance that they aren't in the same directory? If not, we don't do QT_QDBUS_ARGUMENT_INCLUDE_DIR, QT_QDBUS_CONNECTION_INCLUDE_DIR etc either so I don't see why we should do this.

> future/conn-addressing: Add test glib connection implementing Conn.I.Addressing.

Did you write this yourself, such that it's not a tp-glib example CM (which it doesn't look like, because it depends on TpTests stuff?). If so, please rename to TESTS_... like the other tests-only bits.

Note that some of our test CMs are named differently specifically because they're (derived from or directly) tp-glib example (parts of documentation for the library) CMs.

+    // FeatureAvatarData depends on FeatureAvatarToken
+    if (realFeatures.contains(Contact::FeatureAvatarData) &&
+        !realFeatures.contains(Contact::FeatureAvatarToken)) {

.... and the lines following it, are duplicated between the various ContactManager contact requesting functions. Utility fn taking features, making the real features out of them and acting (ensuring tracking and returning ifaces) accordingly?

@@ -565,10 +570,22 @@ void PendingContacts::onAddressingGetContactsFinished(PendingOperation *operatio

This code extracting the attributes and the referenced handles etc also looks awfully familiar. Could you please refactor it so that it shares the code with the codepath I originally wrote that in, rather than copypasting?

+ *
+ * See addressesable addresses specific methods' documentation for more details.
+ *
+ * \sa addressableVCardAddresses(), addressableUris()

The "See..." line is rather vague, but also redundant with the semantical \sa line, so I'd just remove it. The actual \sa's in this commit look very good in general though.

+ * Requesting contacts using ContactManager::contactsForVCardAddresses()
+ * by passing one of the various vCard addresses returned here
+ * will succeed and include this contact in the returned PendingContacts.

I don't think that's the point. If you already have the Contact, why would you still request it? These accessors are more so used to link the contacts by addresses in some other system, aren't they? So you could document in the ContactManager request functions that the returned contacts will likely have matching addresses, but using this information in the other way around makes less sense to me.

You can preserve the \sa here though - generally, I don't want \sa's which are redundant with autolinks in the full text description, but as that description doesn't make sense here, as much as in ContactManager, I think you should move the description away from here (completely, or merging with related ContactManager docs), but keep the \sa which is good.

+ * address in Contact::*() that can identify it.

-> "address that can identify it in Contact::*"

Otherwise looks good. We should make a mental note, however, of splitting PendingContacts to a set of internal sub-classes, with one for each (or a few related) different kinds of requests - but sharing most of the looking up cached contacts etc code, and that signaling the results. These huge if-else mazes for "which type of an object I REALLY am this time around" would disappear with that. But don't need to do that now, as long as the current code works and has reasonable test coverage.

Fine to merge after considering these notes - but of course, proto.i.addressing branch is now blocking this. Will finish reviewing that next.

(In reply to comment #3)
> Mmm, I wonder
> 
> > cmake: Fix FindDBusGLib.cmake to properly search for dbus-glib-1 instead of dbus-glib.
> 
> How did this work before? Didn't we use it at all or something? Or is there now
> some chance that an installation where it worked before, is now broken?cmake: 

It was working before cause most distros install dbus-glib in /usr/include, so even with pkg_check_modules failing it was getting the headers from /usr/include/dbus-1.0/*
But now it properly uses pkg-config for tips on where to find the headers, as it should.

> > Also export DBUS_GLIB_LOWLEVEL_INCLUDE_DIR so one can include <dbus/dbus-glib-lowlevel.h>.
> 
> Uhm, usually library include dir variables are per-library, not per-header. Is
> there some chance that they aren't in the same directory? If not, we don't do
> QT_QDBUS_ARGUMENT_INCLUDE_DIR, QT_QDBUS_CONNECTION_INCLUDE_DIR etc either so I
> don't see why we should do this.

Most dbus-glib headers are installed in /usr/include/dbus-1.0/dbus here
but the dbus-arch-deps.h header is installed in /usr/lib64/dbus-1.0/include/dbus
The issue is that we need to use dbus_g_method_get_sender which is defined in dbus-lowlevel.h that includes dbus-arch-deps.h.
We didn't need it before cause we don't include dbus-lowlevel.h anywhere else, just in this new test CM I wrote.

But as agreed over IRC I merged both variables into one DBUS_GLIB_INCLUDE_DIRS (plural).

> > future/conn-addressing: Add test glib connection implementing Conn.I.Addressing.
> 
> Did you write this yourself, such that it's not a tp-glib example CM (which it
> doesn't look like, because it depends on TpTests stuff?). If so, please rename
> to TESTS_... like the other tests-only bits.
> 
> Note that some of our test CMs are named differently specifically because
> they're (derived from or directly) tp-glib example (parts of documentation for
> the library) CMs.
Yeah, I wrote it myself heh, renamed to use TpTests, etc as namespace

> +    // FeatureAvatarData depends on FeatureAvatarToken
> +    if (realFeatures.contains(Contact::FeatureAvatarData) &&
> +        !realFeatures.contains(Contact::FeatureAvatarToken)) {
> 
> .... and the lines following it, are duplicated between the various
> ContactManager contact requesting functions. Utility fn taking features, making
> the real features out of them and acting (ensuring tracking and returning
> ifaces) accordingly?
Done.

> @@ -565,10 +570,22 @@ void
> PendingContacts::onAddressingGetContactsFinished(PendingOperation *operatio
> 
> This code extracting the attributes and the referenced handles etc also looks
> awfully familiar. Could you please refactor it so that it shares the code with
> the codepath I originally wrote that in, rather than copypasting?
The code here is similar but has some few differences, so no change here.
 
> + *
> + * See addressesable addresses specific methods' documentation for more
> details.
> + *
> + * \sa addressableVCardAddresses(), addressableUris()
> 
> The "See..." line is rather vague, but also redundant with the semantical \sa
> line, so I'd just remove it. The actual \sa's in this commit look very good in
> general though.
Yes, I agree the "See ..." line is vague, but it's used everywhere else, even in all other contact features docs. So for keeping consistency I didn't change this.

> + * Requesting contacts using ContactManager::contactsForVCardAddresses()
> + * by passing one of the various vCard addresses returned here
> + * will succeed and include this contact in the returned PendingContacts.
> 
> I don't think that's the point. If you already have the Contact, why would you
> still request it? These accessors are more so used to link the contacts by
> addresses in some other system, aren't they? So you could document in the
> ContactManager request functions that the returned contacts will likely have
> matching addresses, but using this information in the other way around makes
> less sense to me.
> 
> You can preserve the \sa here though - generally, I don't want \sa's which are
> redundant with autolinks in the full text description, but as that description
> doesn't make sense here, as much as in ContactManager, I think you should move
> the description away from here (completely, or merging with related
> ContactManager docs), but keep the \sa which is good.
Done.

> + * address in Contact::*() that can identify it.
> 
> -> "address that can identify it in Contact::*"
Done
 
> Otherwise looks good. We should make a mental note, however, of splitting
> PendingContacts to a set of internal sub-classes, with one for each (or a few
> related) different kinds of requests - but sharing most of the looking up
> cached contacts etc code, and that signaling the results. These huge if-else
> mazes for "which type of an object I REALLY am this time around" would
> disappear with that. But don't need to do that now, as long as the current code
> works and has reasonable test coverage.
Yeah, we need to do that someday, the code is getting bigger and bigger and difficult to maintain. Added mental note.

I rebased the branch again on top of proto-addressing as I did some changes there. The last review commit is d9eaf0.

(In reply to comment #4)
> (In reply to comment #3)
> > Mmm, I wonder
> > 
> > > cmake: Fix FindDBusGLib.cmake to properly search for dbus-glib-1 instead of dbus-glib.
> > 
> > How did this work before? Didn't we use it at all or something? Or is there now
> > some chance that an installation where it worked before, is now broken?cmake: 
> 
> It was working before cause most distros install dbus-glib in /usr/include, so
> even with pkg_check_modules failing it was getting the headers from
> /usr/include/dbus-1.0/*
> But now it properly uses pkg-config for tips on where to find the headers, as
> it should.

/usr/include/dbus-1.0 is not /usr/include, though. Actually I think that might've been because some other library pkgconfig (telepathy-glib I guess?) pulled in the correct include path as a dependency or something. Dunno, anyway nice that it's now fixed.

> 
> > > Also export DBUS_GLIB_LOWLEVEL_INCLUDE_DIR so one can include <dbus/dbus-glib-lowlevel.h>.
> > 
> > Uhm, usually library include dir variables are per-library, not per-header. Is
> > there some chance that they aren't in the same directory? If not, we don't do
> > QT_QDBUS_ARGUMENT_INCLUDE_DIR, QT_QDBUS_CONNECTION_INCLUDE_DIR etc either so I
> > don't see why we should do this.
> 
> Most dbus-glib headers are installed in /usr/include/dbus-1.0/dbus here
> but the dbus-arch-deps.h header is installed in
> /usr/lib64/dbus-1.0/include/dbus
> The issue is that we need to use dbus_g_method_get_sender which is defined in
> dbus-lowlevel.h that includes dbus-arch-deps.h.
> We didn't need it before cause we don't include dbus-lowlevel.h anywhere else,
> just in this new test CM I wrote.
> 
> But as agreed over IRC I merged both variables into one DBUS_GLIB_INCLUDE_DIRS
> (plural).

Yeah, good now. Weird that the dbus-glib-1 pkgconfig file doesn't pull in this arch deps header correctly. Could be the distro packaging which is a bit faulty here - anyway, your new cmake lookup rules should find it in any case.

> > @@ -565,10 +570,22 @@ void
> > PendingContacts::onAddressingGetContactsFinished(PendingOperation *operatio
> > 
> > This code extracting the attributes and the referenced handles etc also looks
> > awfully familiar. Could you please refactor it so that it shares the code with
> > the codepath I originally wrote that in, rather than copypasting?
> The code here is similar but has some few differences, so no change here.
> 

++ for now, until PendingContacts is otherwise refactored some time in the future. Could you add a fixme to do that please? I believe we can then rearrange the code so that actually the total number of lines goes up slightly, but the complexity goes down severely and it's much easier to follow and make changes in isolation, including sharing the parts of code like this which can be shared.

> > + *
> > + * See addressesable addresses specific methods' documentation for more
> > details.
> > + *
> > + * \sa addressableVCardAddresses(), addressableUris()
> > 
> > The "See..." line is rather vague, but also redundant with the semantical \sa
> > line, so I'd just remove it. The actual \sa's in this commit look very good in
> > general though.
> Yes, I agree the "See ..." line is vague, but it's used everywhere else, even
> in all other contact features docs. So for keeping consistency I didn't change
> this.
> 

Well. Frankly put, you shouldn't duplicate crap just to be consistently crappy. We've tried to weed zero-information "see also" things out as much as possible, e.g. making the references in TextChannel reasonable as part of the last big doc cleanup you admirably carried out.

The important point here is that *there are no more details regarding the feature in the accessor documentation* so you can't "see" it to get more details on the feature. For some other features, there is, though yeah, some of them are just as wrong as this one which has now been written. But let's not worry about cleaning up them now, though still, let's not add MORE misleading references.

Anyway, do whatever you please with the other accessors, but let's not make these newly written docs misleading just for consistency. Merge after proto addressing branch is merged (just now last pass -reviewed it), and cleaning this thing up one way or another, please.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Mmm, I wonder
> > > 
> > > > cmake: Fix FindDBusGLib.cmake to properly search for dbus-glib-1 instead of dbus-glib.
> > > 
> > > How did this work before? Didn't we use it at all or something? Or is there now
> > > some chance that an installation where it worked before, is now broken?cmake: 
> > 
> > It was working before cause most distros install dbus-glib in /usr/include, so
> > even with pkg_check_modules failing it was getting the headers from
> > /usr/include/dbus-1.0/*
> > But now it properly uses pkg-config for tips on where to find the headers, as
> > it should.
It is because we were searching for dbus-1.0/dbus/dbus-glib.h

find_path(DBUS_GLIB_INCLUDE_DIR
          NAMES dbus-1.0/dbus/dbus-glib.h
...

So /usr/include/dbus-1.0/dbus/dbus-glib.h was being found.
 
> > > @@ -565,10 +570,22 @@ void
> > > PendingContacts::onAddressingGetContactsFinished(PendingOperation *operatio
> > > 
> > > This code extracting the attributes and the referenced handles etc also looks
> > > awfully familiar. Could you please refactor it so that it shares the code with
> > > the codepath I originally wrote that in, rather than copypasting?
> > The code here is similar but has some few differences, so no change here.
> > 
> 
> ++ for now, until PendingContacts is otherwise refactored some time in the
> future. Could you add a fixme to do that please? I believe we can then
> rearrange the code so that actually the total number of lines goes up slightly,
> but the complexity goes down severely and it's much easier to follow and make
> changes in isolation, including sharing the parts of code like this which can
> be shared.
FIXME added to pending-contacts.cpp, to refactor it in the future

> > > + *
> > > + * See addressesable addresses specific methods' documentation for more
> > > details.
> > > + *
> > > + * \sa addressableVCardAddresses(), addressableUris()
> > > 
> > > The "See..." line is rather vague, but also redundant with the semantical \sa
> > > line, so I'd just remove it. The actual \sa's in this commit look very good in
> > > general though.
> > Yes, I agree the "See ..." line is vague, but it's used everywhere else, even
> > in all other contact features docs. So for keeping consistency I didn't change
> > this.
> > 
> 
> Well. Frankly put, you shouldn't duplicate crap just to be consistently crappy.
> We've tried to weed zero-information "see also" things out as much as possible,
> e.g. making the references in TextChannel reasonable as part of the last big
> doc cleanup you admirably carried out.
> 
> The important point here is that *there are no more details regarding the
> feature in the accessor documentation* so you can't "see" it to get more
> details on the feature. For some other features, there is, though yeah, some of
> them are just as wrong as this one which has now been written. But let's not
> worry about cleaning up them now, though still, let's not add MORE misleading
> references.
> 
> Anyway, do whatever you please with the other accessors, but let's not make
> these newly written docs misleading just for consistency. Merge after proto
> addressing branch is merged (just now last pass -reviewed it), and cleaning
> this thing up one way or another, please.
Ok, done, also removed the "see also" from the other Contact feature docs.

Thanks for the review, will merge when merging proto-addressing.

Changes merged upstream, it will be in tp-qt 0.9.0