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



 <p>Ship it!</p>



 <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good to me, though I am not an expert in this part of Amarok. Still, you can commit for more testing and feedback, 2.4 is a while away :)</pre>
 <br />







<p>- Leo</p>


<br />
<p>On September 27th, 2010, 11:30 a.m., Nikhil Shantanu Marathe wrote:</p>






<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
 <tr>
  <td>

<div>Review request for amarok.</div>
<div>By Nikhil Shantanu Marathe.</div>


<p style="color: grey;"><i>Updated 2010-09-27 11:30:13</i></p>




<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;">Just another heads up of something I sent earlier but didn&#39;t get replies to. Markey suggested this might be a good place.

This is a fix regarding the exact conditions under which a
setRequiresUpdate(false) is done on a CollectionTreeItem, the parent
of a &quot;Various Artists&quot; item.

Consider the case when there are no entries tagged &quot;Various Artists&quot;
returned by a QueryMaker. In that case, the
CollectionTreeItemModelBase::handleSpecialQueryResult() function
receives a dataList of size == 0. If dataList is empty() we remove the
special node from the tree (or it may not exist previously). This
means we are no longer looking for a various artists type result.

Now consider the middle of :handleSpecialQueryResult

   //populate children will call setRequiresUpdate on vaNode
   //but as the special query is based on specialNode&#39;s parent,
   //we have to call setRequiresUpdate on the parent too
   //yes, this will mean we will call setRequiresUpdate twice
   parent-&gt;setRequiresUpdate( false );

where we have actually FOUND entries, and so we stop trying to update
the parent node again.

My question is, shouldn&#39;t this be the case when dataList is empty too?
Which means shouldn&#39;t we call parent-&gt;setRequiresUpdate( false ); even
when we get empty results. The attached patch fixes what I think is a bug.</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;">I use this fix all the time in my local build and it doesn&#39;t seem to cause problems, and it definitely prevents my upnp-collection from being polled again and again because it doesn&#39;t return any Various Artists entries.</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>src/browsers/CollectionTreeItemModelBase.cpp <span style="color: grey">(c25549b)</span></li>

</ul>

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




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








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