Review Request 120119: KRecursiveFilterProxyModel: Fixed the model

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120119/#review74552
-----------------------------------------------------------

Ship it!


Looks good to me, let's get this in, I'll try to find time to improve the test (and then possibly the implementation).

- David Faure


On Jan. 22, 2015, 4:06 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, 4:06 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/b14d996c/attachment.htm>