[Kde-pim] Re: Review Request: Include collections wich are watched in the ChangeRecorder in the EntityTreeModel

[Christian Mollekopf]

Hi Steve,

On Thursday 27 January 2011 12:16:01 Stephen Kelly wrote:
> Hi,
> 
> Reviewboard doesn't allow counter-proposal patches, so this is just an
> email reply to the list
> 
> Christian Mollekopf wrote:
> > The patch is however work in progress, but it is almost fully working in
> > the cases i tested. So all I need to know is, if I'm going in the right
> > direction.
> 
> You've got a lot of unrelated changes in the patch now - splitting a
> move into insert/remove, emitting notifications for descendants of
> monitored collections, and adding multiple monitored collection
> support to ETM.
> 
> Let's split it up.

Hmm, yes, it got a bit messy =)

> 
> First one I'd like to deal with is splitting the move into insert and
> remove. I attach patches to akonadi and kdepimlibs to make that
> happen. I use the same hack as is used to report item moves, but I
> don't know why the hack is there. Surely there should be a QByteArray
> NotificationMessage::destResource() const ? Maybe Volker can tell us
> something about that one.
> 

That looks generally good. However, with the implicitly included  collections 
that wouldn't work. 
I can see two options:

1. We keep a tree of all implicitly monitored collections always up to date.

With this scenario we can just extend isCollectionMonitored, to also return 
true on implicitly monitored collections (see my review request).

2. We fetch all ancestors of the collection/item and check if there is an 
explicitly monitored collection.

In this case we would have to fetch all ancestors in ensureDataAvailable and 
then extend isCollectionMonitored, to go trough all ancestors.
Of course this means that isCollectionMonitored can only be used after 
ensureDataAvailable returns true, so the checks you're doing right now in 
appendAndCompress would have to be deferred until the ensureDataAvailable is 
through (i.e. in emitNotification).

btw. since we would anyway have to fetch all ancestors this way, we could as 
well fetch the destinationParentCollection, and check the resource of it. 
Although the performance of your hack is probably better.


> As for the notifications in the case of subcollections of monitored
> collections, I think that should happen in ensureDataAvailable, not in
> the ETM.

Agreed, I also wasn't suggesting that it should happen in the ETM. 
In my patch the finding of implicitly included items happens in the monitor.

The only thing where the handling was different to how resources and mimetypes 
are included, was that I exposed all implicitly monitored collections over 
collectionsMonitored().

I reconsidered in the meantime, and added a function which checks all 
criterias (monitored collections, resources, mimetypes) to the monitor.
So from an ETM point of view, the handling is the same if the resource was 
included or the collection (with includeImplicitlyMonitored = true). 
The ETM can just check isCollectionIncluded, and in case it is, recursivly 
fetch all collections/items (excluding by mimetype of course).

The only thing which needs special handling is, if a collection is included, 
without recursivly including all subcollection, where we can't use a recursive 
fetch, but only a CollectionFetchJob::Base fetch.

Cheers,

Chris
> 
> All the best,
> 
> Steve.
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/