[Bug 33525] Helper class(es) for observing calls

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Olli Salli <ollisal at gmail.com> 2011-04-21 10:09:12 PDT ---
(In reply to comment #2)
> can retrieve the account to use from any of the registered SO::Privates
> (instead of just a single one which passed it to the ctor on creation). In this
> approach the check for "is it for my Account" in SO::P::O::observeChannels()
> becomes "is it for any of the registered Private's Accounts".
> 

It should be done this way specifically, and not for example by filtering in
the SO slot connected to SO::P::Observer::newChannels, as the latter approach
would have the observer needlessly doing all of the channel prepare work even
for channels not for any of the accounts it's observing.

+      AbstractClientObserver(channelFilter, false),

Actually (perhaps contrary to some of our earlier discussions on the subject?)
we need shouldRecover = true for SimpleTextObserver. This is so that messages
will be picked up on Text channels which used to exist when the STO was created
as well. We'd also like that to report any already-ongoing calls in SCO when it
was created. So maybe we want it to always be true? Can you think of any use
for the SimpleObserver where we would *not* want the existing channels?

+
+    static SimpleCallObserverPtr create(const AccountPtr &account,
+            CallDirection direction = CallDirectionBoth,
+            CallType type = CallTypeAll);
+    static SimpleCallObserverPtr create(const AccountPtr &account,
+            const ContactPtr &contact,
+            CallDirection direction = CallDirectionBoth,
+            CallType type = CallTypeAll);
+    static SimpleCallObserverPtr create(const AccountPtr &account,
+            const QString &contactIdentifier,
+            CallDirection direction = CallDirectionBoth,
+            CallType type = CallTypeAll);
+

Where are conference calls? Add a IncludeConferences conferences =
WithoutConferences param to each overload (separate enum)?

+        emit
streamedMediaCallStarted(StreamedMediaChannelPtr::qObjectCast(channel),
timestamp);

and

+    emit streamedMediaCallEnded(StreamedMediaChannelPtr::qObjectCast(channel),
+            timestamp);

Check the casts, warn and don't emit if they fail. Otherwise we end up crashing
unsuspecting users of the API with no explanation.

+    if (type & CallTypeAudio) {
+        channelFilter.setStreamedMediaInitialAudioFlag();
+    }
+    if (type & CallTypeVideo) {
+        channelFilter.setStreamedMediaInitialVideoFlag();
+    }

If you use CallTypeAll, you'll actually build a single filter which only
matches calls which have BOTH initial audio and initial video set, whereas the
intention would be to then include a channel class for each of audio-only,
video-only and audio-video calls.

One would also probably want to observe channels with no initial streams in the
properties at all (older CMs - though are they still relevant?). For that, we'd
need a class in the filter which doesn't include any of the initial stream
flags (though, that'd actually need to be filtered further client-side as it'd
match all calls in MC, even ones with initial streams!). Also, one could feel
misled by the fact that we promise watching video calls, but fail to catch
audio calls which were upgraded to a video call!

I guess we need to can the idea about filtering for specific types of calls, as
the API would need to be rather more complex otherwise, especially considering
call upgrades. That way we end up with less different filter combinations and
therefore less total observers as well, which is always good. I guess most uses
of this API only need to observe if there is any call at all, and can dig which
type of a call it is from the channel if they need more specific information.

The call direction filtering is

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