Allow dragging from View mode to external applications
ClosedPublic

Authored by huoni on Apr 1 2018, 11:35 PM.

Details

Summary

This patch adds drag functionality to View mode. A
drag operation is only initiated if dragging wouldn't
otherwise do something, e.g. when zoomed and the image is
pannable, or the crop tool is active.

In order to get a drag pixmap for all cases (videos
included), we use a ThumbnailProvider to generate a file
preview. However, if the image is modified, we generate a
new pixmap using the edited version. Using
MimeTypeUtils::selectionMimeData, this should mean the
drag pixmap always matches the image the receiving
application gets.

BUG: 386034
FIXED-IN: 18.08.0

Test Plan

Drag from View mode, comparison mode.
Try raster, SVG, and video file types, including when the
image is modified (cropped or rotated).
Panning a zoomed image should still be possible.
Drag operation should always get a thumbnail/icon pixmap
representing the dropped image.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
huoni requested review of this revision.Apr 1 2018, 11:35 PM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Apr 1 2018, 11:36 PM
huoni added a project: Gwenview.
huoni updated this revision to Diff 32031.Apr 13 2018, 3:07 AM
huoni edited the summary of this revision. (Show Details)
  • rebase
  • update summary
rkflx requested changes to this revision.May 18 2018, 10:57 PM

Thanks for implementing this feature ;)

Before I look at the code in detail, some comments:

  • You might want to test whether (or rather how…) this interferes with Crop.
  • Dragging is already implemented in Browse mode and for the Thumbnail Bar. Is there any reason you are (partly) reinventing the wheel regarding the thumbnails?
  • Would it make sense (if technically possible) to support dragging and dropping the modified image like in D12871?
  • For startDragIfSensible, is there already similar code which determines whether to show the bird's eye view in the bottom right corner?
This revision now requires changes to proceed.May 18 2018, 10:57 PM
huoni added a comment.May 20 2018, 9:12 AM

Thanks for implementing this feature ;)

Before I look at the code in detail, some comments:

  • You might want to test whether (or rather how…) this interferes with Crop.

That was a glaring oversight on my part! At least it's an easy fix :)

  • Dragging is already implemented in Browse mode and for the Thumbnail Bar. Is there any reason you are (partly) reinventing the wheel regarding the thumbnails?

Had a good look into whether we could leverage the existing thumbnail code, but found it impractical due to:

  1. The thumbnail view thumbnails are dependent on the directory model. If one opens Gwenview directly to View, the directory model isn't loaded before the image is shown
  2. The code is spread over multiple classes, there is no single place we can hook into.

That said, we can use KIO::PreviewJob to fetch a thumbnail when the document loads (this is what the existing code does for anything that isn't a raster image).
But there are issues with this. 1) I believe it accesses the disk (works on a KFileItem), 2) It needs to be done preemtively, since it's a background job, meaning we are creating thumbnails that may never be used, and 3) it doesn't take advantage of the thumbnail cache (see problem 2 above).

  • Would it make sense (if technically possible) to support dragging and dropping the modified image like in D12871?

We can do this, but there's an issue. What do we set the drag pixmap to? Whether we set it to the original image or the edited image, there will be cases when the receiving application renders the wrong one.
E.g. user edits image > drag shows thumbnail of edited image > drops in GIMP > GIMP gets the unedited image.

  • For startDragIfSensible, is there already similar code which determines whether to show the bird's eye view in the bottom right corner?

That logic happens in BirdEyeView, which connects to the DocumentView::zoomChanged signal. Also that logic takes other things into account. So it isn't practical to piggy back on that. And I don't think it's practical to separate out part of the logic into DocumentView to avoid minor duplication.


So we have some options:

  1. Generate a thumbnail from the existing views/adapters - happens on demand, but some duplication and videos only get an icon
  2. Use KIO::PreviewJob to create a thumbnail for every DocumentView - possible disk usage, wastes memory when the user doesn't drag (most of the time), but videos get a proper thumbnail
  3. Don't use the drag pixmap at all - avoids confusion regarding edited images

Thoughts?

  • Dragging is already implemented in Browse mode and for the Thumbnail Bar. Is there any reason you are (partly) reinventing the wheel regarding the thumbnails?

Had a good look into whether we could leverage the existing thumbnail code, but found it impractical due to:

  1. The thumbnail view thumbnails are dependent on the directory model. If one opens Gwenview directly to View, the directory model isn't loaded before the image is shown
  2. The code is spread over multiple classes, there is no single place we can hook into.

