[Kde-pim] Re: Review Request: Emit changed signals for subentities of an explicitly monitored collection
Stephen Kelly
steveire at gmail.com
Tue May 3 20:07:11 BST 2011
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/
More information about the kde-pim
mailing list