[Bug 23284] Add support for ContactCapabilities interface

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=23284





--- Comment #4 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-10-06 09:15:47 PST ---
(In reply to comment #3)
> My most serious objections are:
> 
> We don't seem to provide any way to distinguish between "capabilities not yet
> known" and "known to have no capabilities at all" (in the D-Bus API that's
> omission from the mapping vs. an empty list). That probably requires use of
> contains() when doing the setup? Perhaps we could have isEmpty() (true for both
> unknown and no-caps) and isUnknown() (only true for unknown)?
> 
> We can return a null pointer for peoples' capabilities (if the feature isn't
> ready, and perhaps also when they're really not known), which in practice seems
> very likely to crash UIs; when we support distinguishing between "caps not
> known" and "no caps at all", perhaps we could have a global singleton for "caps
> not known" and return a const pointer to that?
> 
> Every UI is required to implement the failover from contact capabilities to
> connection capabilities by itself, rather than us doing that automatically
> (with a way for advanced UIs to detect when it's happened) as I suggested at
> the design stage. Are you *absolutely* sure that your way is better? Why?
> 
> If so, it should be explicitly documented when to do that (isUnknown() returns
> true) and what you should probably do in response (get the connection
> capabilities).
> 
The basic idea here is as follows:

If ContactCapabilities interface is supported by the connection,
ContactManager::supportedFeatures will return a set that contains
Contact::FeatureCapabilities. If this set does not contain FeatureCapabilities,
the capabilities per user will not be known in any case, so the UI should use
Connection::capabilities instead.

The UI point of view:
1) Connection is ready
2) Connection::contactManager()::supportedFeatures set contains
Contact::FeatureCapabilities
3) Call ContactManager::upgradeContacts to enable Contact::FeatureCapabilities
4) Now all contacts which the capabilities were already advertised will return
a proper ContactCapabilities object on Contact::capabilities() method.
5) For contact that do not have yet known capabilities (not advertised) or for
those whose capabilities changed, the signal Contact::capabilitiesChanged()
will be fired.

========
1) Connection is ready:
2) Connection::contactManager()::supportedFeatures set does not contain
Contact::FeatureCapabilities
3) Fallback to Connection::capabilities if applicable

I don't see the need for a isUnknown/isEmpty in this scenario, and more than
that, having a ContactCapabilities object for contacts that are know to have no
capabilities support is having extra objects in memory for no gain. The user
should fallback to ContactManager::supportedFeatures for any feature that the
user may have and check the return of Contact::capabilities to know if
capabilities is already known, and connect to capabilitiesChanged for change
notification.

> ----
> 
> Less serious:
> 
> As already noted on IRC, we should have a configure check for
> <http://qt.gitorious.org/qt/qt/merge_requests/1657>, and class_ is ugly (I
> prefer cls).
> 
> +bool CapabilitiesBase::supportsUpgradingCalls() const
> +{
> +    QString channelType;
> +    foreach (const RequestableChannelClass &class_, mPriv->classes) {
> +        channelType = qdbus_cast<QString>(class_.fixedProperties.value(
> +                QLatin1String(TELEPATHY_INTERFACE_CHANNEL ".ChannelType")));
> +        if (channelType == TELEPATHY_INTERFACE_CHANNEL_TYPE_STREAMED_MEDIA &&
> +            !class_.allowedProperties.contains(
> +                QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_STREAMED_MEDIA
> ".ImmutableStreams"))) {
> +                // TODO should we test all classes that have channelType
> +                //      StreamedMedia or just one is fine?
> 
> I think in practice, if one channel class has immutable streams, they all will,
> so this is OK. Alter the comment to say so.
> 
> "Added CapabilitiesBase to build system." shouldn't be a separate patch; don't
> change it now, but in future, patches that add new source files should add them
> to the build at the same time.
> 
> Do we actually have ensureVideoCall etc. yet? We probably should, if the docs
> are going to reference them :-)
> 

I will work on that


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.