Can we at least partly refactor and reuse some code? For example I noticed that compared to dragging thumbnails your code positions the drag pixmap differently and does not set a frame border. I assume we are both talking about DragPixmapGenerator?

  • Would it make sense (if technically possible) to support dragging and dropping the modified image like in D12871?

We can do this, but there's an issue. What do we set the drag pixmap to?

The pixmap should always represent what was dragged (and consequently only what was dragged should be dropped eventually).

Whether we set it to the original image or the edited image, there will be cases when the receiving application renders the wrong one.
E.g. user edits image > drag shows thumbnail of edited image > drops in GIMP > GIMP gets the unedited image.

I'm currently testing a patch which will enable copying the edited image to Firefox, LibreOffice, Inkscape, GIMP and Dolphin. Maybe we can do the same for drag'n'drop (unless the receiving application spoils the party), ideally converging on FileOpsContextManagerItem::selectionMimeData.


So we have some options:

  1. Generate a thumbnail from the existing views/adapters - happens on demand, but some duplication and videos only get an icon
  2. Use KIO::PreviewJob to create a thumbnail for every DocumentView - possible disk usage, wastes memory when the user doesn't drag (most of the time), but videos get a proper thumbnail
  3. Don't use the drag pixmap at all - avoids confusion regarding edited images

    Thoughts?

I don't like #3, and I'm undecided yet on #1, #2 or something different entirely. Let me think about it.

huoni added a comment.May 21 2018, 1:43 AM
  • Dragging is already implemented in Browse mode and for the Thumbnail Bar. Is there any reason you are (partly) reinventing the wheel regarding the thumbnails?

Had a good look into whether we could leverage the existing thumbnail code, but found it impractical due to:

  1. The thumbnail view thumbnails are dependent on the directory model. If one opens Gwenview directly to View, the directory model isn't loaded before the image is shown
  2. The code is spread over multiple classes, there is no single place we can hook into.

Can we at least partly refactor and reuse some code? For example I noticed that compared to dragging thumbnails your code positions the drag pixmap differently and does not set a frame border. I assume we are both talking about DragPixmapGenerator?

