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

Tue May 3 22:13:33 BST 2011

On Tue, 03 May 2011 23:11:14 +0200, Christian Mollekopf  
<chrigi_1 at fastmail.fm> wrote:

> 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():

#11 0x00007fcc37c2e308 in Akonadi::MonitorPrivate::isCollectionMonitored  
(this=0x1730390, collection=109, recurs=true) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/monitor_p.h:230
#12 0x00007fcc37c2db83 in  
Akonadi::MonitorPrivate::notifyCollectionStatisticsWatchers  
(this=0x1730390, collection=109, resource=...) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/monitor_p.cpp:771
#13 0x00007fcc37c2ad7e in Akonadi::MonitorPrivate::updatePendingStatistics  
(this=0x1730390, msg=...) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/monitor_p.cpp:324
#14 0x00007fcc37c2b86d in Akonadi::MonitorPrivate::filterMessages  
(this=0x1730390) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/monitor_p.cpp:442
#15 0x00007fcc37c2bb47 in Akonadi::MonitorPrivate::dispatchNotifications  
(this=0x1730390) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/monitor_p.cpp:481
#16 0x00007fcc37c2b786 in Akonadi::MonitorPrivate::slotNotify  
(this=0x1730390, msgs=...) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/monitor_p.cpp:432
#17 0x00007fcc37b8c8f6 in Akonadi::ChangeRecorderPrivate::slotNotify  
(this=0x1730390, msgs=...) at  
/home/chrigi/devel/kde/kdepimlibs/akonadi/changerecorder_p.h:51

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