Bug 33525

Andre has a WIP branch. I'm going the review the current state now.

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

(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

Here's an idea: add const QString &error, const QString &errorMessage as the call ended signal's last args. They can be ignored if not needed, but they allow more precise (error) reporting/handling. I'm going to do the same with the Tube handler APIs.

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.
- 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.
- ChannelFeatureSpec -> ChannelClassFeatures
- No QDateTime anywhere anymore
- Fixed typo in ChannelInvalidationInfo
- AccountFactoty/ChannelFactory/FixedFeatureFactory can now be qobject_cast'ed
- 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.

Hope you like it.

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

(In reply to comment #6)
> (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.
Fixed as we discussed on IRC.

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

> 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.
Done. The SO::channelFilter() accessor still returns the same as passed on construction though.
 
> +        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)?
Done.

> > - 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?
Oh, indeed I forgot to update CMakeLists.txt. Done now. As for moving to types.h, it can't be done as it would require types.h to include ChannelClassSpec and Features and they already include types.h...

> > - 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.
Working on them now.
 
> > 
> > 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.
Just missed that, fixed.
 
> 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).
Fixed all crosstalk, at least I think I did, tests will tell.

Branch updated, missing tests which I am working now.

Adding myself to CC list.

Starting to look even better, though we definitely still need the tests. Some things though:

+    foreach (const ChannelPtr &channel, newChannels) {
+        if (filterChannel(channelsAccount, channel)) {
+            match.insert(channel);
+        }
+    }
+
+    if (match.isEmpty() || channels.contains(match)) {
+        return;
+    }
+

Do if (!channels.contains(channel) && filterChannel(... in the loop already and only check for match being empty afterwards to not emit redundant additions in the signal? (When you get newChannels in which some of them are new and some existed already). That'd be also slightly faster than checking the channels and match sets for being a subset (set.contains(set)).

+void SimpleObserver::Private::removeChannel(const AccountPtr &channelAccount,
+        const ChannelPtr &channel,

Similarly, skip the filtering and possible emitting if channels doesn't contain the channel anymore in the first place (i.e. redundant channel invalidation). This wouldn't likely happen in practice, though, unlike the first one, but would be good for consistency / clarity.

> Properly install ChannelCLassFeatures instead of ChannelFeatureSpec.

As it wasn't possible to move the typedef to types.h, you need to install channel-class-features.h too.

Branch updated with the following changes:
- Do not leak internal observers
- Exported SimpleObserver class :P
- Avoid emitting redundant signals
- Tests

+        ret << ChannelPtr::qObjectCast(channel);

Tp::SharedPtr supports upcasting, shouldn't ret << channel just work too?

+        memset(mConns, 0, sizeof(mConns) / sizeof(ConnInfo));
+        memset(mMessagesChanServices, 0, sizeof(mMessagesChanServices) / sizeof(ExampleEcho2Channel*));
+        memset(mCallableChanServices, 0, sizeof(mCallableChanServices) / sizeof(ExampleCallableMediaChannel*));

I'd rather make ConnInfo have a ctor which initializes the members to NULL, and use std::fill(mMessagesChanServices, mMessagesChanServices + 2, NULL) etc to be cleaner C++ code.

+            SimpleTextObserverPtr textObserver = SimpleTextObserver::create(acc, contact);
+            QCOMPARE(textObserver->account(), acc);
+            QCOMPARE(textObserver->contactIdentifier(), contact);
+            QVERIFY(textObserver->textChats().isEmpty());
+            textObservers.append(textObserver);
+            QCOMPARE(ourObservers().size(), numRegisteredObservers);
+
+            textObserver = SimpleTextObserver::create(acc, contact);
+            QCOMPARE(textObserver->account(), acc);
+            QCOMPARE(textObserver->contactIdentifier(), contact);
+            QVERIFY(textObserver->textChats().isEmpty());
+            textObservers.append(textObserver);
+            QCOMPARE(ourObservers().size(), numRegisteredObservers);

What's your intention with creating two text observers in a row?

In case your intention is to check that two STOs can share an observer, then you have a bug: the creation of the second STO frees the first one, because you assign to the same STOPtr, so they co-exist only for the duration of the assignment operation.

OTOH, if your intention is to just test that the STO picks up the observer registered for the SO directly created earlier, why have two STOs at all?

In general, please add oneliner comments every now and then to describe the intention of the tests when you have a lot of not very explicit things going on. The crosstalk test could really use a few "we should have this, but not that, because the filter blahblah" type of comments between the big blocks of checks too.

In addition to helping the reviewer see just what is being tested (or should be, enabling cross referencing with what's actually being checked), it also helps you to think about whether you're testing the right things or not :)

+    while (observers[0]->channels().size() != 1 ||
+           observers[1]->channels().size() != 1 ||
+           textObservers[0]->textChats().size() != 1 ||
+           textObservers[1]->textChats().size() != 1 ||
+           textObserversNoContact[0]->textChats().size() != 1 ||
+           textObserversNoContact[1]->textChats().size() != 1 ||
+           callObservers[0]->streamedMediaCalls().size() != 1 ||
+           callObservers[1]->streamedMediaCalls().size() != 1 ||
+           callObserversNoContact[0]->streamedMediaCalls().size() != 1 ||
+           callObserversNoContact[1]->streamedMediaCalls().size() != 1) {
+        mLoop->processEvents();
+    }

If there IS, in fact, crosstalk, this'd hang forever, which is an irritating way to discover you've caused a regression (not making the root cause very clear). So maybe change this loop to wait for the channels()/chats()/calls() to be !isEmpty()? And after the loop, QCOMPARE their channel counts individually with 1. Then you know directly from the failed check which one either lost their channel prematurely or got extra ones due to crosstalk if you cause a regression.

+void TestSimpleObserver::cleanupTestCase()
+{
+    for (int i = 0; i < 2; ++i) {

You might want to check what I changed TestConnRosterGroups(Legacy)::cleanup() code to: it's both shorter/cleaner, and doesn't hang if one of the conns didn't connect, or started disconnecting already (but we didn't catch the invalidation yet). I hit both cases due to races in the roster code, which was again very irritating to debug as make check just froze forever...

Perhaps we should actually make the "here's a Tp::Connection and a TpBaseConnection, make them die" piece of cleanup code shared between the tests in the Test baseclass?

As the final touches are needed just in the tests, feel free to merge and release after handling them.

(In reply to comment #11)
> +        ret << ChannelPtr::qObjectCast(channel);
>
> Tp::SharedPtr supports upcasting, shouldn't ret << channel just work too?
Done.
 
> +        memset(mConns, 0, sizeof(mConns) / sizeof(ConnInfo));
> +        memset(mMessagesChanServices, 0, sizeof(mMessagesChanServices) /
> sizeof(ExampleEcho2Channel*));
> +        memset(mCallableChanServices, 0, sizeof(mCallableChanServices) /
> sizeof(ExampleCallableMediaChannel*));
> 
> I'd rather make ConnInfo have a ctor which initializes the members to NULL, and
> use std::fill(mMessagesChanServices, mMessagesChanServices + 2, NULL) etc to be
> cleaner C++ code.
Add the default ctor to ConnInfo, but didn't change the rest to use std::fill, first because I never used it before and second because std::fill(mMessagesChanServices, mMessagesChanServices + 2, NULL) won't build and I won't bother trying to figure it out why. I would rather use memset here as everybody knows how it works.

> +            SimpleTextObserverPtr textObserver =
> SimpleTextObserver::create(acc, contact);
> +            QCOMPARE(textObserver->account(), acc);
> +            QCOMPARE(textObserver->contactIdentifier(), contact);
> +            QVERIFY(textObserver->textChats().isEmpty());
> +            textObservers.append(textObserver);
> +            QCOMPARE(ourObservers().size(), numRegisteredObservers);
> +
> +            textObserver = SimpleTextObserver::create(acc, contact);
> +            QCOMPARE(textObserver->account(), acc);
> +            QCOMPARE(textObserver->contactIdentifier(), contact);
> +            QVERIFY(textObserver->textChats().isEmpty());
> +            textObservers.append(textObserver);
> +            QCOMPARE(ourObservers().size(), numRegisteredObservers);
> 
> What's your intention with creating two text observers in a row?
> 
> In case your intention is to check that two STOs can share an observer, then
> you have a bug: the creation of the second STO frees the first one, because you
> assign to the same STOPtr, so they co-exist only for the duration of the
> assignment operation.
> 
> OTOH, if your intention is to just test that the STO picks up the observer
> registered for the SO directly created earlier, why have two STOs at all?
As I said on IRC, you missed the line that calls observers.append(observer), so the first one won't get deleted when the second one is created. And yes the idea is to test if the two share the same observer, by checking the number of observers returned by ourObservers() does not change between the two calls.

> In general, please add oneliner comments every now and then to describe the
> intention of the tests when you have a lot of not very explicit things going
> on. The crosstalk test could really use a few "we should have this, but not
> that, because the filter blahblah" type of comments between the big blocks of
> checks too.
> 
> In addition to helping the reviewer see just what is being tested (or should
> be, enabling cross referencing with what's actually being checked), it also
> helps you to think about whether you're testing the right things or not :)
Still missing, but I will add some code comments there to make it easier to follow it.

> +    while (observers[0]->channels().size() != 1 ||
> +           observers[1]->channels().size() != 1 ||
> +           textObservers[0]->textChats().size() != 1 ||
> +           textObservers[1]->textChats().size() != 1 ||
> +           textObserversNoContact[0]->textChats().size() != 1 ||
> +           textObserversNoContact[1]->textChats().size() != 1 ||
> +           callObservers[0]->streamedMediaCalls().size() != 1 ||
> +           callObservers[1]->streamedMediaCalls().size() != 1 ||
> +           callObserversNoContact[0]->streamedMediaCalls().size() != 1 ||
> +           callObserversNoContact[1]->streamedMediaCalls().size() != 1) {
> +        mLoop->processEvents();
> +    }
> 
> If there IS, in fact, crosstalk, this'd hang forever, which is an irritating
> way to discover you've caused a regression (not making the root cause very
> clear). So maybe change this loop to wait for the channels()/chats()/calls() to
> be !isEmpty()? And after the loop, QCOMPARE their channel counts individually
> with 1. Then you know directly from the failed check which one either lost
> their channel prematurely or got extra ones due to crosstalk if you cause a
> regression.
Done.

> +void TestSimpleObserver::cleanupTestCase()
> +{
> +    for (int i = 0; i < 2; ++i) {
> 
> You might want to check what I changed TestConnRosterGroups(Legacy)::cleanup()
> code to: it's both shorter/cleaner, and doesn't hang if one of the conns didn't
> connect, or started disconnecting already (but we didn't catch the invalidation
> yet). I hit both cases due to races in the roster code, which was again very
> irritating to debug as make check just froze forever...
Done.

Pushed -f the branch, will add some comments and merge/release. Thanks for the review.

Released in tp-qt4 0.5.16.