No, DragPixmapGenerator takes one or more existing pixmaps and generates a single pixmap for the drag, e.g. creating the 'fan' effect when dragging multiple items in Browse, and adding the border.
The problem we are facing is generating the thumbnail of the image/video in the first place. (We can use the generator to get the white border if that's preferable.)

  • Would it make sense (if technically possible) to support dragging and dropping the modified image like in D12871?

We can do this, but there's an issue. What do we set the drag pixmap to?

The pixmap should always represent what was dragged (and consequently only what was dragged should be dropped eventually).

Whether we set it to the original image or the edited image, there will be cases when the receiving application renders the wrong one.
E.g. user edits image > drag shows thumbnail of edited image > drops in GIMP > GIMP gets the unedited image.

I'm currently testing a patch which will enable copying the edited image to Firefox, LibreOffice, Inkscape, GIMP and Dolphin. Maybe we can do the same for drag'n'drop (unless the receiving application spoils the party), ideally converging on FileOpsContextManagerItem::selectionMimeData.

Yeah the recieving app is spoiling the party. If I set the image data just like the copy patch (mimeData->setImageData(doc->image())), GIMP still gets the original, so it must treat drops differently to pastes.

So we have some options:

  1. Generate a thumbnail from the existing views/adapters - happens on demand, but some duplication and videos only get an icon
  2. Use KIO::PreviewJob to create a thumbnail for every DocumentView - possible disk usage, wastes memory when the user doesn't drag (most of the time), but videos get a proper thumbnail
  3. Don't use the drag pixmap at all - avoids confusion regarding edited images

    Thoughts?

I don't like #3, and I'm undecided yet on #1, #2 or something different entirely. Let me think about it.

There's really two issues here:

  1. Generating the drag thumbnail - either stick with using the image views / adapters to render a thumbnail, or generate a new thumbnail using KIO::PreviewJob
  2. Whether to set the image data to the original or unedited image, and how receiving apps deal with that.

I am updating the revision so you can see what I mean.

huoni updated this revision to Diff 34558.May 21 2018, 1:45 AM
  • Use KIO::PreviewJob to generate pixmap thumbnail
  • Add (possibly edited) image data to the drag mimedata

This current revision shows that receiving apps may use the URL or the imagedata,
and it's confusing when the drag pixmap can only show one or the other.

huoni updated this revision to Diff 34559.May 21 2018, 1:48 AM
  • Remove include that I missed
rkflx added a comment.May 21 2018, 9:20 PM

I don't have good answers yet, but I'd like to add a couple of points I noticed:

  • We should be able to replicate the mime handling of my copying patch (D13028, based on D12871) for dropping too, i.e. return always exactly what is being dragged, so there won't be any ambiguities. In my tests this mostly worked, only Dolphin behaved strange for edited images (possibly needs patching in Dolphin, but not worse than the current state).
  • For now we can duplicate the mime handling, but ideally we'd refactor so that there aren't 3 code locations for this (View dragging, Thumbnail dragging, Copying). Perhaps a good first step would be to standardize mime handling between copying and thumbnail dropping, before resuming work on this patch (just an idea, though).
  • Styling should be done by DragPixmapGenerator, with the pixmap source still undecided. Please don't reinvent the wheel, this will hurt us later on (e.g. HiDPI patches). If you don't like the current look, improve it in that central place.
  • Dragging should respect the minimum dragging distance. This is automatic for ThumbnailView::startDrag, here (unless you see another way?) we have to do this by ourselves. Grep Spectacle for // drag handlers and see https://doc.qt.io/qt-5/dnd.html for what I mean.
  • Implementing the previous point and the drag starting in general is likely best done in AbstractImageView, where the drag handling for panning is already implemented. Or are there important reasons why this has to be done partly in DocumentView like you are proposing now?
  • updateCursor in hoverMoveEvent is called way too often, and looks suspicious.
  • Ignore my question about BirdEyeView, your code is fine.
  • Polishing, not strictly needed: Dragging the "Loading invalid.png failed" message should be disabled.
huoni planned changes to this revision.EditedMay 23 2018, 4:42 AM

Thanks for the feedback. I will wait until D13028 is finalised before doing anything. I also think waiting until we can put the mimedata code in a central place is a good idea. I think Document::mimeData could be a good spot?

Changes planned:

  • Use DragPixmapGenerator to keep consistent styling
  • Move event handling to AbstractImageView if possible
  • Figure out a better way to fix stuck cursor after dragging
  • Don't start drag if image failed to load
  • Respect minimum drag distance

I think Document::mimeData could be a good spot?

Not really, because Document only knows about a single document, while we also need to select a list of URLs. Essentially we (at least currently) depend on ContextManager in the implementation, but that's quite hard to get to in ThumbnailView as I found out today. Fortunately, I'm not out of ideas yet, it will only take me more time than planned…

I think Document::mimeData could be a good spot?

Not really, because Document only knows about a single document, while we also need to select a list of URLs. Essentially we (at least currently) depend on ContextManager in the implementation, but that's quite hard to get to in ThumbnailView as I found out today. Fortunately, I'm not out of ideas yet, it will only take me more time than planned…

But if we have multiple items selected, we are ignoring the actual image data. So I imagined something like the following.

QMimeData mimeData;
if (list.count() == 1) {
    // Get the document here if necessary
    mimeData = document->mimeData()
} else {
    mimeData->setUrls(list.urlList());
}

I also think waiting until we can put the mimedata code in a central place is a good idea. I think Document::mimeData could be a good spot?

Sneak peak: P225

@huoni I hope passing a KFileItemList will work for your case too?

Note that dropping modified images to Dolphin does not result in a saved file (if anyone feels like investigating that in KIO).

rkflx added a comment.May 31 2018, 7:10 PM

I also think waiting until we can put the mimedata code in a central place is a good idea. I think Document::mimeData could be a good spot?

Sneak peak: P225

@huoni I hope passing a KFileItemList will work for your case too?

For the record: I've now gone ahead and submitted the paste for review. See D13248 and D13249.

huoni updated this revision to Diff 35438.Jun 3 2018, 6:17 AM
huoni retitled this revision from Allow drag drop operation from View mode unless image pannable to Allow dragging from View mode to external applications.
huoni edited the summary of this revision. (Show Details)
huoni edited the test plan for this revision. (Show Details)
  • Don't drag if document failed to load
  • Update to use MimeTypeUtils::selectionMimeData
  • Respect mimimum drag distance
  • Use DragPixmapGenerator
huoni added a comment.Jun 3 2018, 6:20 AM

I kept everything in DocumentView because I wanted to support dragging a video.

There's really just one cosmetic issue I can't find a good solution for - setting the cursor back to 'open hand' after dragging. The problem is that AbstractImageView::updateCursor checks d->mLastDragPos to determine whether to set closed or open hand, and this is only reset to null if AbstractImageView gets a mouseReleaseEvent. AbstractImageView goes in to 'drag mode`, but is then interrupted by the drag handling in DocumentView.

Any ideas would be helpful. The ones I've come up with aren't great:

  1. Update cursor on hoverMove events - must move mouse to fix cursor, and lots of repeated calls to updateCursor
  2. Set cursor appropriately after dragging in DocumentView - seems like unnecessary logic to determine which cursor to set (different whether it's an image or video)
  3. Handle drags in AbstractImageView and forget about videos (can't drag to videos after all)

Unrelated to this patch, I noticed that dragging an edited image to Dolphin doesn't work. Doesn't matter whether from View or Browse, indicating an issue with MimeTypeUtils::selectionMimeData I believe.
Dolphin asks for a filename (with no suggested filename), and after entering one and clicking OK, nothing happens.

rkflx added a comment.Jun 3 2018, 6:28 AM

Thanks for the update ;)

Any ideas would be helpful.

I'll try to look into it later.


Unrelated to this patch, I noticed that dragging an edited image to Dolphin doesn't work. Doesn't matter whether from View or Browse, indicating an issue with MimeTypeUtils::selectionMimeData I believe.
Dolphin asks for a filename (with no suggested filename), and after entering one and clicking OK, nothing happens.

Yup, I mentioned that in D13249 as a known issue. Copying works, though, hinting at the problem being in KIO, somewhere here. Or somewhere else entirely…

huoni added a comment.Jun 3 2018, 10:57 AM

Thanks for the update ;)

Any ideas would be helpful.

I'll try to look into it later.

Take your time :)

