[Kde-pim] Review Request 114629: Separated referencing of collections from the selection in FavoriteCollectionsModel.

[Christian Mollekopf]

> On Jan. 4, 2014, 12:05 p.m., Kevin Krammer wrote:
> > akonadi/favoritecollectionsmodel.cpp, line 163
> > <https://git.reviewboard.kde.org/r/114629/diff/2/?file=226987#file226987line163>
> >
> >     if you put the model manipulation into, say, dereferenceHelper() then you could call it from here and from clearReferences(), avoiding the lookup overhead when clearing

This codepath is not called very often (only while loading and on manual user interaction), so I'll keep it as is since I think it keeps the code simpler and easier to read.


> On Jan. 4, 2014, 12:05 p.m., Kevin Krammer wrote:
> > akonadi/favoritecollectionsmodel.cpp, line 254
> > <https://git.reviewboard.kde.org/r/114629/diff/2/?file=226987#file226987line254>
> >
> >     is the order of references important? otherwise a QSet might be better, i.e. faster then linear lookups

A QSet is indeed cleaner and faster.

Thanks for reviewing Kevin =)


> On Jan. 4, 2014, 12:05 p.m., Kevin Krammer wrote:
> > akonadi/favoritecollectionsmodel.cpp, line 166
> > <https://git.reviewboard.kde.org/r/114629/diff/2/?file=226987#file226987line166>
> >
> >     Maybe use referencedCollections.find() in line 159 and use referencedCollections.erase() here?
> >     not sure how often this is called but that sounds like a simple optimization

Same as above.


- Christian


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


On Dec. 23, 2013, 7:11 a.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114629/
> -----------------------------------------------------------
> 
> (Updated Dec. 23, 2013, 7:11 a.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Bugs: 327865
>     http://bugs.kde.org/show_bug.cgi?id=327865
> 
> 
> Repository: kdepimlibs
> 
> 
> Description
> -------
> 
> Separated referencing of collections from the selection in FavoriteCollectionsModel.
> 
> The FavoritCollectionsModel used an internal selection model (that isn't used in any GUI),
>  to reuse the referencing of the Akonadi::SelectionProxyModel that corresponds to the current selection.
> However, the selection is frequently cleared and restored during
> layoutChanged signals, resulting in the collections frequently getting
> dereferenced and rereferenced.
> Since this happens for each favorite collection, this resultet in the
> complete buffer getting cleared on every layoutChanged result, essentially
> killing the buffering
> (if 10 favorite collections get dereferenced, each of them ends up in the
> buffer pushing other buffered collections out of the buffer).
> 
> Since a QSortProxyModel that filters dynamically translates each dataChanged
> signal into a layoutChanged, the buffer was more or less constantly cleared.
> 
> We could try to avoid clearing the selection on layoutChanged signals in KSelectionProxyModel,
> but not relying on the internal selection in the first place seem much more
> robust and straightforward. The FavoriteCollectionsModel remains a subclass
> of SelectionProxyModel for binary compatibility only, and should eventually be
> turned into a KRecursiveFilterModel.
> 
> 
> Diffs
> -----
> 
>   akonadi/favoritecollectionsmodel.h 17faff1275d500782fccd4ec52c8db5b9f467bd1 
>   akonadi/favoritecollectionsmodel.cpp 884a206088edad5d2699dd8570e3822323e89a27 
>   akonadi/tests/CMakeLists.txt 76d49ad903287c23b0db9097b09667571f759e98 
>   akonadi/tests/favoriteproxytest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/114629/diff/
> 
> 
> Testing
> -------
> 
> Tried with kmail.
> 
> 
> 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/