Branch at URL attempts to fix this.

Comments:

- Use requestAllProperties() instead of direct Properties iface GetAll QDBusPendingCall

> ProtocolInfo: Add connectionManager() accessor and make isValid() check if CM is still valid.

It's a bit questionable to do this in this branch. The ProtocolInfos aren't really standalone objects - just records which ConnectionManager contains a list of, and Account has one. So it doesn't make sense to store a ProtocolInfo reference somewhere which can't access the ConnectionManager easily anyway (either directly or through an Account), but still can guarantee it stays alive.

It seems to me you only added this for constructing the Proto.I.Addressing proxy when needed. I think you should rather just store the manager's dbus connection, service name and object path on ProtocolInfo initialization instead, and use them later for constructing the proxy. Then the CM doesn't even need to stay alive for that API to work (less preconditions for users to worry about).

If there is some clear reason I'm not seeing why ProtocolInfo::connectionManager() makes sense, please remove ProtocolInfo::cmName() etc completely then, as it's wholly redundant with just doing connectionManager()->name() etc.

+ * An example would be a vCard TEL field with a formatted number in the form of
+ * +1 (206) 555 1234, this would be normalized to +12065551234.

-> For example, a vCard TEL field formatted as +1 (206)... could be normalized to +1206...

+ * \param vcardField The vCard field of the address we are normalizing.

The address is the value of the vCard field, an address doesn't have multiple vCard fields, which we could be normalizing one of. So "The vCard field the address belongs to" or something?

+ *         when the \a uri has being normalized or an error occurred.

has been normalized (in 2 places)

There's also a glaring omission: you don't use the immutable properties introspecting ConnectionManager.Protocols gives you. Note that it's a dictionary of *fully qualified* (multiple interfaces) property values. In fact it seems currently they're used only for the core Protocol interface properties, but separate GetAll calls are still done for all the auxiliary interface properties. This is inefficient - so please utilize them for the two Addressing props here, both of which are immutable as far as I can see.

Please also file separate bugs to use the immutable props for Avatars and Presence ifaces too. You don't have to worry about them in the context of this work though. But later on, we have to fix that as well.

This is especially inexcusable, because we originally justified not having ConnectionManager::FeaturePresence, FeatureAvatars etc by "they can anyway be got from the Properties with no additional cost for each protocol". But currently we make up to three additional GetAll calls for each protocol, even if the immutable properties map common to all protocols actually included properties for them too! So instead of 1 call to GetAll everything in ConnectionManager, we do that, and in addition up to 3*number of protocols GetAll calls. This makes ConnectionManager/Account::FeatureProtocolInfo introspection way slower than it should be.

