[Bug 28819] Implement Protocol interface support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from George Goldberg <grundleborg at googlemail.com> 2010-08-03 02:32:46 PDT ---
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).

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