[Kde-pim] Review Request 119418: LIST: Non-Recursive listing

[Christian Mollekopf]

> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 287
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line287>
> >
> >     This has to be col.id() - otherwise collections get sometimes listed multiple times in the output, which leads to the assert in CollectionSync that I was getting. Apparently it's only reproducible on more complicated hierarchies, and also dependso n the order in which you get collections from the database.
> 
> Christian Mollekopf wrote:
>     Could you perhaps come up with a test or supply me with a tree that exposes the bug? Otherwise I'll try to figure it out.
> 
> Dan Vrátil wrote:
>     I think any hierarchy with folder depth > 3 will do.
>     
>     I'm too lazy to write a test, but I can explain: the loop will insert each collection to knownIds and then it will check, whether it's parent is already in knownIds. If not, it will add the parent into missingCollections (that can happen when the parent collection is not in the collections list, or when descendants are checked before their ancestors).
>     
>     In next iteration, the code will insert the next collection into knownIds and will remove the collection from missingCollections if it's there, which literally means "some collection was missing a parent, but now I found it, so it's no longer missing". The bug is obvious, since the code was trying to remove the missings parent's parent instead of just the parent.

Thanks for the explanation. I'm using now two loops instead which avoids the add/remove making the code clearer. And looping once more over the list isn't a big overhead IMO (not sure which version is better performance-wise, but I think they should be pretty much the same).


> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 282
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line282>
> >
> >     You already have knownIds calculated from the mimetype filter above (at the "replace QHash by QSet" comment), so there's no need to create one here again - you could reuse it.

I can't because some collectoins may have been removed meanwhile, and I don't think it's worth complicating this further by trying to keep this cache up-to-date as well.


- Christian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119418/#review63078
-----------------------------------------------------------


On July 24, 2014, 6:55 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119418/
> -----------------------------------------------------------
> 
> (Updated July 24, 2014, 6:55 p.m.)
> 
> 
> Review request for Akonadi.
> 
> 
> Repository: akonadi
> 
> 
> Description
> -------
> 
> CollectonReferenceTest: Fixed the tests after fixing the reference manager
> 
> This test couldn't work because the referenced state is reset after
> each scenario. I adapated the test now accordingly.
> 
> LIST: Non-recursive listing
> 
> Instead of recursively query for children,
> we query for a list of all collections we're interested in,
> and then assemble a tree.
> 
> This potentially results in a small overhead with few collections,
> but should scale much better when we have many collections while most
> are getting filtered by the initial query (i.e. disabled collections).
> 
> Additionally this patch ensures enabled collections that are nested in
> a disabled collection are correctly found.
> 
> Share DbInitializer for other tests and make use in listhandlertest.
> 
> use dbinitializer
> 
> remove the collections manually since sqlite doesn't deal with constraints.
> 
> 
> Diffs
> -----
> 
>   server/src/handler/list.h 56401b3164e5035518d63ed39e5a048481808560 
>   server/src/handler/list.cpp b891d10d2ceb63482a4453695dc38ee625b8c768 
>   server/tests/unittest/CMakeLists.txt b9744d93a3b0cb9e895141c10ddaf2703f12acf0 
>   server/tests/unittest/collectionreferencetest.cpp 808227f9771a33dc1c77d960160770ca919ea2fd 
>   server/tests/unittest/dbinitializer.h PRE-CREATION 
>   server/tests/unittest/dbinitializer.cpp PRE-CREATION 
>   server/tests/unittest/listhandlertest.cpp b25b6a85538cec786c09a2f2cc629b2be5c82fec 
> 
> Diff: https://git.reviewboard.kde.org/r/119418/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

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