KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals

Review Request #122252 - Created Jan. 25, 2015 and updated

Information
David Faure
kdelibs
master
7fddda6...
Reviewers
kdelibs
cmollekopf

by using an internal cache of the filtering state.

The tricky part is that filterAcceptsRow() must not use the cache
(too bad, it would be faster), because of the setFilterFixedString()
feature which can change the filtering status of indexes without
KRFPM even being informed (QSFPM has no virtual method, no event...)
So it only ever writes to the cache, but when dataChanged()
or row insertion/removal comes in, we can look into the cache
to find the old state and compare.

Unit tests.

Using kmail (and especially testing the substring filtering in the "move dialog", which an earlier version of this patch broke).
Looking at the number of calls to Akonadi::FavoriteCollectionsModel::Private::reload() for a single dataChanged() signal from the ETM, which went from 10 to 4 (still too many, but the remaining problem is elsewhere).

Issues

  • 6
  • 0
  • 0
  • 6
Description From Last Updated
{ on newline Milian Wolff Milian Wolff
QHash? or is the order important somewhere? Milian Wolff Milian Wolff
please use iterators here as well to the double lookup, first for value, then for remove. Milian Wolff Milian Wolff
either use qCDebug, or wrap this in a macro that is easy to enable/disable: // #define ifDebug(x) x #define ifDebug(x) ... Milian Wolff Milian Wolff
again, use iterators Milian Wolff Milian Wolff
again, use iterators Milian Wolff Milian Wolff
Milian Wolff

some nitpicks, but otherwise looks good to me, esp. as you have tests and tested it on kmail.

{ on newline

QHash? or is the order important somewhere?

please use iterators here as well to the double lookup, first for value, then for remove.

either use qCDebug, or wrap this in a macro that is easy to enable/disable:

// #define ifDebug(x) x
#define ifDebug(x)
...

ifDebug(qDebug() << index.data() ...);

also below

again, use iterators

again, use iterators

Christian Mollekopf

Looks reasonable to me. I'll apply the patch locally and test it for a while.

  1. This patch brings the original problem back, that shared folders do not appear until something causes a dataChanged signal (usually a sync). Since the model now seems to be behaving correctly, I assume the kmail model stack is buggy in yet another place (would have been to trivial otherwise wouldn't it?), and the superfluous dataChanged signal just happened to hide that problem.

  2. I tracked the problem down to still be in FolderTreeWidgetProxyModel (which is a KRecursiveFilterProxyModel). I know the relevant index makes it through the sourcemodel because of the debug output added in FolderTreeWidgetProxyModel::acceptRow (which also returns the correct values). I'm fairly sure that the model is the cause for the folder not showing up because I set the foldertreewidgetproxymodel directly as source of the folderTreeView (a QTreeView). Given that the filtering seems to work correctly, and assuming QTreeView isn't buggy, the reason for the folder not showing up has to be in KRecursiveFilterProxyModel (right?). The problem is most likely timing/order dependant because I cannot reproduce the problem with another user but the same kmail/kdelibs version + the same account.

    The problematic folder tree looks as follows:

      * Shared Folders (no mimetype)
      ** shared (no mimetype)
      *** lists (no mimetype)
      **** kde.org (no mimetype)
      ***** pim (mail mimetype)
      ***** ...
      **** ...
      *** ...
    

    The problem is that the complete folder hierarchy, including "Shared Folders" doesn't make it into the visible tree. The top 4 folders of the hierarch would of course only be pulled in by the actual mail folder (pim).

    So far I couldn't find the actual problem to write another testcase, but I have to assume that KRecursiveFilterProxyModel ist still buggy, and the additional dataChanged signal just happen to rectify the problem.

  3. This definitely sounds like missing rowsInserted signals from the proxy.

    I'll try adding a testcase where the source model does rowsInserted(1), rowsInserted(1.1), rowsInserted(1.1.1 + with flag set).

  4. Hmm, that's exactly what testInsert() already does.

    If you can't reproduce this on another user, I guess there's little point in me trying to reproduce it.
    Can you try to trace:
    - the rows{AboutToBe,}Inserted signals emitted by the source
    - the rowsInserted and dataChanged emitted by the proxy
    ?

    The first information will give us more chances of writing a testcase that reproduces the issue,
    and (assuming the first information looks correct), the second information will confirm that it's the proxy being buggy, not something else.

    If there's a way to avoid too much unrelated noise from other folders, you could also uncomment all qDebugs in krfpm.cpp, so we get a full trace of what happens internally.

    All this with this patch applied, of course.

Albert Astals Cid

No progress here in 2 years and kdelibs is in very-life-support, should it be discarded?

  1. The same patch would apply to KF5, the problem isn't kdelibs specific.

    The new plan is to have this as part of Qt (https://codereview.qt-project.org/151000) then port kdepim to it, and then we'll have to retest the regression that Christian reported here. If you want we can close the RR, we just need to keep in mind to retest this scenario with the new code...

Loading...