Note: Recent Files does not show the modified version of the thumbnail, although dragging is fine (should anyone want to investigate).

Thanks for the quick review!

Sadly nothing we can do about the NEF example, as we are required to set the URL to make dropping to Dolphin work as expected. We simply cannot know in advance which app we'll be dropped onto, and Inkscape seems to prefer the URL over the pixmap (ideally it should try both). OTOH, for copying it's the other way round…

Gwenview, we have a problem:

While triaging https://bugs.kde.org/show_bug.cgi?id=396974, I noticed that dropping an image to the (Plasma) desktop does not really work anymore. The drag cursor becomes really laggy and turns into the green accepting drop cursor only after hovering for a while, and after releasing the mouse button the context menu takes several seconds to show up. For normal usage, this will seem to be totally broken, with no menu showing up at all.

Can anybody confirm? If this is really an issue, obviously it won't be acceptable to ship Gwenview in such a state for 18.08, since setting the Plasma wallpaper by dragging to the desktop is a feature often asked for.

Findings while digging deeper:

• Does not happen on Wayland.
• Dragging to Dolphin is fine, only Plasma is affected.
• Pasting of unmodified and modified images (which sets the same mimetypes as dragging unmodified or modified images) works fine, only dragging is affected.
• Setting the raw image data seems to be what triggers the problem, which is done in mimeData->setImageData(doc->image());.
• Only after the long wait is over, plasmashell will print Arrived mimeData, which is coming from ContainmentInterface::processMimeData, so this cannot be the issue.
• On the gwenview side, begin enumerating printers is printed, which comes from a custom openSUSE patch, so somehow we end up in a code path enumerating printers via CUPS, which can be painfully slow.
• Getting a backtrace from plasmashell while interrupting it during the wait, I eventually traced the issue down to DeclarativeMimeData::DeclarativeMimeData:
  • In particular copy->data(format) takes noticable time for each of the many entries we get through application/x-qt-image set by Gwenview's code. Adding qDebug, they appear one-by-one in the log, with even longer waits for the EPS based formats (related to the CUPS hint from above?). For this to behave well, all formats combined should be processed in a fraction of a second.
  • This seems to actually trigger a full conversion to the formats requested (confirmed by calling size()), which is quite expensive, instead of only passing around mimetypes and deferring the conversion to a single format to the final receiver of the data.
  • The constructor is not only called when dropping, but during moving the mouse too, so it gets called multiple times per second, cannot keep up and gets everything stuck in the end.

What's left to figure out is:

• How to fix the issue in kdeclarative? Or report a bug and hope for the best?
• How to work around the problem in Gwenview for now? Ideas:
  • Can we only return image/png instead of what we get from Qt?
  • We might be able to revert the patch for ThumbnailView::startDrag (i.e. the current one), but how to handle DocumentViewPrivate::startDragIfSensible?
  • Perhaps add a flag to MimeTypeUtils::selectionMimeData, to only disable it for call sites handling dragging?

Any other ideas, any offers for help?

In D13249#300894, @rkflx wrote:

Can anybody confirm? If this is really an issue, obviously it won't be acceptable to ship Gwenview in such a state for 18.08, since setting the Plasma wallpaper by dragging to the desktop is a feature often asked for.

I can confirm this in Kubuntu 18.04, it behaves exactly as described, no matter if Desktop- or Folder-View is active.
In some cases Gwenview crashes after some seconds of hovering (roughly the time where the cursor otherwise changes to accept) with the console message The X11 connection broke (error 4). Did the X11 server die?.

I'll take a look at the imagedata setting. Setting one special mime type may help as first workaround.

In D13249#301085, @muhlenpfordt wrote:

I'll take a look at the imagedata setting. Setting one special mime type may help as first workaround.

It doesn't work for modified images, e.g. dolphin accepts the drop but does nothing more. Furthermore the conversion from QImage to PNG-QByteArray is very slow.
I have no other idea at the moment than disabling setImageData for drags until we find a good fix. Maybe also disabling dragging of modified images to prevent confusion?

Btw. Spectacle seems to have a similiar issue when dragging the screenshot preview to the desktop.

In D13249#300894, @rkflx wrote:

• How to fix the issue in kdeclarative? Or report a bug and hope for the best?

Just an untested idea to fix the all-formats-copy in DeclarativeMimeData...
Before copying all types there could be a check with hasImage() and in case it exists a setImageData() for the new QMimeData copy.
After that all remaining (not Qt-generated) mimetypes could be copied.

if (copy->hasImage()) {
    setImageData(copy->imageData());
}
foreach(QString format, copy->formats()) {
    if (!hasFormat(format)) {
        QMimeData::setData(format, copy->data(format));
    }
}

Thanks for investigating!

In some cases Gwenview crashes

This seems to be related to sending large amounts of data, for example sometimes I get this when dragging a large modified image to GIMP:

The X11 connection broke: Maximum allowed requested length exceeded (code 4)

We might have to research what's the maximum allowed size, and add a check to Gwenview. (Would you like to work on that? Not really urgent, though.)

dolphin accepts the drop but does nothing more

This is also broken with current master (see test plan of the Diff).

conversion from QImage to PNG-QByteArray is very slow

Too bad! The conversion is probably also done in advance, which is not really what we want. (It might still be possible, though, perhaps by setting the mimetype, and providing a hook to create the data only on demand like Qt is doing for application/x-qt-image, but we might have to tinker with X directly, so probably not something we should do.)

Spectacle seems to have a similiar issue when dragging the screenshot preview to the desktop.

That's interesting, thanks for the hint! I assume you are running Spectacle 17.12, but since Spectacle 18.04 there is D10427, which actually removes setImageData to make it work for Chromium, and apparently also fixes the issue you are reporting when dragging to the desktop. At the same time, this means here we broke dragging from Gwenview to Chromium (which I can confirm with https://fineuploader.com/demos.html).