(In reply to comment #2)
> Comments:
> 
> - Use requestAllProperties() instead of direct Properties iface GetAll
> QDBusPendingCall
Done, also done for the other Protocol interfaces and the main ConnectionManager interface while at it.

> > ProtocolInfo: Add connectionManager() accessor and make isValid() check if CM is still valid.
> 
> It's a bit questionable to do this in this branch. The ProtocolInfos aren't
> really standalone objects - just records which ConnectionManager contains a
> list of, and Account has one. So it doesn't make sense to store a ProtocolInfo
> reference somewhere which can't access the ConnectionManager easily anyway
> (either directly or through an Account), but still can guarantee it stays
> alive.
> 
> It seems to me you only added this for constructing the Proto.I.Addressing
> proxy when needed. I think you should rather just store the manager's dbus
> connection, service name and object path on ProtocolInfo initialization
> instead, and use them later for constructing the proxy. Then the CM doesn't
> even need to stay alive for that API to work (less preconditions for users to
> worry about).
> 
> If there is some clear reason I'm not seeing why
> ProtocolInfo::connectionManager() makes sense, please remove
> ProtocolInfo::cmName() etc completely then, as it's wholly redundant with just
> doing connectionManager()->name() etc.
Yes, I added the constructor param only to get the dbus connection, bus name and object path from the ConnectionManager object to build the Proto.I.Addr proxy. I've now removed the accessor but left the constructor receiving the ConnectionManagerPtr object, so I can retrieve the info above + the CM name from it without passing 4 params.
We can always remove, update this constructor later without breaking API/ABI as this is an unexported private constructor.
ProtocolInfo::isValid() also only cares about mPriv.constData() again.

> + * An example would be a vCard TEL field with a formatted number in the form
> of
> + * +1 (206) 555 1234, this would be normalized to +12065551234.
> 
> -> For example, a vCard TEL field formatted as +1 (206)... could be normalized
> to +1206...
Done

> + * \param vcardField The vCard field of the address we are normalizing.
> 
> The address is the value of the vCard field, an address doesn't have multiple
> vCard fields, which we could be normalizing one of. So "The vCard field the
> address belongs to" or something?
Done
 
> + *         when the \a uri has being normalized or an error occurred.
> 
> has been normalized (in 2 places)
Done
 
> There's also a glaring omission: you don't use the immutable properties
> introspecting ConnectionManager.Protocols gives you. Note that it's a
> dictionary of *fully qualified* (multiple interfaces) property values. In fact
> it seems currently they're used only for the core Protocol interface
> properties, but separate GetAll calls are still done for all the auxiliary
> interface properties. This is inefficient - so please utilize them for the two
> Addressing props here, both of which are immutable as far as I can see.
> 
> Please also file separate bugs to use the immutable props for Avatars and
> Presence ifaces too. You don't have to worry about them in the context of this
> work though. But later on, we have to fix that as well.
> 
> This is especially inexcusable, because we originally justified not having
> ConnectionManager::FeaturePresence, FeatureAvatars etc by "they can anyway be
> got from the Properties with no additional cost for each protocol". But
> currently we make up to three additional GetAll calls for each protocol, even
> if the immutable properties map common to all protocols actually included
> properties for them too! So instead of 1 call to GetAll everything in
> ConnectionManager, we do that, and in addition up to 3*number of protocols
> GetAll calls. This makes ConnectionManager/Account::FeatureProtocolInfo
> introspection way slower than it should be.
Done. Actually refactored this code for all Protocol interfaces at once, it was easier. Now if all the info needed is in the immutable properties no introspection will be made.

(In reply to comment #3)
> (In reply to comment #2)
> > Comments:
> > 
> > - Use requestAllProperties() instead of direct Properties iface GetAll
> > QDBusPendingCall
> Done, also done for the other Protocol interfaces and the main
> ConnectionManager interface while at it.

Great!

> 
> > > ProtocolInfo: Add connectionManager() accessor and make isValid() check if CM is still valid.
> > 
> > It's a bit questionable to do this in this branch. The ProtocolInfos aren't
> > really standalone objects - just records which ConnectionManager contains a
> > list of, and Account has one. So it doesn't make sense to store a ProtocolInfo
> > reference somewhere which can't access the ConnectionManager easily anyway
> > (either directly or through an Account), but still can guarantee it stays
> > alive.
> > 
> > It seems to me you only added this for constructing the Proto.I.Addressing
> > proxy when needed. I think you should rather just store the manager's dbus
> > connection, service name and object path on ProtocolInfo initialization
> > instead, and use them later for constructing the proxy. Then the CM doesn't
> > even need to stay alive for that API to work (less preconditions for users to
> > worry about).
> > 
> Yes, I added the constructor param only to get the dbus connection, bus name
> and object path from the ConnectionManager object to build the Proto.I.Addr
> proxy. I've now removed the accessor but left the constructor receiving the
> ConnectionManagerPtr object, so I can retrieve the info above + the CM name
> from it without passing 4 params.
> We can always remove, update this constructor later without breaking API/ABI as
> this is an unexported private constructor.
> ProtocolInfo::isValid() also only cares about mPriv.constData() again.
> 

Yeah, looks good now. The constructor is fine, what matters is only storing the needed info - how to pass them to the ctor is no issue.

> > + * An example would be a vCard TEL field with a formatted number in the form
> > of
> > + * +1 (206) 555 1234, this would be normalized to +12065551234.
> > 
> > -> For example, a vCard TEL field formatted as +1 (206)... could be normalized
> > to +1206...
> Done

Still says "a formatted number in the form of". If you want it to say "number", then "a number in a vCard TEL field formatted as +1 (234) 567 could be normalized to +1234567" would be better.

> Done. Actually refactored this code for all Protocol interfaces at once, it was
> easier. Now if all the info needed is in the immutable properties no
> introspection will be made.

Great, that you did this for all the ifaces! Could you test all three of these cases:

1) all info in .manager file
2) no .manager file, but some immutable properties present, though not all
3) all immutable props present

