attempts to fix an infinite recursion in the messagelib, see bug 387903
Details
Details
- Reviewers
dvratil
running for a few days and haven't see the infinite recursion in the "standard mailing list" aggregation scheme.
I can't test ever possible aggregation scheme though. so I'm hoping other people will also test.
Diff Detail
Diff Detail
- Repository
- R94 PIM: Message Library
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Hmm, sounds like the real problem is that we have a MessageItem which has itself as its own parent, but fixing that might be more complex quite hard.
Maybe a better modification would be to change the !parent() condition above (not visible in this patch)
// TODO: investigate why Items may have self-referential parent
if (!parent() || parent() == this) {
return this;
}Comment Actions
Dan,
I tried your patch and hit the infinite recursion within seconds in my inbox folder.
Going back to my patch.
Comment Actions
How can a check *after* the recursive call resolve an infinite recursion? To prevent those, we have to treat base cases *before* the recursive call. Otoh, it seemed to help in your case. I'm quite curious how that's possible...
Also note that cycles with length >1 (A is parent of B, B is parent of C, ... Z is parent of A) might cause the infinite recursion. This would explain why Dan's approach did not work, because no item is its own parent.
Comment Actions
I just wanted to have a quick look at the message threading code to see if it might possibly introduce cycles. It is roughly from model.cpp:2067 to model.cpp:3361, 1300 lines of untested, untestable, deeply nested code with 13 FIXME comments in it, right?
Comment Actions
Now that I reread my comments, they sound more aggressive than I intended them to.
I am currently working on a patch for eventviews' IncidenceTreeModel that extracts the tree-ifying code from this already big class into its own TreeNodeManager (working title). I was wondering if the message item code could profit from such a class, in which case I would try to make it powerful enough to manager Akonadi::Items instead of only VTODOs. Before I try, I have a question though: Is it safe to store Akonadi::Items persistently, and impose a tree datastructure on a collection of such items? The question is inspired by the distinction between QModelIndex and QPersistentModelIndex. To turn my negativity above into something constructive: My TreeNodeManager ("ItemTree"?) already contains some tests! Finally, is there a good repository for stuff that aids both messagelib and eventviews?
Comment Actions
@dkurz please open a new phabricator task for your effort, we can discuss your questions there - and generally, it's a good idea to do that before you start something bigger, so we know what's going on and maybe discuss some details before you start working on it. The main reason is to avoid situations where you write a big chunk of code but it does not get merged because it does not do what we think it should or is missing some important bits. Thanks!
@winterz, I now realize another odditty with your code:
if (this != tItem) {
return tItem;
} else {
return this;
}The else branch implies, that this == tItem, in which case your code could be rewritten as
if (this != tItem) {
return tItem;
} else {
return tItem;
}Which basically means that your change does not do anything, Or am I missing something here?