[Kde-pim] Review Request 109259: Remove inode/directory from MailCommon::FolderCollectionMonitor

[Wolfgang Rohdewald]

> On March 3, 2013, 6:14 p.m., Wolfgang Rohdewald wrote:
> > > fix a problem, that removing a completely unrelated resource (like Google Calendar
> > > was totally messing up KMail's folder view (usually leading to crash).
> > 
> > How does it fix that? By not going thru the code path which makes kmail crash? But then the crashing bug is still there, it is just not triggered. Is it safe to assume that this dangerous code path is never taken elsewhere? To me, a bug messing up the folder view and crashing seems to be worthy of its own investigation, but you are removing one (although maybe the only one) way how to trigger it.
> 
> Dan Vrátil wrote:
>     I believe the crash is caused by one of the proxy models which are on top of the EntityTreeModel. When collections owned by the removed resource are removed from to model, the proxy model does not handle that, because it does not expect collections with non-KMIME content to be in the model.
> 
> Dan Vrátil wrote:
>     *...are removed from the root model...
> 
> Wolfgang Rohdewald wrote:
>     so could the proxy model be changed to always ignore items with non-KMIME content? (I say items and not collections because I do not know if it is allowed to mix different types of content within the same collection)
> 
> Dan Vrátil wrote:
>     I don't see any reason why a KMail-specific proxy model should handle collections that don't contain emails...
> 
> Volker Krause wrote:
>     What's actually causing the crash still needs to be analyzed I think, but even if this just hides a bug in a proxy model down the line, the fact that KMail was loading many more collections than actually relevant for it is wrong by itself already. So, this patch is correct IMHO.

I fully agree - I never meant to say anything against this patch by itself. Only the fact that this makes it improbable the crash reason will ever be analysed worries me a bit. For me crashes have a much higher priority than optimizations. Is there a bug report about that crash? Could it be augmented with a hint that this patch hides it?


- Wolfgang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109259/#review28487
-----------------------------------------------------------


On March 3, 2013, 2:59 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109259/
> -----------------------------------------------------------
> 
> (Updated March 3, 2013, 2:59 p.m.)
> 
> 
> Review request for KDEPIM and Laurent Montel.
> 
> 
> Description
> -------
> 
> Don't watch collections in KMail that KMail is not really interested in. This should improve performance a bit and fix a problem, that removing a completely unrelated resource (like Google Calendar) was totally messing up KMail's folder view (usually leading to crash).
> 
> I assume this was added as a workaround for a bug in Monitor, that collections trees where the root collection did not match the mimetype filter were ignored. This has been fixed some time ago, so this is not needed anymore.
> 
> 
> Diffs
> -----
> 
>   mailcommon/foldercollectionmonitor.cpp 49de537 
> 
> Diff: http://git.reviewboard.kde.org/r/109259/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

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