----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122252/#review74727 -----------------------------------------------------------
Ship it! some nitpicks, but otherwise looks good to me, esp. as you have tests and tested it on kmail. kdeui/itemviews/krecursivefilterproxymodel.cpp <https://git.reviewboard.kde.org/r/122252/#comment51804> { on newline kdeui/itemviews/krecursivefilterproxymodel.cpp <https://git.reviewboard.kde.org/r/122252/#comment51805> QHash? or is the order important somewhere? kdeui/itemviews/krecursivefilterproxymodel.cpp <https://git.reviewboard.kde.org/r/122252/#comment51806> please use iterators here as well to the double lookup, first for value, then for remove. kdeui/itemviews/krecursivefilterproxymodel.cpp <https://git.reviewboard.kde.org/r/122252/#comment51808> 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 kdeui/itemviews/krecursivefilterproxymodel.cpp <https://git.reviewboard.kde.org/r/122252/#comment51807> again, use iterators kdeui/itemviews/krecursivefilterproxymodel.cpp <https://git.reviewboard.kde.org/r/122252/#comment51809> again, use iterators - Milian Wolff On Jan. 25, 2015, 6:51 p.m., David Faure wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122252/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2015, 6:51 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > ------- > > 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. > > > Diffs > ----- > > kdeui/itemviews/krecursivefilterproxymodel.h > c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4 > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/122252/diff/ > > > Testing > ------- > > 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). > > > Thanks, > > David Faure > >
