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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #7 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-04-26 11:57:50 PDT ---
(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.

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