----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122227/#review74725 -----------------------------------------------------------
Ship it! Looking at the code, I can't see anything obvious, but its highly complicate code base anyway. So given that you add all these tests, I'm all for landing this. Many thanks! other than that: the code style is neither kf5 nor qt5, is it? something we might want to change in the future. kdeui/tests/krecursivefilterproxymodeltest.cpp <https://git.reviewboard.kde.org/r/122227/#comment51801> is this still required in qt5? this should really be added upstream, imo... kdeui/tests/krecursivefilterproxymodeltest.cpp <https://git.reviewboard.kde.org/r/122227/#comment51803> style: { on newline? - Milian Wolff On Jan. 23, 2015, 6:11 p.m., David Faure wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122227/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2015, 6:11 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > ------- > > 1) setData(false), i.e. a dataChanged that removes and item from the filter, > didn't actually lead to removal. The code was only looking at changing to > get in, not changing to get out. > > 2) On insertion, we can avoid emitting dataChanged up the chain, by > finding out before the insertion which exact ancestor will be changed > (lastHiddenAscendantForInsert). > > 3) On removal, well simplify the code (completeRemove was always true, unless > ignoreRemove was set, so we only need to keep ignoreRemove), and avoid > emitting dataChanged up the chain, by finding out which the last parent > before one that should still be visible, and hide just that one. > > 4) While at it, an obvious optimization that could have been done > since day one: filterAcceptsRow can return true as soon as a child wants > to be shown. > > > Diffs > ----- > > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/122227/diff/ > > > Testing > ------- > > Unittest, obviously. > + KMail smoke testing. > > > Thanks, > > David Faure > >
