> On Jan. 22, 2015, 7:47 a.m., David Faure wrote:
> > kdeui/tests/krecursivefilterproxymodeltest.cpp, line 126
> > <https://git.reviewboard.kde.org/r/120119/diff/1/?file=310594#file310594line126>
> >
> > Shouldn't rowsInserted even be emitted 3 times? Once for row1, once for
> > child, once for subchild?
>
> Christian Mollekopf wrote:
> AFAIK our models typically query for children, so toplevel is enough if
> children are immediately available.
You're right. We should however check that rowsInserted was emitted at the
right "level". My usual solution for that is to come up with a string
representation for indexes. Either the data string or some 1.1.1. stuff. Makes
writing unittests easy
(QCOMPARE(rowsInsertedSignals(), QString("0,1,A")) for rowsInserted(from=0,
to=1, parent=A (index whose data is "A")).
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120119/#review74519
-----------------------------------------------------------
On Jan. 22, 2015, 3:11 p.m., Christian Mollekopf wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120119/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2015, 3:11 p.m.)
>
>
> Review request for kdelibs and Stephen Kelly.
>
>
> Bugs: 338950
> http://bugs.kde.org/show_bug.cgi?id=338950
>
>
> Repository: kdelibs
>
>
> Description
> -------
>
> (This problem probably applies to KF5 as well, and we'll need to forward port
> this patch.)
>
>
> KRecursiveFilterProxyModel: Fixed the model
>
> The model was not working properly and didn't include all items under
> some circumstances.
> This patch fixes the following scenarios in particular:
>
> * The change in sourceDataChanged is required to fix the shortcut condition.
> The idea is that if the parent is already part of the model (it must be if
> acceptRow returns true),
> we can directly invoke dataChanged on the parent, resulting in the changed
> index
> getting reevaluated. However, because the recursive filterAcceptsRow version
> was used
> the shortcut was also used when only the current index matches the filter and
> the parent index is in fact not yet in the model. In this case we failed to
> call
> dataChanged on the right index and thus the complete branch was never added
> to the model.
>
> * The change in refreshAscendantMapping is required to include indexes that
> were
> included by descendants. The intended way how this was supposed to work is
> that we
> traverse the tree upwards and find the last index that is not yet part of the
> model.
> We would then call dataChanged on that index causing it and its descendants
> to get reevaluated.
> However, acceptRow does not reflect wether an index is already in the model
> or not.
> Consider the following model:
>
> - A
> - B
> - C
> - D
>
>
> If C is include in the model by default but D not and A & B only gets
> included due to C, we have the following model:
> - A
> - B
> - C
> - D
>
> If we then call refreshAscendantsMapping on D it will not consider B as
> already being part of the model.
> This results in the toplevel index A being considered lastAscendant, and a
> call to dataChanged on A results in
> a reevaluation of A only, which is already in the model. Thus D never gets
> added to the model.
>
> Unfortunately there is no way to probe QSortFilterProxyModel for indexes that
> are
> already part of the model. Even the const mapFromSource internally creates a
> mapping when called,
> and thus instead of revealing indexes that are not yet part of the model, it
> silently
> creates a mapping (without issuing the relevant signals!).
>
> As the only possible workaround we have to issues dataChanged for all
> ancestors
> which is ignored for indexes that are not yet mapped, and results in a
> rowsInserted
> signal for the correct indexes. It also results in superfluous dataChanged
> signals,
> since we don't know when to stop, but at least we have a properly behaving
> model
> this way.
>
>
> Diffs
> -----
>
> kdeui/itemviews/krecursivefilterproxymodel.cpp
> 6d6563166bcc9637d826f577925c47d5ecbef2cd
> kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1
> kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/120119/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Christian Mollekopf
>
>
