DevHeads.net

Review Request 122227: KRecursiveFilterProxyModel: many many more unittests, and fixing what they found.

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: <a href="https://git.reviewboard.kde.org/r/122227/diff/" title="https://git.reviewboard.kde.org/r/122227/diff/">https://git.reviewboard.kde.org/r/122227/diff/</a>

Testing
Unittest, obviously.
+ KMail smoke testing.

Thanks,

David Faure

Comments

Re: Review Request 122227: KRecursiveFilterProxyModel: many many

By David Faure at 01/26/2015 - 04:16

(Updated Jan. 26, 2015, 8:16 a.m.)

Status
This change has been marked as submitted.

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: <a href="https://git.reviewboard.kde.org/r/122227/diff/" title="https://git.reviewboard.kde.org/r/122227/diff/">https://git.reviewboard.kde.org/r/122227/diff/</a>

Testing
Unittest, obviously.
+ KMail smoke testing.

Thanks,

David Faure

Re: Review Request 122227: KRecursiveFilterProxyModel: many many

By Christian Mollekopf at 01/26/2015 - 03:48

Ship it!

I tested this over the weekend. It fixes the problem originally addressed and seems to fix the crash I was having.

- Christian Mollekopf

On Jan. 23, 2015, 6:11 p.m., David Faure wrote:

Re: Review Request 122227: KRecursiveFilterProxyModel: many many

By Milian Wolff at 01/25/2015 - 18:11

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:

Re: Review Request 122227: KRecursiveFilterProxyModel: many many

By David Faure at 01/26/2015 - 04:15

I don't see this line anywhere in the Qt5 autotests, so it looks like Stephen's metatype changes made it unnecessary. In any case, this review is for Qt4 code, for now :)

fixed

- David

On Jan. 23, 2015, 6:11 p.m., David Faure wrote: