[Kde-pim] More about KRecursiveFilterProxyModel

[David Faure]

On Monday 26 January 2015 09:54:11 Christian Mollekopf wrote:
> On Sunday 25 January 2015 19.55:41 David Faure wrote:
> > The problem with the superfluous dataChanged() signals emitted by
> > KRecursiveFilterProxyModel (see e.g.
> > https://git.reviewboard.kde.org/r/122227/)
> > 
> > is that they trigger many calls to "reload()" in the
> > FavoritesCollectionModel. Try adding a kDebug in there and using kmail,
> > it's horrendous (and it's a real time sink, any time I interrupt kmail
> > with
> > gdb, it's in that code - even before my changes to
> > KRecursiveFilterProxyModel, but possibly even more so now that it handles
> > dataChanged more correctly).
> 
> Yeah, FavoriteCollectionsModel is a timesink.
>
> > Somehow it's made even worse by the stack of proxies used in kmail,
> > because
> > in the debug output I see this:
> > imagine you have nested folders like A / B / C / D, if the number of
> > unread
> > emails in D changes, then dataChanged signals are received by
> > FavoritesCollectionModel like this: D, C, B, A, C, B, A, B, A, A.
> > It's as if there were two KRecursiveFilterProxyModel in the stack....
> > (I didn't check whether that was the case).
> 
> I don't think there are two, see below.

Right. I found where this came from though: KStatisticsProxyModel's dataChanged also emits dataChanged up the hierarchy
(because of the case where a subtree is closed and the parent folder then shows the aggregate totals from the children folder).

So I was right about D, C, B, A, C, B, A, B, A, A, just not about the second guilty proxy :)
With my KRFPM patch it's down to D, C, B, A because of KStaticsProxyModel.
How much I wish for Qt5's QVector<int> roles in the dataChanged signal....

> > Apart from reviewing my two RB patches, I'd welcome any input on whether:
> > 
> > * ETM's monitoredCollectionChanged really needs to be emitted so often
> > (it seems any FetchJob emits it, even if nothing changed?)
> 
> What do you mean by *any* fetchjob? Internal fetchjobs?
> We need emit dataChanged rather frequently because we update i.e. collection
> statistics through it (whenever an item is added/removed), or the fetch
> state (if a fetch job is running or not). External fetch jobs should of
> course not result in a dataChanged signal in the ETM, otherwise we have a
> bug in akonadi server.

Yes I mean fetch jobs triggered by the ETM. But during IMAP sync, many
collections have not changed at all, and yet dataChanged is emitted for them,
triggering lots of work for nothing.

ETM dataChanged 1 QModelIndex(0,0,0x22488b0,Akonadi::EntityTreeModel(0x1173940) )  "Personal Contacts" 
17:01:12 kmail2(29832)/libakonadi Akonadi::EntityTreeModelPrivate::monitoredCollectionChanged: collection changed 6419

I know for sure that this collection hasn't changed.
Can't we avoid calling monitoredCollectionChanged when nothing changed?

> > * FavoriteCollectionsModel really needs to do so much work on dataChanged
> > (Christian, I can't help but wonder if your changes a year ago to this
> > class, for performance reasons, actually made it worse - I keep seeing
> > kmail spending time there)
> 
> It's of course possible that FavoriteCollectionsModel ends up emitting more
> dataChanged signals due to this patch. I think at least superfluous
> selectionChanged signals could be avoided by checking the current selection
> before selecting it again (QItemSelectionModel::select doesn't do that it
> seems). Currently we're reselecting the index on every dataChanged signal,
> and reselecting all indexes on every layoutChanged signals.

Oh, this is a very good point. I'll try that locally.
http://www.davidfaure.fr/2015/select-optimisation.diff

Another thing I've been trying is to comment out the code in the dataChanged slot
in favoritecollectionsmodel.cpp. I can't see any reason why this would ever be useful?
Favorite collections are recognized by ID, and an ID doesn't change...
 
> > * Maybe FavoriteCollectionsModel could sit on top of ETM (or some
> > intermediate model that just filters for folders) rather than sitting on
> > top of so many proxies, so that it gets called less often? (the fact that
> > any QSFPM turns any source dataChanged into a layoutChanged really doesn't
> > help with performance in models sitting on top...)
> 
> The stack is (from my notes):
> 
> KMKernel::collectionModel() (the ETM)
> QuotaColorProxyModel (KIdentityProxyModel)
> KPIM:StatisticsProxyModel (KIdentityProxyModel)
> FolderTreeWidgetProxyModel (KRecursiveFilterProxyModel)
> EntityCollectionOrderProxyModel (QSortFilterProxyModel)
> FavoriteCollectionsModel

Thanks, that's useful to know.

> The problem is we probably want the same sorting and filtering, but we could
> of course just add those models on top of the FavoriteCollectionsModel
> again.

Why would we want sorting and filtering in the FavoriteCollectionsModel?
The model displays N folders of its choice, sorted by the user, doesn't it?

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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