[Bug 23284] Add support for ContactCapabilities interface

[bugzilla-daemon at freedesktop.org]

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


Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|patch                       |




--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-10-06 08:14:48 PST ---
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 :-)


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