Bug 23284

The bindings aren't generated because the ContactCapabilities interface is still a draft and subject to change.

Hijacking this bug for the high-level API...

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

----

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 :-)

(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

From IRC, with some parallel conversations deleted:

17:20 < smcv> andrunko: what you want to be able to do in a UI is: should I 
              display a microphone icon in Empathy or not?
17:21 < smcv> andrunko: if the capabilities object is always non-NULL you can 
              just do contact->capabilities()->supportsAudioCalls()
17:21 < smcv> andrunko: but if the capabilities object can be NULL, you need to 
              put a pile of NULLness checks in to not crash
17:22 < smcv> andrunko: I confidently predict that basically all UIs will want 
              to treat "unknown caps" and "known to have no caps" as synonymous
17:22 < smcv> andrunko: and similarly, they'll want to treat XMPP 
              this-guy-can-do-audio as synonymous with SIP 
              people-can-probably-do-audio
17:23 < wjt> smcv: so why should Gabble expose the difference between the two?
17:23 < wjt> (i agree with you that no UI cares about the difference)
17:23 < smcv> wjt: a valid point. perhaps it shouldn't
17:23 < smcv> wjt: although in Gabble things are clouded by the fact that there 
              are "assumed capabilities" like Text
...
17:26 < smcv> andrunko: I think I'd find it easier to see what you intend about 
              caps if you could come up with a code snippet that e.g. Kopete 
              could use for "given this not-necessarily-ready Contact, should I 
              display a microphone icon?"
17:27 < smcv> andrunko: (i.e. it would run this code snippet on setup, when 
              upgradeCapabilities returned, and on every capabilitiesChanged 
              signal)
17:28 < andrunko> but we would not have an extra object for each contact with 
                  no caps
17:28 < smcv> andrunko: I meant they'd all have a pointer to a shared singleton 
              sort of thing
17:28 < smcv> andrunko: could you post a code snippet to the bug so I can 
              understand how you intend UI code to look?
17:29 < andrunko> ahh don't know, let me post a code snippet
17:30 < smcv> andrunko: you may assume that Core is ready on the Connection if 
              you want, but I think code like this should be non-ugly if 
              optional features might still be in-progress

Example code snippet

void UIContactManager::getContactsCaps()
{
    ContactManager *cm = m_connection->contactManager();
    if (cm->supportedFeatures().contains(Contact::FeatureCapabilities)) {
        PendingContacts *pc = cm->upgradeContacts(cm->allKnownContacts(), Contact::FeatureCapabilities);
        connect(pc,
                SIGNAL(finished(...)),
                SLOT(onContactUpgraded(...)));
    } else {
        for (uiContact, uiContacts) {
            uiContact->useConnectionCaps();
        }
    }
}

void UIContactManager::onContactUpgraded(PendingOperation *op)
{
    PendingContacts *pc = cast<>(op);
    QList<ContactPtr> contacts = op->contacts();

    for (contact, contacts) {
        uiTpContact = findUITpContactForContact(contact);
        // wathever
        uiTpContact->checkCapabilities();
    }
}

void UITpContact::UITpContact(ContactPtr contact)
    : internalContact(contact)
{
    // we can always connect here, even if Capabilities is not supported
    connect(contact,
            SIGNAL(capabilitiesChanged(...)),
            SLOT(checkCapabilities());
}

void UITpContact::checkCapabilities()
{
    if (!internalContact->capabilities()) {
        return;
    }

    // check caps and update UI
}

void UITpContact::useConnectionCaps()
{
    // connection FeatureCaps is already enabled
    // connection caps has not change notification, so just do it once

    // check caps and update UI
}

(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

Looks good now, I think. Merge merge merge! :-)

Fixed upstream, will be in next release 0.2