+    bool loadImmutableProperties();
+    bool loadMainProperties(const QVariantMap &props);
+    bool loadAvatarsProperties(const QVariantMap &props);
+    bool loadPresenceProperties(const QVariantMap &props);
+    bool loadAddressingProperties(const QVariantMap &props);

To be consistent with the rest of tp-qt, we should call these kinds of functions extract<iface short name>Properties(). See e.g. Channel and its subclasses. Also usually they don't have a bool return value - and except for loadImmutableProperties, I don't think you're using them either. So you could take the return values out, except maybe for loadImmutableProperties.

(In reply to comment #4)
> > > + * An example would be a vCard TEL field with a formatted number in the form
> > > of
> > > + * +1 (206) 555 1234, this would be normalized to +12065551234.
> > > 
> > > -> For example, a vCard TEL field formatted as +1 (206)... could be normalized
> > > to +1206...
> > Done
> 
> Still says "a formatted number in the form of". If you want it to say "number",
> then "a number in a vCard TEL field formatted as +1 (234) 567 could be
> normalized to +1234567" would be better.
Done
 
> > Done. Actually refactored this code for all Protocol interfaces at once, it was
> > easier. Now if all the info needed is in the immutable properties no
> > introspection will be made.
> 
> Great, that you did this for all the ifaces! Could you test all three of these
> cases:
> 
> 1) all info in .manager file
> 2) no .manager file, but some immutable properties present, though not all
> 3) all immutable props present
Tried hard to implement this but always got into some issue. 

I tried adding some members to ExampleEcho2Protocol holding the counters of how many times the introspection methods were called. The problem is that we inherit TpBaseProtocol that only expose some methods for introspection, and some of these methods are called in the constructor of TpBaseProtocol among others. So I can't reliably check the counters.

With that I cannot test that all the info is in the .manager file and no introspection was made, as the introspection methods are always called when creating a TpBaseProtocol.

As for checking immutable props, what happens now is that we are testing (3) always, cause TpBaseProtocol also automatically fill the immutable properties by calling the introspection callbacks when appropriate.

I also thought about adding a .manager file containing all the info we need for Protocol* and create a ContactManager instance with the name of the file (the CM name) and check if all the info was retrieved. The issue is that this does not mean that even returning the valid data, no extra D-Bus roundtrip was made. 
 
Ideas, suggestions?

> +    bool loadImmutableProperties();
> +    bool loadMainProperties(const QVariantMap &props);
> +    bool loadAvatarsProperties(const QVariantMap &props);
> +    bool loadPresenceProperties(const QVariantMap &props);
> +    bool loadAddressingProperties(const QVariantMap &props);
> 
> To be consistent with the rest of tp-qt, we should call these kinds of
> functions extract<iface short name>Properties(). See e.g. Channel and its
> subclasses. Also usually they don't have a bool return value - and except for
> loadImmutableProperties, I don't think you're using them either. So you could
> take the return values out, except maybe for loadImmutableProperties.
Done

(In reply to comment #5)
> I tried adding some members to ExampleEcho2Protocol

Please do things like this in a subclass (or fork it and rename it, InstrumentedProtocol or something); ExampleEcho2Protocol is meant to be a simple example for CM authors, and if it contains this sort of instrumentation, it's no longer a good example. (Yes, I still think files that also exist in tp-glib should be in sync.)

It sounds as though you actually need at least three TpBaseProtocol subclasses: one with a full .manager file, one with a partial .manager file and one with no .manager file at all.

> The problem is that we
> inherit TpBaseProtocol that only expose some methods for introspection, and
> some of these methods are called in the constructor of TpBaseProtocol among
> others. So I can't reliably check the counters.

I can see two ways round this, neither of which is suitable to appear in an example:

* put conflicting information in the manager file and the code, so you can
  always tell where it came from; in the "partial manager file" case,
  accept either version

* override and reimplement the D-Bus interface (i.e. don't use
  TpBaseProtocol's implementation)

Actually: 

    QMap<QString, QString> addressableVCardAddresses() const;
    QStringList addressableUris() const;

addressable addresses? addressable URIs? These are actually VCard fields the contact is addressable WITH, ditto Uris. What's wrong with just vCardAddresses() and uris(). (vCardAddressesAdressableWith(), while correctly expressing what they are, would be too clumsy IMO).

Similarly, we don't have identifiableId. Or idIdentifiableWith :) We have id().

(In reply to comment #7)
> Actually: 
> 
>     QMap<QString, QString> addressableVCardAddresses() const;
>     QStringList addressableUris() const;
>
> addressable addresses? addressable URIs? These are actually VCard fields the
> contact is addressable WITH, ditto Uris. What's wrong with just
> vCardAddresses() and uris(). (vCardAddressesAdressableWith(), while correctly
> expressing what they are, would be too clumsy IMO).
> 
> Similarly, we don't have identifiableId. Or idIdentifiableWith :) We have id().
Actually this is from bug #43599 (Conn.I.Addressing), but will address it there.

Branch updated with the following tests:
1) all info in .manager file
2) no .manager file, but some immutable properties present, though not all
3) all immutable props present
4) no .manager file, no immutable properties present

