Review Request: Possible error in the CollectionTreeItemModelBase fetch cycle

[Leo Franchi]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100003/#review26
-----------------------------------------------------------

Ship it!


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 :)

- Leo


On 2010-09-27 11:30:13, Nikhil Shantanu Marathe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100003/
> -----------------------------------------------------------
> 
> (Updated 2010-09-27 11:30:13)
> 
> 
> Review request for amarok.
> 
> 
> Summary
> -------
> 
> Just another heads up of something I sent earlier but didn'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 "Various Artists" item.
> 
> Consider the case when there are no entries tagged "Various Artists"
> 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's parent,
>    //we have to call setRequiresUpdate on the parent too
>    //yes, this will mean we will call setRequiresUpdate twice
>    parent->setRequiresUpdate( false );
> 
> where we have actually FOUND entries, and so we stop trying to update
> the parent node again.
> 
> My question is, shouldn't this be the case when dataList is empty too?
> Which means shouldn't we call parent->setRequiresUpdate( false ); even
> when we get empty results. The attached patch fixes what I think is a bug.
> 
> 
> Diffs
> -----
> 
>   src/browsers/CollectionTreeItemModelBase.cpp c25549b 
> 
> Diff: http://git.reviewboard.kde.org/r/100003/diff
> 
> 
> Testing
> -------
> 
> I use this fix all the time in my local build and it doesn't seem to cause problems, and it definitely prevents my upnp-collection from being polled again and again because it doesn't return any Various Artists entries.
> 
> 
> Thanks,
> 
> Nikhil Shantanu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20100927/18ceb210/attachment.htm