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

[Dan Vrátil]

> On July 24, 2014, 5:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 225
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line225>
> >
> >     Since the childMap is only used few lines below just to check whether the collection has any children, it could just be a QSet<qint64> instead of a QHash.
> >     
> >     Actually if you build the map before the
> 
> Christian Mollekopf wrote:
>     a part of this sentence got lost ;-)

I think the sentence got finished in the second comment above :-)


> On July 24, 2014, 5: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.

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.


- Dan


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


On July 23, 2014, 2:01 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119418/
> -----------------------------------------------------------
> 
> (Updated July 23, 2014, 2:01 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/tests/unittest/listhandlertest.cpp b25b6a85538cec786c09a2f2cc629b2be5c82fec 
>   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 
> 
> 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/