I used adaptors here as it was the best way to properly test this without adding backdoors.

I also fixed an issue with the introspection code.

(In reply to comment #9)
> Branch updated with the following tests:
> 1) all info in .manager file
> 2) no .manager file, but some immutable properties present, though not all
> 3) all immutable props present
> 4) no .manager file, no immutable properties present
> 

The tests look nice, except for one thing:

+    static int introspectionCalled;

Why are these statics? Apart from the reset thing (which should be a method on the CM helper object, or perhaps just creating a new CMHelper object in the beginning of each test case as needed, and deleting it in cleanup() IMO), you seem to access them through an instance pointer. This looks a bit messy to me, could you please clean it up slightly?

Having them as non-statics would make the test more flexible wrt. future changes, especially ones where we have multiple adaptors of a single kind (for multiple protocols) instantiated at a time, so we should do it unless there is some really big benefit for the current usage in making them statics.

> I used adaptors here as it was the best way to properly test this without
> adding backdoors.
> 

Indeed, the backdoors might have been a bit complicated to check all the cases? Anyway, great that you went that (bothersome) distance to implement the fake services instead.

> I also fixed an issue with the introspection code.

Ah, it didn't properly extract the interfaces from received properties before checking for them to decide what to introspect next? Very good fix - this is why we unit-test our code! (cf. http://blogs.msdn.com/cfs-filesystemfile.ashx/__key/communityserver-blogs-components-weblogfiles/00-00-01-32-02-metablogapi/7317.image_5F00_0F65063B.png)

I assume the test coverage is now very much in the green zone, with all these important tests?

Overall, a very good job. After perhaps cleaning up the introspection counters a bit, please merge, along with the conn.i.addressing branch you based on top of this.

(In reply to comment #10)
> (In reply to comment #9)
> > Branch updated with the following tests:
> > 1) all info in .manager file
> > 2) no .manager file, but some immutable properties present, though not all
> > 3) all immutable props present
> > 4) no .manager file, no immutable properties present
> > 
> 
> The tests look nice, except for one thing:
> 
> +    static int introspectionCalled;
> 
> Why are these statics? Apart from the reset thing (which should be a method on
> the CM helper object, or perhaps just creating a new CMHelper object in the
> beginning of each test case as needed, and deleting it in cleanup() IMO), you
> seem to access them through an instance pointer. This looks a bit messy to me,
> could you please clean it up slightly?
> 
> Having them as non-statics would make the test more flexible wrt. future
> changes, especially ones where we have multiple adaptors of a single kind (for
> multiple protocols) instantiated at a time, so we should do it unless there is
> some really big benefit for the current usage in making them statics.
Done.

> > I also fixed an issue with the introspection code.
> 
> Ah, it didn't properly extract the interfaces from received properties before
> checking for them to decide what to introspect next? Very good fix - this is
> why we unit-test our code! (cf.
> http://blogs.msdn.com/cfs-filesystemfile.ashx/__key/communityserver-blogs-components-weblogfiles/00-00-01-32-02-metablogapi/7317.image_5F00_0F65063B.png)
lol :)

> I assume the test coverage is now very much in the green zone, with all these
> important tests?
Yes, the new Protocol bits are basically fully tested and ConnectionManager coverage raised to 79.1% now.

> Overall, a very good job. After perhaps cleaning up the introspection counters
> a bit, please merge, along with the conn.i.addressing branch you based on top
> of this.
Thanks, very good review also.

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