Review Request 120119: KRecursiveFilterProxyModel: Fixed the model

[David Faure]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150122/d6feac57/attachment.htm>