Review Request: Fix bug 295275

[Matěj Laitl]

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


Hmm, my understanding why the code was there:

1) User starts to drag tracks from a collection tree view: CollectionTreeItemModelBase::mimeData() has following code:
    AmarokMimeData *mimeData = new AmarokMimeData();
    mimeData->setTracks( tracks );
    mimeData->setQueryMakers( queries );  // queries are non-empty only if you have something in search field. try: "tracknumber:1"
    mimeData->startQueries();
2) QueryMakers are started and processing while the user is dragging
3) User drops tracks; if she is _very_ quick, not all QueryMakes have completed.  AmarokMimeData::tracks() therefore must assure that all QueryMakers complete (and the newResultReady() and queryDone() slots are activated) before returning the tracks, which is done in the loop this patch removes.

I'd suggest using QEventLoop::ExcludeUserInputEvents, too.

- Matěj Laitl


On March 17, 2012, 11:10 p.m., Alexey Neyman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104321/
> -----------------------------------------------------------
> 
> (Updated March 17, 2012, 11:10 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> This patch fixes issue https://bugs.kde.org/show_bug.cgi?id=295275; here is
> a discussion from #amarok:
> 
> [15:41:30] <stilor> Sentynel: could you reproduce if the processEvents() loop is removed from AmarokMimeData::tracks()?
> [15:42:43] <stilor> idea is, drag-n-drop, AFAIU is two events, "drag" and "drop". The 1st event is fetched before CollectionTreeView::dragEnterEvent is called
> [15:42:58] <Sentynel> sounds plausible, let's have a look
> [15:43:10] <stilor> so this processEvents() loop fetches the "drop" event for which it doesn't have source
> [15:44:45] <stilor> I played aggressively with my mouse for a few minutes and can't get it to crash anymore
> [15:44:56] <Sentynel> yeah I can't get it to crash either
> [15:45:04] <Sentynel> so the next question is, why is that code there in the first place
> [15:45:39] <stilor> I have no idea, I didn't look into Amarok code until two days ago ;)
> [15:45:49] <Sentynel> whatever it is it's been there since 2007
> [15:48:05] <stilor> yeah, I found the commit but the message doesn't say anything about why nested event processing is needed
> [15:51:15] <Sentynel> well, it doesn't seem to break anything as far as I can tell...
> [15:51:29] <Sentynel> my suggestion would be to submit a review request for taking that loop out
> [15:51:33] <Sentynel> and see if anybody else has any ideas why it's there
> [15:54:01] <Sentynel> the other thing that works is changing QEventLoop::AllEvents to QEventLoop::ExcludeUserInputEvents
> [15:54:22] <Sentynel> which stops the crash and should theoretically still let it process whatever events it was trying to process
> [15:54:51] <Sentynel> but I'd rather get rid of that code block entirely if it's not doing anything useful
> 
> 
> This addresses bug 295275.
>     https://bugs.kde.org/show_bug.cgi?id=295275
> 
> 
> Diffs
> -----
> 
>   src/AmarokMimeData.cpp 226b0fa 
> 
> Diff: http://git.reviewboard.kde.org/r/104321/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexey Neyman
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120318/1c1dee7e/attachment-0001.html>