'Re: Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals
From:       "Christian Mollekopf" <chrigi_1 () fastmail ! fm>
Date:       2015-01-26 9:41:40
Message-ID: 20150126094140.30682.32469 () probe ! kde ! org
[Download RAW message or body]

--===============2550577652790142036==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


Looks reasonable to me. I'll apply the patch locally and test it for a while.

- Christian Mollekopf


On Jan. 25, 2015, 6:51 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122252/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2015, 6:51 p.m.)
> 
> 
> Review request for kdelibs and Christian Mollekopf.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> by using an internal cache of the filtering state.
> 
> The tricky part is that filterAcceptsRow() must not use the cache
> (too bad, it would be faster), because of the setFilterFixedString()
> feature which can change the filtering status of indexes without
> KRFPM even being informed (QSFPM has no virtual method, no event...)
> So it only ever writes to the cache, but when dataChanged()
> or row insertion/removal comes in, we can look into the cache
> to find the old state and compare.
> 
> 
> Diffs
> -----
> 
> kdeui/itemviews/krecursivefilterproxymodel.h \
> c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4  \
> kdeui/itemviews/krecursivefilterproxymodel.cpp \
> efa286ad87ded962b20c8a581b659d1b154ebf3a  \
> kdeui/tests/krecursivefilterproxymodeltest.cpp \
> 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6  
> Diff: https://git.reviewboard.kde.org/r/122252/diff/
> 
> 
> Testing
> -------
> 
> Unit tests.
> 
> Using kmail (and especially testing the substring filtering in the "move dialog", \
> which an earlier version of this patch broke). Looking at the number of calls to \
> Akonadi::FavoriteCollectionsModel::Private::reload() for a single dataChanged() \
> signal from the ETM, which went from 10 to 4 (still too many, but the remaining \
> problem is elsewhere). 
> 
> Thanks,
> 
> David Faure
> 
> 


--===============2550577652790142036==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit




<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/122252/">https://git.reviewboard.kde.org/r/122252/</a>
  </td>
    </tr>
   </table>
   <br />





 <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;">Looks \
reasonable to me. I'll apply the patch locally and test it for a while.</p></pre>  \
<br />









<p>- Christian Mollekopf</p>


<br />
<p>On January 25th, 2015, 6:51 p.m. UTC, David Faure 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 Christian Mollekopf.</div>
<div>By David Faure.</div>


<p style="color: grey;"><i>Updated Jan. 25, 2015, 6:51 p.m.</i></p>









<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;">by using an internal cache of the filtering state.</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The tricky part is that filterAcceptsRow() must not \
use the cache (too bad, it would be faster), because of the setFilterFixedString()
feature which can change the filtering status of indexes without
KRFPM even being informed (QSFPM has no virtual method, no event...)
So it only ever writes to the cache, but when dataChanged()
or row insertion/removal comes in, we can look into the cache
to find the old state and compare.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Unit tests.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Using kmail (and \
especially testing the substring filtering in the "move dialog", which an earlier \
version of this patch broke). Looking at the number of calls to \
Akonadi::FavoriteCollectionsModel::Private::reload() for a single dataChanged() \
signal from the ETM, which went from 10 to 4 (still too many, but the remaining \
problem is elsewhere).</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.h <span style="color: \
grey">(c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4)</span></li>

 <li>kdeui/itemviews/krecursivefilterproxymodel.cpp <span style="color: \
grey">(efa286ad87ded962b20c8a581b659d1b154ebf3a)</span></li>

 <li>kdeui/tests/krecursivefilterproxymodeltest.cpp <span style="color: \
grey">(3bcb72980730cb22f887ae8fa5fbd91b5609aeb6)</span></li>

</ul>

<p><a href="https://git.reviewboard.kde.org/r/122252/diff/" style="margin-left: \
3em;">View Diff</a></p>






  </td>
 </tr>
</table>








  </div>
 </body>
</html>


--===============2550577652790142036==--


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic