[Bug 46484] Add high-level Call bindings

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Andre Moreira Magalhaes <andrunko at gmail.com> 2012-03-14 22:01:19 PDT ---
(In reply to comment #3)
> (In reply to comment #1)
> > - missing pretty headers for CallContent and CallStream, and the newly added
> > public pending operations.
> > 
> 
> CallContent/CallStream headers are there. You added them ;)
> PendingCallContent was missing one, fixed now.
heh, I am getting crazy, it is fine now :P

> > - 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.
> 
> The content name is considered significant, but not required, so I made it an
> optional argument:
> 
>     PendingChannelRequest *ensureAudioCall(
>             const QString &contactIdentifier,
>             const QString &initialAudioContentName = QString(),
>             const QDateTime &userActionTime = QDateTime::currentDateTime(),
>             const QString &preferredHandler = QString(),
>             const ChannelRequestHints &hints = ChannelRequestHints());
> 
> Do you think we really need extra overloads without this argument?
No, that is ok.

> 
> > +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 purposefully didn't add a mediCall() method there, as according to the spec a
> Call1 channel that has neither InitialAudio nor InitialVideo is not valid. I
> think the public API should only have things that users are safe to use.
> 
> Quoting the spec:
> "The RequestableChannelClasses for Call1 channels can be:
> [( Fixed = { ...ChannelType: ...Call1,
>             ...TargetHandleType: Contact,
>             ...InitialVideo: True
>           },
>   Allowed = [ ...InitialVideoName,
>               ...InitialAudio,
>               ...InitialAudioName
>             ]
> ),
> ( Fixed = { ...ChannelType: ...Call1,
>             ...TargetHandleType: Contact,
>             ...InitialAudio: True
>           },
>   Allowed = [ ...InitialAudioName,
>               ...InitialVideo,
>               ...InitialVideoName
>             ]
> )]
> Clients aren't allowed to make outgoing calls that have neither initial audio
> nor initial video"
Ok then, makes sense.

> 
> Btw, now that I look at it again, maybe I should do s/MediaCall/Call/g in the
> relevant ChannelFactory and ChannelClassSpec methods? We have things like:
> 
> bool hasMediaCallInitialAudioFlag() const
> bool hasStreamedMediaInitialAudioFlag() const
> 
> which is a bit confusing... If it reflected the channel name in the spec, maybe
> it would be more obvious that the first method is for Call and the other is for
> SM.
I agree, I think we can remove "Media" from the names there.

Please add Farstream support and comment here and I will do a final review. The
branch is looking really good.

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