[Kde-pim] Review Request 118702: Support for referenced collections and ETM stuff

[Dan Vrátil]

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

Ship it!


The code itself looks OK to me, but I don't understand ETM enough to spot any mistakes in the logic. The tests seem solid, so if they pass, this can be shipped and we can solve any problems later in master :-)


akonadi/collection.h
<https://git.reviewboard.kde.org/r/118702/#comment41746>

    Whitespace



akonadi/entitytreemodel.h
<https://git.reviewboard.kde.org/r/118702/#comment41747>

    Whitespace



akonadi/tests/entitytreemodeltest.cpp
<https://git.reviewboard.kde.org/r/118702/#comment41929>

    Whitespace :-)


- Dan Vrátil


On June 12, 2014, 9:59 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118702/
> -----------------------------------------------------------
> 
> (Updated June 12, 2014, 9:59 p.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Repository: kdepimlibs
> 
> 
> Description
> -------
> 
> Diff for the dev/etmAndReferencedCollections branch.
> 
> This branch contains the client API for referenced collections as well as a slew of fixes for the ETM.
> 
> In particular this patch:
> * adds support to the ETM to monitor several collections (which is a requirement for the referencing of collections as well).
> * fixes the emission of dataChanged to be correct (I'm pretty sure that didn't make sense before and supspect I broke it myself in a previous commit).
> * removes the FirstList hack which AFAIK never worked and is IMO also not necessary
> * makes the code easier to understand
> 
> 
> EntityTreeModelTest: Cleanup.
> 
> The full test passes now.
> 
> ETM: Removed unused code
> 
> 
> ETM: The corresponding collection could have been removed while a fetch job is in progress.
> 
> 
> ETMPopulationTest: Fixed waiting for signals with isFullyPopulated.
> 
> 
> ETM: Provide isFullyPopulated to include population of collections and items.
> 
> This returns only true once all collections and items have been fetched.
> This is primarily important for tests to ensure no further signals are emitted
> by the initial population.
> 
> ETM: Split fetchJobDone + correctly emit dataChanged signals.
> 
> Split fetchJobDone into separate versions for Item/Collection fetch jobs.
> Having a combined slot was confusing since the codepaths were separate anyways.
> Additionally dataChanged was emitted in the wrong slot, as it is required
> to notify about the changed fetch state, so it has to be emitted after the
> itemFetchJob.
> 
> ETM: removed unused code.
> 
> 
> ETM: Don't use reinterpret_cast to cast void* to class.
> 
> static_cast is what we want, reinterpret_cast in the best case does the same
> and in the worst case breaks something.
> 
> ETMPopulationTest: Test signals while referencing.
> 
> 
> ETM: only emit rowsInserted when we actually insert a new row.
> 
> And otherwise emit the signal individually for the subrows
> 
> ETM: use the correct parent for inserted ancestors.
> 
> ETM: Removed FirstList hack.
> 
> This was never working with a mimetype filter because the toplevel collections
> would never match the filter and the initial fetch was thus always empty.
> It used to be fixed automatically because the second fetch used to be
> a recursive fetch from root anyways.
> 
> Simply listing all collections right away should be fast enough, and removing
> the hack allowed me to fold a bunch of codepaths into one.
> 
> ETMPopulationTest: displayfilter test
> 
> 
> ETM: Support for referencing collections.
> 
> 
> ETM: Monitoring specific collections only.
> 
> 
> Referenced support for CollectionModifyJob & Collection
> 
> Including CollectionJobTest.
> 
> ETM-Test: Only trigger the event loop but don't wait any longer.
> 
> 
> ETM: Only keep collection if it has subcollections.
> 
> Otherwise a disabled collection that contains items is never removed.
> 
> Collection: Only set enabledChanged if it has changed.
> 
> The code before would reset enabledChanged to false should we i.e. call
> setEnabled(true) twice
> 
> 
> Diffs
> -----
> 
>   akonadi/collection.h ac2548861fe5a17474da7e2bd42a18ab2b369758 
>   akonadi/collection.cpp 42f1f0a09c8d8b17573235343181d11074225380 
>   akonadi/collection_p.h 5ff173e00c76be6a581ed51a705e964c076d8df9 
>   akonadi/collectionmodifyjob.cpp 6ae772e833b4bc22e43dc700951e50206f910aec 
>   akonadi/entitytreemodel.h 2d52a3d7cd04ffa4d8c313ff41f1f607f8f51c78 
>   akonadi/entitytreemodel.cpp 7fb35f584ed8dd97aa6a8e984d9d4cc3d1438aa3 
>   akonadi/entitytreemodel_p.h ecd3c675ff12fef9be83a6936a14d59a9ec8968e 
>   akonadi/entitytreemodel_p.cpp b4d292b36766a9beaa39c37e0da219415a9dda33 
>   akonadi/protocolhelper.cpp 7e98b7775b5df85df62af22364c960d4f00b3947 
>   akonadi/protocolhelper_p.h b0303a327f0ab12504c4c92c733b2c1c5c9312a5 
>   akonadi/tests/CMakeLists.txt e2a4e2423c994b1e65fe89fc0a7f93c690688dee 
>   akonadi/tests/collectionjobtest.h 233bf0d49c91ad209eef113811a3e871eacb93e3 
>   akonadi/tests/collectionjobtest.cpp 566398d926b0ed83c8d00d9f60cf065c3f5a3fb3 
>   akonadi/tests/entitytreemodeltest.cpp d98df38dfdd20a5438ae8a8f8487416793986cb9 
>   akonadi/tests/etmpopulationtest.cpp PRE-CREATION 
>   akonadi/tests/fakeakonadiservercommand.h 3dd928c24a0ab1a5080bf4ceb1166641eb8c7e09 
>   akonadi/tests/fakeakonadiservercommand.cpp e71559515806fb2aafd1aec9af3ef6bbc9058b3d 
> 
> Diff: https://git.reviewboard.kde.org/r/118702/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/