IIRC the main motivation for setting the pixmap data was to make copying to GIMP work, and dragging to GIMP worked already without the pixmap data. Therefore we might be able to get away with not setting pixmap data for dragging unmodified images.

For dragging modified images, it's perhaps still desirable to set pixmap data:

• It works in a bunch of applications (e.g. GIMP, but as we know it's broken for Dolphin), which otherwise would get the unmodified image or no image at all.
• I think the previous point is more important than broken dragging of modified images to the desktop or to Chromium. (Who would do that? Also, we could refer to the bug in KDeclarative or to Chromium if there are complaints).
• We can avoid adding extra code on the UI side to disable dragging if the image is modified, and keep everything contained in selectionMimeData.

Does that make sense? If so, I'll work on a patch on Wednesday (disabling setImageData for dragging of unmodified images), to hopefully make the RC on Thursday.

Just an untested idea to fix the all-formats-copy in DeclarativeMimeData...

That's quite elegant, I did not think of that! However, if (!hasFormat(format)) won't work as such. The challenge is to figure out what list of formats application/x-qt-image will result in, and leave everything else intact. I vaguely remember there being an API for determining the supported image formats when I looked into the Qt drag-and-drop sources for the other Diffs.

I have not fully understood the purpose of the code in the constructor yet or whether "we have troubles passing const to QML" still applies today (so maybe the default compiler-generated copy constructor could be used instead), but we should definitely consider proposing a change similar to that to Frameworks. (This would then fix the remaining case, i.e. dragging modified images to the desktop.)

In D13249#301464, @rkflx wrote:

We might have to research what's the maximum allowed size, and add a check to Gwenview. (Would you like to work on that? Not really urgent, though.)

Ok, I take a look at this.

Spectacle seems to have a similiar issue when dragging the screenshot preview to the desktop.

That's interesting, thanks for the hint! I assume you are running Spectacle 17.12, but since Spectacle 18.04 there is D10427, which actually removes setImageData to make it work for Chromium, and apparently also fixes the issue you are reporting when dragging to the desktop.

Correct - I used 17.12.3. Tested with 18.07.80 in a Neon VM does not show this problem (but drop on desktop does nothing).

For dragging modified images, it's perhaps still desirable to set pixmap data:

• It works in a bunch of applications (e.g. GIMP, but as we know it's broken for Dolphin), which otherwise would get the unmodified image or no image at all.
• I think the previous point is more important than broken dragging of modified images to the desktop or to Chromium. (Who would do that? Also, we could refer to the bug in KDeclarative or to Chromium if there are complaints).
• We can avoid adding extra code on the UI side to disable dragging if the image is modified, and keep everything contained in selectionMimeData.
  
  Does that make sense? If so, I'll work on a patch on Wednesday (disabling setImageData for dragging of unmodified images), to hopefully make the RC on Thursday.

I think this is ok. For unmodified images there is no mandatory reason to transfer the whole image data.

Just an untested idea to fix the all-formats-copy in DeclarativeMimeData...

That's quite elegant, I did not think of that! However, if (!hasFormat(format)) won't work as such. The challenge is to figure out what list of formats application/x-qt-image will result in, and leave everything else intact. I vaguely remember there being an API for determining the supported image formats when I looked into the Qt drag-and-drop sources for the other Diffs.

I tested this by completely disabling the QMimeData::setData() call and therefore copying the data here. The result is not really better. :(
The problem seems to be anywhere else in the dragging part.

In D13249#301827, @muhlenpfordt wrote:

Spectacle seems to have a similiar issue when dragging the screenshot preview to the desktop.

That's interesting, thanks for the hint! I assume you are running Spectacle 17.12, but since Spectacle 18.04 there is D10427, which actually removes setImageData to make it work for Chromium, and apparently also fixes the issue you are reporting when dragging to the desktop.

Correct - I used 17.12.3. Tested with 18.07.80 in a Neon VM does not show this problem (but drop on desktop does nothing).

Hm, works for me. Are you sure you ran the unmodified plasmashell, and dragged to a free space on the desktop? If this is really broken, please file a bug.

I tested this by completely disabling the QMimeData::setData() call and therefore copying the data here. The result is not really better. :(
The problem seems to be anywhere else in the dragging part.

Did you add some qDebug() and watch the output of plasmashell? For me, after disabling setData and only adding setImageData the delay before we reach ContainmentInterface::processMimeData is gone (or at least it is noticeably shorter, because only the image data is copied, but no conversion for each and every format takes place – ideally we would not copy anything).

This won't work yet to get the context menu to show, because it looks for the URL, which we did not pass. IOW, now the reason is missing data, but we don't overload the system anymore.

In D13249#301967, @rkflx wrote:

Correct - I used 17.12.3. Tested with 18.07.80 in a Neon VM does not show this problem (but drop on desktop does nothing).

Hm, works for me. Are you sure you ran the unmodified plasmashell, and dragged to a free space on the desktop? If this is really broken, please file a bug.

Ok, my own trap... I had the no-setData version installed. Dropping from Spectacle works fine.

Did you add some qDebug() and watch the output of plasmashell? For me, after disabling setData and only adding setImageData the delay before we reach ContainmentInterface::processMimeData is gone (or at least it is noticeably shorter, because only the image data is copied, but no conversion for each and every format takes place – ideally we would not copy anything).

I added a qWarning() to the DeclarativeMimeData constructor and checked ~/.xsession-errors to see this code is really called.
Checked these two cases now:

• added setImageData() but disabled setData() loop: no speed difference for Gwenview (stuttering)
• no setImageData() and no setData() loop: dragging is fluently