Review Request: Possible error in the CollectionTreeItemModelBase fetch cycle

[Maximilian Kossick]

Does that actually have any impact? Any time we have a special query
(VA or NoLabel), there is actually another query running at the same
time, assigned to the same parent as the "special" query. And
populateChildren, which will be called for the normal query, always
calls setRequiresUpdate.

Why is setRequiresUpdate called in handleSpecialQueryResult in the first place?


On Mon, Sep 27, 2010 at 11:41 PM, Leo Franchi <lfranchi at kde.org> wrote:
>
> This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100003/
>
> 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 September 27th, 2010, 11:30 a.m., Nikhil Shantanu Marathe wrote:
>
> Review request for amarok.
> By Nikhil Shantanu Marathe.
>
> Updated 2010-09-27 11:30:13
>
> Description
>
> 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.
>
> 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.
>
> Diffs
>
> src/browsers/CollectionTreeItemModelBase.cpp (c25549b)
>
> View Diff
>
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>