Bug 28819

Ping

Some review comments (it's a while since I've done any work on/with the TpQt4 source, so I'm a bit rusty and might have missed something, but here's a few comments). Also, I don't think I'm really qualified to review the glib bits.

One general comment: are these changes going to fit in with the profiles support in a reasonably sane way once it lands in the spec?

Specific comments:
==================

----- connection-manager.cpp -----

+/**
+ * Return the name of the most common vCard field used for this protocol's
+ * contact identifiers, normalized to lower case.
+ *
+ * \return The most common vCard field used for this protocol's contact
+ *         identifiers, or an empty string if there is no such field.
+ */
+QString ProtocolInfo::vcardField() const
+{
+    return mPriv->vcardField;
+}

I don't understand the point of this. Please can you clarify in the docs what this information is meant to be useful for (maybe give an example if there is a specific scenario in which API users should make use of this data).

-----

+/**
+ * Return the name of an icon for this protocol in the system's icon theme,
+ * such as "im-msn".
+ *
+ * \return The name of an icon for this protocol in the system's icon theme.
+ */
+QString ProtocolInfo::iconName() const
+{
+    return mPriv->iconName;
+}

Are these icon names in the freedesktop.org icon name spec? If not, they probably should be·

-----

+/**
+ * Return the ProtocolInfo object for the protocol specified by
+ * \a protocolName.
+ *
+ * This method requires ConnectionManager::FeatureCore to be enabled.
+ *
+ * \return A ProtocolInfo object or 0 if the protocol specified by \a
+ *         protocolName is not supported.
+ * \sa hasProtocol()
+ */
+ProtocolInfo *ConnectionManager::protocol(const QString &protocolName) const
+{
+    foreach (ProtocolInfo *info, mPriv->protocols) {
+        if (info->name() == protocolName) {
+            return info;
+        }
+    }
+    return 0;
+}

I think, just to be safe, the API docs should say here that the caller shouldn't delete the returned ProtocolInfo*.

----- connection-manager.h -----

-    PendingConnection *requestConnection(const QString &protocol,
+    PendingConnection *requestConnection(const QString &protocolName,

Is this change binary compatible? (I don't actually know, but it looks suspicious).

(In reply to comment #2)
> Some review comments (it's a while since I've done any work on/with the TpQt4
> source, so I'm a bit rusty and might have missed something, but here's a few
> comments). Also, I don't think I'm really qualified to review the glib bits.
> 
> One general comment: are these changes going to fit in with the profiles
> support in a reasonably sane way once it lands in the spec?
Yep, profiles support depend on this, as well as the support to filter accounts by RCC.

The idea is that you will be able to filter accounts by infos provided by protocol and profiles, as well as using the info from profiles to build account creation UIs.

> Specific comments:
> ==================
> 
> ----- connection-manager.cpp -----
> 
> +/**
> + * Return the name of the most common vCard field used for this protocol's
> + * contact identifiers, normalized to lower case.
> + *
> + * \return The most common vCard field used for this protocol's contact
> + *         identifiers, or an empty string if there is no such field.
> + */
> +QString ProtocolInfo::vcardField() const
> +{
> +    return mPriv->vcardField;
> +}
> 
> I don't understand the point of this. Please can you clarify in the docs what
> this information is meant to be useful for (maybe give an example if there is a
> specific scenario in which API users should make use of this data).
>
I will update the doc to explain better what this field means
 
> -----
> 
> +/**
> + * Return the name of an icon for this protocol in the system's icon theme,
> + * such as "im-msn".
> + *
> + * \return The name of an icon for this protocol in the system's icon theme.
> + */
> +QString ProtocolInfo::iconName() const
> +{
> +    return mPriv->iconName;
> +}
> 
> Are these icon names in the freedesktop.org icon name spec? If not, they
> probably should be·
> 
> -----
I am not sure about this, but I believe they are. As stated in the spec this is the name of icon in the system's icon theme.
 
> +/**
> + * Return the ProtocolInfo object for the protocol specified by
> + * \a protocolName.
> + *
> + * This method requires ConnectionManager::FeatureCore to be enabled.
> + *
> + * \return A ProtocolInfo object or 0 if the protocol specified by \a
> + *         protocolName is not supported.
> + * \sa hasProtocol()
> + */
> +ProtocolInfo *ConnectionManager::protocol(const QString &protocolName) const
> +{
> +    foreach (ProtocolInfo *info, mPriv->protocols) {
> +        if (info->name() == protocolName) {
> +            return info;
> +        }
> +    }
> +    return 0;
> +}
> 
> I think, just to be safe, the API docs should say here that the caller
> shouldn't delete the returned ProtocolInfo*.
> 
Sure thing, will do it.

> ----- connection-manager.h -----
> 
> -    PendingConnection *requestConnection(const QString &protocol,
> +    PendingConnection *requestConnection(const QString &protocolName,
> 
> Is this change binary compatible? (I don't actually know, but it looks
> suspicious).
Actually no, we are just renaming the param for consistency, actually there is no need to add param names at all. What really matters here is the signature of the method, which didn't change, so we are safe.

I will work on the issues you pointed and update here

Rebased against spec 0.19.10, as it depends on it now

EnglishName and Icon: The spec suggests a client could derive them from the protocol name if absent or empty, so I'd add this fallback to both ManagerFile::Private::parse() and ConnectionManager::Private::ProtocolWrapper::gotMainProperties(). The fact that the fallback is implemented should also be documented so that client authors reading the spec don't needlessly duplicate it in their applications. 

The spec leaves the actual mapping open, but I think the only sensible choice is icon == im-<protocol-name> and english name == <Protocol Name>, eg. for salut icon == im-local-xmpp and english name == Local Xmpp (sadly the acronym capitalization can't be correct).

--

ConnectionManager::onProtocolReady(): In the error case I think you should output the protocol name that errored out and was omitted. Tbh, I'm not sure if we should just fail the entire CM core introspection if a protocol fails, since the CM is somehow broken anyway if it can't respond to a simple GetAll on a protocol object it claims as having. I'm fine with this more lenient behavior as well though, as long as there's an indication (the warning) to indicate which protocols got left out, for troubleshooting purposes.

--

ConnectionManager::protocol(): Documentation addition to advise not to delete ++, but shouldn't it return a pointer to const in the first place?

Example for the vcard field could be eg. importing foreign address books employing vcard-alike formats, to map the contacts therein having eg. jabber addresses to a jabber account (would work for the simple cases). The vcard field is also used in some current implementations as the native tp-aware addressbook field key, but obviously this is not required.

Changing the param name preserves ABI on G++, but generally nothing can be said as C++ lacks a standard ABI specification :/ However we've generally gone by the G++ (3.2+) ABI rules, so this change is OK too.

--

Otherwise ++ and a clever (but not too clever :)) implementation! Will review account-filter-by-rcc next (tomorrow).

Feature merged to git master. Will be in 0.3.8.

After the last review, I fixed the branch to actually work sensibly with CMs which don't implement the Protocol objects from the new spec. We can emulate most of the functionality though. This also has the side effect of optimizing out redundant per-Protocol object GetAll calls for services conforming to current spec.

Additionally, I implemented the EnglishName fallback suggested in my previous comment.