<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 />
<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>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">added not that this applies to KF5 as well.</pre>
</td>
</tr>
</table>
<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 (updated)</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<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
some circumstances.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The idea is that if the parent is already part of the model (it must be if acceptRow returns true),<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
we can directly invoke dataChanged on the parent, resulting in the changed index<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
getting reevaluated. However, because the recursive filterAcceptsRow version was used<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
the shortcut was also used when only the current index matches the filter and<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
the parent index is in fact not yet in the model. In this case we failed to call<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
included by descendants. The intended way how this was supposed to work is that we<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
traverse the tree upwards and find the last index that is not yet part of the model.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
We would then call dataChanged on that index causing it and its descendants to get reevaluated.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
However, acceptRow does not reflect wether an index is already in the model or not.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Consider the following model:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">- A<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- B<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- C<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- 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:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- A<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- B<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- C<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- 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.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
already part of the model. Even the const mapFromSource internally creates a mapping when called,<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
and thus instead of revealing indexes that are not yet part of the model, it silently<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
which is ignored for indexes that are not yet mapped, and results in a rowsInserted<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
signal for the correct indexes. It also results in superfluous dataChanged signals,<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
since we don't know when to stop, but at least we have a properly behaving model<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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>