[prev in list] [next in list] [prev in thread] [next in thread]
List: kfm-devel
Subject: Re: Review Request 113168: Filter expanded folders
From: "Frank Reininghaus" <frank78ac () googlemail ! com>
Date: 2013-10-16 21:06:42
Message-ID: 20131016210642.28481.62762 () vidsolbach ! de
[Download RAW message or body]
> 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
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/113168/">http://git.reviewboard.kde.org/r/113168/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 10th, 2013, 4:13 p.m. UTC, <b>Frank \
Reininghaus</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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?</pre> </blockquote>
<p>On October 14th, 2013, 6:26 p.m. UTC, <b>Emmanuel Pescosta</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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)</pre> <br />
<p>- Frank</p>
<br />
<p>On October 8th, 2013, 9:50 a.m. UTC, Emmanuel Pescosta wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Dolphin.</div>
<div>By Emmanuel Pescosta.</div>
<p style="color: grey;"><i>Updated Oct. 8, 2013, 9:50 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-baseapps
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Works fine for me.
Old unit tests don't pass.
New unit tests pass.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>dolphin/src/kitemviews/kfileitemmodel.h <span style="color: \
grey">(d005705)</span></li>
<li>dolphin/src/kitemviews/kfileitemmodel.cpp <span style="color: \
grey">(ea7ac2f)</span></li>
<li>dolphin/src/tests/kfileitemmodeltest.cpp <span style="color: \
grey">(62ff4fa)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113168/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic