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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Olli Salli <ollisal at gmail.com> 2011-04-21 08:59:58 PDT ---
+typedef QPair<ChannelClassSpec, Features> ChannelFeatureSpec;

"Specification for a Channel Feature". Not really. Remember, the word Spec is
not some meaningless suffix we want to tackle to all new misc types.
ChannelClassFeatures? Meaning, "the Features for a Channel Class"?

+    debug() << "Registering observer for account" << account->objectPath();
+    observer = SharedPtr<Observer>(observers.value(account));
+    if (!observer) {
+        /* ... */
+        observer = SharedPtr<Observer>(new Observer(cr,
+                    channelFilter, account, extraChannelFeatures));

How about when there is already e.g. a Text observer (so it has a filter with a
ChannelType = ChannelTypeText class) for a particular account, and a Call
observer is attempted to be created for that same account? Wouldn't the Call
observer end up trying to catch the calls with the text-only Observer?

** OH, I see that you went for not sharing the observers at all a few commits
later. I don't think this is the way to go, as MC has to invoke and wait for
all observers to finish doing their thing before it can proceed with handling
channels. So if we register an observer for each different set of channel
features enabled for each set of channels matched by a filter for each contact
on every account, we end up with gazillions of observers. That would add a lot
of latency to all MC requesting and handling of channels of those classes *on
the entire system* (say, to the from receiving an incoming message to
displaying it in the Handler). So, please consider my suggestions below to keep
the number of registered Observers lower. **

As a registered Observer's filter can't be changed, I think the only solution
is to change the key to include the filter as well. The filter is reducible to
a list of variant maps. The individual variant maps need to be compared with
strict equality, while a list (filter) of the same maps (channel classes) in
whichever order is the same, so you need to sort the list and eliminate
duplicate maps (channel classes) from it somehow, effectively normalizing it
for keying.

The same problem with incorrect sharing exists for the "extra channel
features". For them, however, a better alternative would be to store them in
the individual SO::Private classes instead of the shared SO::Private::Observer
instances. This requires registering the SO::Privates with the
SO::Priv::Observers and the Observer calling a method returning a PendingReady
on each Private registered to it, and only returning with the observeChannels
MethodInvocationContext when a PendingComposite formed from them has finished.
This doesn't add a whole lot of complexity (adding the feature list to the
observer key wouldn't be that easy either) and allows us to get away with fewer
observers, as all observers for a particular account with a given filter can be
shared no matter any differing extra features.

In fact, even better would be to key the observers with just (bus connection
name, filter). The observers aren't really account-specific, that's just a
limitation of their fake account factory implementation. If you register the
SO::Privates with shared SO::Priv::Observers, the fake account factory they use
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".

Please don't dismiss sharing the Observers between multiple Accounts as
premature optimization: once you have implemented the Private registration for
correct extra features handling, it's trivial to extend that to sharing between
the accounts and reap the full benefits. It's desirable to keep the user-facing
API of the SimpleTextObserver and SimpleCallObserver classes Account-specific,
though, as channel requests are also done on a single Account - thus we don't
mix the hierarchy layers in the API.

+    QDateTime now = QDateTime::currentDateTime();

and

+    void newChannels(const QList<Tp::ChannelPtr> &channels, const QDateTime
&timestamp);
+    void channelInvalidated(const Tp::ChannelPtr &channel, const QString
&errorName,
+            const QString &errorMessage, const QDateTime &timestamp);

The timestamp for handling channels is the *user action time* and is received
from MC, having originally been given to it by the requesting application. It
is used for judging whether to focus/popup UI elements in *handlers*. This use
is not valid for observers (as there can be many of them, all of them couldn't
focus/popup their stuff anyway), hence MC doesn't give you any timestamps.

Do you have some *Observer* usecase in mind for a timestamp generated locally
for the time when MC invoked you, or even moreso, when a Channel invalidation
was processed? If not, removing the timestamp machinery would take out some
needless complexity and clean up the API.

+    struct ChannelInvadationInfo;

typo. Good thinking with the queue, though.

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