<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120119/">https://git.reviewboard.kde.org/r/120119/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 22nd, 2015, 7:47 a.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The commit log has the same tree twice. Was "D" supposed to be omitted from the second tree?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Overall I'm extremely surprised that this class didn't have a unittest yet.
I recently wrote many proxymodels for a customer, with full test coverage, so this hurts my eyes. I will expand this unittest once it's in.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If I understand correctly, the bug in refreshAscendantMapping is that basically the code was hoping to test the "old" state of the tree, while in fact acceptRow returns the new state already (so in this case, acceptRow returns true for every ascendant, so we don't know where to stop). A cache (remembering what we have inserted into the tree) would solve this, but managing the cache would be so much more work.
A more clever solution could be to store the state of things (in fact just "lastAscendant") in a slot connected to rowsAboutToBeInserted, and use that later in the slot connected to rowsInserted. What do you think of this approach?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(Personally I think we need more unittests before making any kind of change :-)</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"D" indeed was supposed to be omitted from the second tree.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">More tests for that model are clearly not a bad idea ;-) </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we could test the old state that could indeed solve the problem, not sure what the original intentations fo the code where. In general we simply need a way to ask the model wether it already has a mapping for a certain index, but I couldn't find anything providing this information. So IMO it's either QSortFilterProxyModel's fault that it doesn't provide that information and we have to workaround that, or we shouldn't be using QSortFilterProxyModel. The caching of the information does sound like yet another thing that can break in this already fragile construct, so I'd rather avoid it. Besides performance superfluous dataChanged signals shouldn't hurt, so I'd suggest to stick to that. In th
e long run we should figure out wether we can adjust QSortFilterProxyModel to facilitate our usecase, or move away from a QSortFilterProxyModel based implementation.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 22nd, 2015, 7:47 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120119/diff/1/?file=310594#file310594line126" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/tests/krecursivefilterproxymodeltest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">126</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">spy</span><span class="p">.</span><span class="n">mSignals</span><span class="p">,</span> <span class="n">QStringList</span><span class="p">()</span> <span class="o"><<</span> <span class="n">QLatin1String</span><span class="p">(</span><span class="s">"rowsInserted"</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Shouldn't rowsInserted even be emitted 3 times? Once for row1, once for child, once for subchild?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">AFAIK our models typically query for children, so toplevel is enough if children are immediately available.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 22nd, 2015, 7:47 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120119/diff/1/?file=310594#file310594line159" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/tests/krecursivefilterproxymodeltest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">159</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">spy</span><span class="p">.</span><span class="n">mSignals</span><span class="p">,</span> <span class="n">QStringList</span><span class="p">()</span> <span class="o"><<</span> <span class="n">QLatin1String</span><span class="p">(</span><span class="s">"rowsInserted"</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would also expect 3 signal emissions here....</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">same reason as above</p></pre>
<br />
<p>- Christian</p>
<br />
<p>On September 10th, 2014, 8:15 a.m. UTC, Christian Mollekopf wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for kdelibs and Stephen Kelly.</div>
<div>By Christian Mollekopf.</div>
<p style="color: grey;"><i>Updated Sept. 10, 2014, 8:15 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=338950">338950</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(This problem probably applies to KF5 as well, and we'll need to forward port this patch.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KRecursiveFilterProxyModel: Fixed the model</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The model was not working properly and didn't include all items under
some circumstances.
This patch fixes the following scenarios in particular:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">* 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">* 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:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">- A
- B
- C
- D</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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!).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></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>kdeui/itemviews/krecursivefilterproxymodel.cpp <span style="color: grey">(6d6563166bcc9637d826f577925c47d5ecbef2cd)</span></li>
<li>kdeui/tests/CMakeLists.txt <span style="color: grey">(f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1)</span></li>
<li>kdeui/tests/krecursivefilterproxymodeltest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120119/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>