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

[Christian Mollekopf]

On Tue, 03 May 2011 21:07:11 +0200, Stephen Kelly <steveire at gmail.com>  
wrote:

> 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.
>
Heres the backtrace when asserting in isCollectionMonitored():


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

True, there might be some UI which needs to be noticed of this change,
so better emit it for completeness sake.
>
>>
>> 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/


-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/
_______________________________________________
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/