[Bug 28819] Implement Protocol interface support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Andre Moreira Magalhaes <andrunko at gmail.com> 2010-08-03 05:21:03 PDT ---
(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

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