-----------------------------------------------------------
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
>
>
_______________________________________________
Amarok-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/amarok-devel
