[Kde-pim] Re: Review Request: Emit changed signals for subentities of an explicitly monitored collection

[Stephen Kelly]

Christian Mollekopf wrote:

> On Tue, 03 May 2011 11:57:25 +0200, Stephen Kelly <steveire at gmail.com>
> wrote:
> 
>> Stephen Kelly wrote:
>>
>>> Christian Mollekopf wrote:
>>>
>>>> On Monday, April 25, 2011 10:59:57 AM Stephen Kelly wrote:
>>>>> Stephen Kelly wrote:
>>>>> > It will need unit tests though, and I won't get a chance to write
>>>>> them
>>>>> > before Monday at the earliest.
>>>>> >
>>>>> > - Stephen
>>>>>
>>>>> Please test the attached patch which includes the unit tests. I'll
>>>>> commit it after you test & review.
>>>>
>>>> I don't know why, but with your patch my akonadi goes berserk.
>>>> I get lots of "RID mismatch" messages and my imap account started to
>>>> generate invalid email items in an endless loop.
>>>>
>>>> I attached my console output. Tell me if you need something else for
>>>> debugging.
>>>
>>> I might have reproduced this. I got a RID mismatch message in the
>>> maildir
>>> resource, which is code I don't know unfortunately.
>>>
>>> I'll look into it a bit though.
>>>
>>
>> New patch attached. I think the ChangeRecorder stuff might still be
>> broken.
>> Will have to review that with Volker later.
>>
> 
> The only thing I noticed is that I get "Invalid Collection in prefetch"
> messages from isCollectionMonitored() when I delete a collection with
> items in it (i.e. a subcollection of the monitored collection).
> I don't know which exact call it is, but I think it is to be expected as
> the parentCollection is already invalidated (according to one of your
> comments).

Interesting. I added the code (with comments) to work around that warning. 
I'll have another look.

> 
>  From a behaviour POV it works perfectly together with the ETM patches.

Good to hear. I'm looking into those next.

> 
> Regarding the TODO: q->setCollectionMonitored( Collection( msg.uid() ),
> false );
> I think you can just silently remove it from collections, as the
> collectionMonitored( collection, monitored ); signal is not needed.
> It is also not useful to keep it in collections.
>
> Maybe add a call to cleanOldNotifications to be sure that there are not
> more waiting (not sure if this is possible).

I agree it's not useful to keep it in d->collections and to call 
cleanNotifications, but I'm not certain about not emitting the signal. The 
signal is probably harmless though even though I can't think of a use case 
for it right now. It seems wrong to remove it from collections without 
signalling that.


> 
> I had a look at the monitor part of the patch, and it looks all ok IMO.
> 
> So from my side you can commit, the minor issues can be checked later.

Great, thanks for testing and reviewing.

Seeing as I'll be afk for a few days from tomorrow, I think I'll wait until 
I'm back before pushing the commit. I need to strong-arm volker into 
reviewing too because the current implementation of ChangeRecorder doesn't 
make sense to me.

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/