<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/104321/">http://git.reviewboard.kde.org/r/104321/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 18th, 2012, 10:11 a.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Okay, that makes sense, thanks. Pushing the ExcludeUserInputEvents version.</pre>
<br />
<p>- Sam</p>
<br />
<p>On March 17th, 2012, 11:10 p.m., Alexey Neyman wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/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 Alexey Neyman.</div>
<p style="color: grey;"><i>Updated March 17, 2012, 11:10 p.m.</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;">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</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=295275">295275</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/AmarokMimeData.cpp <span style="color: grey">(226b0fa)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104321/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>