Unrelated to this patch, I noticed that dragging an edited image to Dolphin doesn't work. Doesn't matter whether from View or Browse, indicating an issue with MimeTypeUtils::selectionMimeData I believe.
Dolphin asks for a filename (with no suggested filename), and after entering one and clicking OK, nothing happens.

Yup, I mentioned that in D13249 as a known issue. Copying works, though, hinting at the problem being in KIO, somewhere here. Or somewhere else entirely…

Oops, missed that.

rkflx added a comment.Jun 8 2018, 12:59 PM
  • Don't drag if document failed to load
  • Update to use MimeTypeUtils::selectionMimeData
  • Respect mimimum drag distance
  • Use DragPixmapGenerator

Can confirm that this works great now ;)

This leaves us with the cursor issue, where to get the pixmap from and what to do about videos. I'm more or less busy for the next days, I'll have to continue afterwards.


Just one note regarding KIO::PreviewJob: This is only a small part of ThumbnailProvider, which seems to contain lots of additional code for special cases. For example this can be observed for fish://, where your approach fails to provide a pixmap, while you'd get a proper thumbnail when dragging from the Thumbnail Bar. I'm sure there are many more cases.

Would it be possible to create a new ThumbnailProvider upon initiating the drag, connect its thumbnailLoaded signal to setPixmap, and then appendItems? That would be the next thing I would try, as it uses proven code and avoids creating a thumbnail when not starting to drag at all.


As for the cursor, so far I noticed that for the context menu it is also broken. Maybe this needs more work in general besides the specific problem in this patch… (Could not look at the code yet, though.)

lib/documentview/documentview.cpp
401–406

↑ Nothing to see here, keep reading ;)

huoni planned changes to this revision.Jun 8 2018, 11:06 PM

Just one note regarding KIO::PreviewJob: This is only a small part of ThumbnailProvider, which seems to contain lots of additional code for special cases. For example this can be observed for fish://, where your approach fails to provide a pixmap, while you'd get a proper thumbnail when dragging from the Thumbnail Bar. I'm sure there are many more cases.

Would it be possible to create a new ThumbnailProvider upon initiating the drag, connect its thumbnailLoaded signal to setPixmap, and then appendItems? That would be the next thing I would try, as it uses proven code and avoids creating a thumbnail when not starting to drag at all.

That sounds reasonable, I'll try it out.

As for the cursor, so far I noticed that for the context menu it is also broken. Maybe this needs more work in general besides the specific problem in this patch… (Could not look at the code yet, though.)

I think it would be a problem whenever a cursor change relies on a mouse release event, and blocking operations are possible (i.e. context menu and drag).

huoni updated this revision to Diff 35868.Jun 9 2018, 3:04 AM
huoni edited the summary of this revision. (Show Details)
  • Change to using ThumbnailProvider to generate drag pix
  • Fix stuck cursor after dragging an image
