[Bug 46484] Add high-level Call bindings

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Andre Moreira Magalhaes <andrunko at gmail.com> 2012-03-12 09:27:22 PDT ---
(In reply to comment #0)
> As you know, I have been working on high-level bindings for the Call1 spec. I
> believe that that those bindings are now ready for a first review round.
Great, bellow you can find the first review round. All in all it looks really
good.

> This branch contains only high-level bindings for Call. I will do the farstream
> wrapper library in a separate branch.
It would be good to have the farstream bits in place before merging, so it
could be released in 0.9.1

> Some open issues/questions in this branch:
> * Should SMChannel and all the related methods be deprecated? Currently it's a
> bit confusing which API to use in classes like Account (ensureAudioCall or
> ensureStreamedMediaAudioCall? that is the question!)
I would not deprecate it for now, lets for it later in 0.9.2 or so as this is
the first version of Call and there may be others still using SM. Lets give
them a chance to update first.

> * Should Tp::CallStateReason be wrapped into some higher level class? It's a
> bit ugly as it is, although in most cases it's just a useless object. If it is
> wrapped in a high level class, though, how can a Contact be constructed in the
> place of the actor handle without making the emission of all those signals
> async?
IMO we can use CallStateReason as is, no need for a high-level class.

Now the review:

- missing pretty headers for CallContent and CallStream, and the newly added
public pending operations.

+++ account.cpp

+        if (!audioName.isEmpty()) {
+            request.insert(TP_QT_IFACE_CHANNEL_TYPE_CALL +
QLatin1String(".InitialVideoName"),
+                           audioName);
+        }
s/audioName/videoName/g

- I guess audio/video content name are not that important API wise and may be
ignored in some cases and be generated internally? So I believe we should have
helper methods overloads in Account that don't take the content name as
argument. For all cases.

- Please also update copyright in account.cpp to 2008-2012, we completely
forgot to update it, it is still 2008.

- Move PendingCallContent to call-content.h/cpp instead of call-channel.h/cpp.

+Features ChannelFactory::featuresForMediaCalls(const QVariantMap
&additionalProps) const
+{
+    return featuresFor(ChannelClassSpec::audioCall(additionalProps));
+}
+
+void ChannelFactory::addFeaturesForMediaCalls(const Features &features,
+        const QVariantMap &additionalProps)
+{
+    addFeaturesFor(ChannelClassSpec::audioCall(additionalProps), features);
+    addFeaturesFor(ChannelClassSpec::videoCall(additionalProps), features);
+}
+
+void ChannelFactory::setConstructorForMediaCalls(const ConstructorConstPtr
&ctor,
+        const QVariantMap &additionalProps)
+{
+    setConstructorFor(ChannelClassSpec::audioCall(additionalProps), ctor);
+    setConstructorFor(ChannelClassSpec::videoCall(additionalProps), ctor);
+}
+
I guess you want a ChannelClassSpec::mediaCall and use it here instead?

I will have to double check the branch later again before merging as I just
skin read it, but as most of the code is from tp-qt-yell which was already
reviewed, I guess we will be fine.

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