Review Request 113168: Filter expanded folders

[Frank Reininghaus]

> On Oct. 10, 2013, 4:13 p.m., Frank Reininghaus wrote:
> > Thanks for the patch!
> > 
> > I did not quite remember what the behavior of QSortFilterProxyModel is when a filter is used in a tree view, so I built the KDE/4.7 branch of kde-baseapps and tried it. It only shows an item if it matches the filter, and all of its parents match the filter as well.
> > 
> > The current behavior of Dolphin is that expanded folders are always shown when filtering. I think that this is better than the QSortFilterProxyModel behavior because it allows to view all matching items in expanded folders (but that might be a matter of taste).
> > 
> > I agree that your approach to the problem is the most "correct" one because it prevents that expanded folders are shown if they have no direct or indirect children that match the filter.
> > 
> > However, I'm a bit worried about the increase in code complexity. Changes are needed in quite a few places (and to be 100% correct, you would need even more changes - for example, deleting a matching child item does not cause the parent(s) to be removed at the moment). Moreover, I think that the new "filterItems" functions that your patch adds don't make it obvious what some of their parameters and return values mean (but I can't see a possible better solution), so the code does get harder to read.
> > 
> > Therefore, I'm unsure if this issue is important enough to justify that we make KFileItemModel, which is quite a complex beast already, even more complex. But I'm interested in other opinions. Do people consider the current behavior problematic?
> 
> Emmanuel Pescosta wrote:
>     > I think that this is better than the QSortFilterProxyModel behavior
>     Yes definitely! ;)
>     
>     > justify that we make KFileItemModel, which is quite a complex beast already, even more complex
>     Hmm ... maybe we should consider some code refactoring in KFileItemModel to increase the maintainability and extensibility? 
>     Maybe smth. for the next dev cycle? ;)
>     
>     After thinking about this (non-trivial) patch again, I can understand your worries and I'll close this report if noone has a problem with the current behavior.

> Hmm ... maybe we should consider some code refactoring in KFileItemModel to increase the maintainability and extensibility? 
> Maybe smth. for the next dev cycle? ;)

What ideas do you have for the refactoring? (could also be discussed in a separate thread on the ML instead of here on ReviewBoard)


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113168/#review41527
-----------------------------------------------------------


On Oct. 8, 2013, 9:50 a.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113168/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2013, 9:50 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> The current filtering implementation doesn't filter out expanded directories, also
> when there are no sub-items anymore and they don't match with the filter string.
> 
> With this patch the filtering is done right ;)
> 
> For example (folders a, a/b and a/b/c):
> Expand a and b
> 
> a-|
>   |-b-|
>       |-c
> 
> Set the filter string to "b". 
> 
>   * Current implementation
> 
> a-|
>   |-b-|
>       |-c
> 
>   * Patched version:
> 
> a-|
>   |-b
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.h d005705 
>   dolphin/src/kitemviews/kfileitemmodel.cpp ea7ac2f 
>   dolphin/src/tests/kfileitemmodeltest.cpp 62ff4fa 
> 
> Diff: http://git.reviewboard.kde.org/r/113168/diff/
> 
> 
> Testing
> -------
> 
> Works fine for me.
> 
> Old unit tests don't pass.
> New unit tests pass.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20131016/28c2f15e/attachment.htm>