huoni updated this revision to Diff 35869.Jun 9 2018, 3:13 AM
  • Clean up KFileItemList code
huoni added a comment.Jun 9 2018, 3:18 AM

ThumbnailProvider worked nicely. I was initially put off by the complicated use in ThumbnailView which is why I didn't try it to begin with.

The cursor problem is kind of solved. There is still some flickering sometimes.
There's also another cursor oddity - when dragging from the image, the cursor goes to 'closed hand' during the drag, opposed to the arrow with green/red icon like when dragging a thumbnail from Browse or the Thumbnail Bar. I couldn't figure out why this is different, as the QDrag object should be practically identical. Dragging an edited image behaved differently still.

lib/documentview/documentview.cpp
374–376

Not the most elegant solution to the cursor issue, but it's not too bad.

390

Not entirely sure if this is necessary, but from what I've read, the object referenced isn't destroyed when the point changes.

437–443

I felt having a single ThumbnailProvider was better than repeatedly creating and destroying one. However, the downside is we always create one even if the user doesn't drag.

huoni updated this revision to Diff 35925.Jun 10 2018, 12:15 AM
huoni marked an inline comment as done.
  • Remove redundent comments
  • Improve variable naming, code readability
rkflx requested changes to this revision.Jun 19 2018, 10:25 PM

We are pretty close now, just some final polishing 👍

ThumbnailProvider worked nicely.

Glad to hear!

The cursor problem is kind of solved. There is still some flickering sometimes.

Nice. Let me look into this in the next days, then we can decide whether we can fix things now or simply open a bug to work on it later…

lib/documentview/documentview.cpp
390

Not sure how to trigger this, but probably makes sense. However, why not delete mDrag? I fear that deleteLater() might execute after you already set the new object.

393–395

Found a much more concise way to express this:

const auto itemList = KFileItemList({q->document()->url()});
437–443

Why not combine the advantages of both options, i.e. have a single object, and create it only on demand? :D

https://en.wikipedia.org/wiki/Lazy_initialization

881

const

891

const

945

Omitting the last two parameters also seems to work for me.

947

Wouldn't it be safer to check mDrag where it is accessed, i.e. in executeDrag() and in setDragPixmap?

951
removeItems(KFileItemList({item}))
956

(Here too.)

959
removeItems(KFileItemList({item}))
This revision now requires changes to proceed.Jun 19 2018, 10:25 PM

The cursor problem is kind of solved. There is still some flickering sometimes.

Nice. Let me look into this in the next days, then we can decide whether we can fix things now or simply open a bug to work on it later…

I identified 5 different cursor issues (and lots of unrelated deficiencies…), and I figured out how to fix 4 of them. I'll keep you posted.

lib/documentview/abstractimageview.h
108

Maybe resetDragCursor would be easier to understand and still accurate enough?

huoni updated this revision to Diff 36542.Jun 23 2018, 5:11 AM
huoni marked 8 inline comments as done.
  • const
  • better way of working with KFileItem/KFileItemList
  • lazy initialize thumbnail provider
  • check for mDrag closer to its usage
huoni updated this revision to Diff 36543.Jun 23 2018, 5:14 AM
  • resetDragDetection rename
huoni marked an inline comment as done.Jun 23 2018, 5:15 AM

The cursor problem is kind of solved. There is still some flickering sometimes.

Nice. Let me look into this in the next days, then we can decide whether we can fix things now or simply open a bug to work on it later…

I identified 5 different cursor issues (and lots of unrelated deficiencies…), and I figured out how to fix 4 of them. I'll keep you posted.

Well done, only one to go.

lib/documentview/documentview.cpp
390

delete is blocking I believe, I didn't want to delay the drag. I don't think deleteLater is a problem with QPointer, the new drag object will replace the old one, and the old one will get deleted, ...later.

393–395

Nice one.

rkflx accepted this revision.Jun 23 2018, 10:12 PM

Final polishing LGTM. I'll submit the cursor fixes in separate Diffs, don't worry about it ;) Thanks again for the patch, and sorry it took so long to review…

I identified 5 different cursor issues (and lots of unrelated deficiencies…), and I figured out how to fix 4 of them. I'll keep you posted.

Well done, only one to go.

Haha, I actually meant that I was going to add more details and possibly Diffs for the issues later on. I don't think the fifth issue is that easy to solve… Woo hoo, I solved it!

lib/documentview/documentview.cpp
390

Ah, of course, I think I got confused by the (possibly incorrect) assumption that there can only be a single drag object at any point in time.

