[Bug 23284] Add support for ContactCapabilities interface

[bugzilla-daemon at freedesktop.org]

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





--- Comment #7 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-10-06 11:27:47 PST ---
(In reply to comment #4)
> (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
> 

Done all changes


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