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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Olli Salli <ollisal at gmail.com> 2011-04-26 07:29:11 PDT ---
(In reply to comment #5)
> Branch updated with the following changes:
> - ChannelClassSpec/List can now be used in QHash/QSet/QPair/<wherever a qHash
> is required>
>   As a bonus point there is a test to check whether the hash actually works.

Hey, that was some seriously clever work over there! However one thing isn't
clear to me:

+    // Convert back to list and we have an ordered unique list
+    QList<ChannelClassSpec> uniqueOrderedList = uniqueSet.toList();

As discussed in IRC, this doesn't guarantee that you'd get the same set of
channel classes in the same order, so the hash needs to be made
order-independent.

> - Observers are now shared by QPair<dbus, ChannelClassSpec>, the same observer
> will be used for multiple accounts if on the same bus and using the same
> filter.

Good, but this works incorrectly in at least one way still:

+        observer = SharedPtr<Observer>(new Observer(cr, fakeAccountFactory,
+                    channelFilter, extraChannelFeatures));

>From this, the actual extra channel features will be the same for everybody
sharing an SO::P::Observer as the ones passed to the first created
SimpleObserver. This is why I said in my earlier comment that *you have to
register the SimpleObservers*, not just the accounts. As you went the register
accounts route though, the same can be achieved, by adding a
registerExtraFeatures which unites  the current extra features on the shared
observer with the registered extra features. Then you can remove the extra
feature constructor param.

Of course, in the current usage by STO and SCO, the extra features for a given
filter will always be the same anyway so this problem isn't apparent, but as
SimpleObserver is intended as a reusable implementation helper this'd mean
further SimpleObservers created with the same filter but different extra
features wouldn't work as expected.

The sharing (note: this is very much rather sharing than "caching") is also a
bit inefficient/messy in a few other ways:

+    static QHash<QPair<QString, ChannelClassSpecList>, QWeakPointer<Observer>
> observers;

You could actually use QPair<QString, QSet<ChannelClassSpec> > and also use the
normalized set of channel classes for registering the observer (convert back to
list for passing to AbstractClientObserver ctor). It's not beneficial to have
duplicates in the Client filter, and you wouldn't need to renormalize the list
on every value() on the observers hash (building a the same set out of a given
list in the key) every time.

+        foreach (const AccountPtr &account, mAccounts) {
+            if (account->objectPath() == objectPath) {
+                return account;
+            }
+        }

Use QHash from object path to account ptr to avoid linear search and duplicate
registered accounts (which only slows down the linear search even more,
especially since you never unregister them)?

> - ChannelFeatureSpec -> ChannelClassFeatures

You forgot to change the installed header names in CMakeLists.txt. Also, as
ChannelClassFeatures is just a typedef, maybe we only need the pretty header,
and could have the typedef in types.h like the other typedefs - and we could
ditch the channel-class-features.h header entirely?

> - No QDateTime anywhere anymore
> - Fixed typo in ChannelInvalidationInfo

Good!

> - AccountFactoty/ChannelFactory/FixedFeatureFactory can now be qobject_cast'ed

Good catch there.

> - Added warnings in case a channel of invalid type is received in SCO and STO
> 
> Missing tests for SCO (STO is tested in contact-messenger test), will still
> write it but most of its code is already tested by the contact-messenger test
> which tests SimpleObserver indirectly.

Yes, please test it separately. It'd actually even have been worth to test STO
too independently from ContactMessenger. Perhaps you should test the remaining
codepaths (not used by ContactMessenger ie. not using contact filtering, or
passing the contact in different ways) to make sure your refactored internals
still work with those parts? lcov-check is your friend here.

> 
> Hope you like it.

I'm definitely starting to like it a lot! However, some issues remain:

    ContextInfo *info = mObserveChannelsInfo.value(op);

    emit newChannels(info->account, info->channels);

    foreach (const ChannelPtr &channel, info->channels) {

Any particular reason for emitting newChannels before the channels are actually
committed to the set returned by the channels() accessor? Seems inconsistent to
me.

QList<ChannelPtr> SimpleObserver::channels() const
{
    return mPriv->observer ? mPriv->observer->channels() : QList<ChannelPtr>();
}

Wouldn't this always return all channels, even the channels which were filtered
out by the contact filter?

Moreover, now that you're sharing the observers, wouldn't this, and
consequently SCO::streamedMediaCalls() and STO::textChats() now return all
channels on the shared observer (even e.g. from different accounts, or for
different contacts if contact matching is used)?

Please test the sharing by using two different accounts concurrently, and
perhaps have a few SCO / STO instances for one of the accounts, with two
different contact filters and with no contact filter, and verify that the
accessors return the correct channels for each instance with no "crosstalk".

You could implement STO::textChats() easily in a way which wouldn't have this
problem, and would be more efficient: return the keys from mPriv->channels.
Those are already checked to be of the correct channel class etc. For SCO you
can't do that however (don't introduce a separate channels set in it just for
that, solve the crosstalk problem at the SimpleObserver level please so that
other simple observers implemented using SimpleObserver don't have to work
around it).

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