[Kde-pim] Review Request 113680: ETM refcounting fixes

[Dan Vrátil]

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


Generally the change makes sense, I'd like Steven to take a look anyway though :)

Except for that, just some coding style issues in the patch. Also please take a look on unifying coding style of the unit test, especially spaces within parentheses - either pick the kdepimlibs with spaces, or kdelibs without spaces (I know you don't like the pimlibs one ;-))


akonadi/monitor_p.cpp
<http://git.reviewboard.kde.org/r/113680/#comment31269>

    Missing spaces around parentCollectionId, add {} please



akonadi/monitor_p.cpp
<http://git.reviewboard.kde.org/r/113680/#comment31270>

    Missing space after parentCollectionId, add {} please



akonadi/monitor_p.cpp
<http://git.reviewboard.kde.org/r/113680/#comment31272>

    I don't see this used anywhere, is it still necessary?



akonadi/monitor_p.cpp
<http://git.reviewboard.kde.org/r/113680/#comment31271>

    Missing spaces around argument


- Dan Vrátil


On Nov. 7, 2013, 4:37 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113680/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2013, 4:37 p.m.)
> 
> 
> Review request for KDEPIM, Dan Vrátil, Stephen Kelly, and Volker Krause.
> 
> 
> Repository: kdepimlibs
> 
> 
> Description
> -------
> 
> This review contains 4 different patches since I seem to be unable to upload patches that depend on each other. Sorry about that, but it should still be reviewable. You can also find the commits in git at git.kde.org:clones/kdepimlibs/cmollekopf/kdepimlibs dev/refcounting.
> 
> 
> Together they should fix http://bugs.kde.org/show_bug.cgi?id=312460, as well as the buffering of reference counted collections.
> => Makes reference counting work as it should.
> 
> I already uploaded a diff separately for the first patch (that is included here as well), that just fixes 312460, but not the other related problems.
> 
> -----
> 
> Simplify logic by using isMonitored consequently.
> 
> -----
> 
> Only buffer a collection after the refcount reaches zero.
> 
> Before a collection that was dereffe'd at least once
> (althouh the refcount is still >0), would already be buffered, resulting in
> the buffer being occupied by reffe'd collections (which is pointless).
> 
> -----
> 
> Don't keep outdated copies of items.
> 
> A collection is purged if reference counting is used and a collection
> exits the buffer after being referenced. By not purging the items, it becomes
> possile that we miss updates, and when refetching the collection because it's
> referenced again, we don't emit change notifications because the items were in the model already.
> 
> Since we anyways have to fetch all items, we can as well purge all items.
> 
> Alternatives:
> * compare revisions and emit change notifications if necessary in itemsFetched
> * Still emit notifications in the monitor for modifications only
> 
> -----
> 
> Fixed fetching of items that exited the buffer after being referenced.
> 
> After a collection exits the buffer after being referenced,
> the monitor no longer emits updates for this collection.
> It is therefore necessary for the ETM to refetch the items to get missing updates.
> 
> 
> Diffs
> -----
> 
>   akonadi/tests/lazypopulationtest.cpp PRE-CREATION 
>   akonadi/tests/CMakeLists.txt 6ac1538da358b33b752ae8511516bf0f684c04ea 
>   akonadi/monitor_p.cpp 778dd752071413b51d68ed97d71f335256be1768 
>   akonadi/monitor_p.h 3d3d60b6121365369e1b42b23d2448874886a1d0 
>   akonadi/entitytreemodel_p.cpp 2505fee5dfc001c843019e29d885baa1c492e6f6 
> 
> Diff: http://git.reviewboard.kde.org/r/113680/diff/
> 
> 
> Testing
> -------
> 
> Tested manually, I'll follow up with unittests.
> 
> 
> 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/