deleteLater is probably fine here, although it would be interesting to measure whether posting to the event loop is really faster than simply calling delete (not that it would make any difference to the user here).

This revision is now accepted and ready to land.Jun 23 2018, 10:12 PM
This revision was automatically updated to reflect the committed changes.
huoni added a comment.Jun 24 2018, 1:28 AM

Final polishing LGTM. I'll submit the cursor fixes in separate Diffs, don't worry about it ;) Thanks again for the patch, and sorry it took so long to review…

No problem at all, I'm pretty busy these days. This will probably be my last patch for the foreseeable future :(

I identified 5 different cursor issues (and lots of unrelated deficiencies…), and I figured out how to fix 4 of them. I'll keep you posted.

Well done, only one to go.

Haha, I actually meant that I was going to add more details and possibly Diffs for the issues later on. I don't think the fifth issue is that easy to solve… Woo hoo, I solved it!

Awesome!

rkflx added a comment.Jun 25 2018, 9:48 PM

Finally got around to summarize the cursor issues:

  1. When moving very slowly while initiating a drag, the ForbiddenCursor would appear briefly until you moved the mouse further along. I think this is an issue in Qt, as we can observe the same behaviour in Dolphin. In QBasicDrag::drag, DragCopyCursor is set, then QBasicDrag::updateCursor changes the cursor to ForbiddenCursor, until finally QBasicDrag::updateCursor updates to the final cursor, e.g. DragCopyCursor. We can avoid the flickering by always displaying ForbiddenCursor (see next point).
  1. https://bugs.kde.org/show_bug.cgi?id=386034 proposed that "the image should not be able to be dropped onto itself.", which your patch still allows. As that's actually more related to 131d25855e11, I'll simply fix this in a separate Diff, see D13724.
  1. ClosedHandCursor shown when trying to drop to Dolphin: Turns out that's the correct cursor for the default case of moving objects, which is set implicitly by QGraphicsView. For dragging from the ThumbnailView it works fine, because there copying is set as the default action. That's where there actually is a difference to what your patch is implementing ;) In D13725 I'm doing some further refinements to the dragging of thumbnails, and replicate that change for dragging from the canvas too, which should give us the more pleasant DragCopyCursor.
  1. Not strictly related, but as I mentioned it above: The spurious ClosedHandCursor after showing the context menu will be fixed with D13723.
  1. Slowly starting to drag so the ClosedHandCursor shows, the ClosedHandCursor is shown after the drag. Again, here I believe the issue might be in Qt. qDebug() shows that the cursor is correctly (re)set, but somehow the change takes effect only after moving the mouse again. Workaround which seems to fix the problem in D13726.
huoni added a comment.Jul 8 2018, 12:47 AM

Finally got around to summarize the cursor issues:

  1. When moving very slowly while initiating a drag, the ForbiddenCursor would appear briefly until you moved the mouse further along. I think this is an issue in Qt, as we can observe the same behaviour in Dolphin. In QBasicDrag::drag, DragCopyCursor is set, then QBasicDrag::updateCursor changes the cursor to ForbiddenCursor, until finally QBasicDrag::updateCursor updates to the final cursor, e.g. DragCopyCursor. We can avoid the flickering by always displaying ForbiddenCursor (see next point).
  2. https://bugs.kde.org/show_bug.cgi?id=386034 proposed that "the image should not be able to be dropped onto itself.", which your patch still allows. As that's actually more related to 131d25855e11, I'll simply fix this in a separate Diff, see D13724.
  3. ClosedHandCursor shown when trying to drop to Dolphin: Turns out that's the correct cursor for the default case of moving objects, which is set implicitly by QGraphicsView. For dragging from the ThumbnailView it works fine, because there copying is set as the default action. That's where there actually is a difference to what your patch is implementing ;) In D13725 I'm doing some further refinements to the dragging of thumbnails, and replicate that change for dragging from the canvas too, which should give us the more pleasant DragCopyCursor.
  4. Not strictly related, but as I mentioned it above: The spurious ClosedHandCursor after showing the context menu will be fixed with D13723.
  5. Slowly starting to drag so the ClosedHandCursor shows, the ClosedHandCursor is shown after the drag. Again, here I believe the issue might be in Qt. qDebug() shows that the cursor is correctly (re)set, but somehow the change takes effect only after moving the mouse again. Workaround which seems to fix the problem in D13726.

Sorry for taking so long to review.

I think all these changes significantly improve the perception of quality, good detective work!