I'm not sure about the sidebar position of the menu entry. Technically it's more an image than a file operation, but where would a user expect to find this option?
Should we add a default shortcut (e.g. Ctrl+⇧+C)?

Nice feature! I would approve of adding a shortcut for this, yeah.

However, I wonder what the balance of usage for these two copy styles is. Is it roughly 50/50, or do people want to copy the path or the image far more frequently than the other. I'm guessing that copying the image itself is maybe more likely given Gwenview's use cases, so perhaps we should give the regular ctrl+C shortcut to it? Or perhaps we could have a single Copy menu item, with the default behavior (path vs image) being selectable in Settings, and temporarily overridable by holding down the shift key, with a little label indicating this in the settings window? Just some thoughts, feel free to ignore if none of them seem sane and sensible.

Do we really need this as a separate action? IIRC the regular Edit → Copy action already works for pasting images to LibreOffice and Dolphin. If pasting to GIMP does not work, maybe the regular action could be changed to also support that, i.e. by advertising multiple mimetypes? Or maybe this is a bug in GIMP?

Are there any other apps around which provide two separate actions?

In D12871#262258, @rkflx wrote:

Do we really need this as a separate action? IIRC the regular Edit → Copy action already works for pasting images to LibreOffice and Dolphin.

LibreOffice and Dolphin parse the url string(s) and load/copy the file(s). This does not work for e.g. pasting to a browser form. The bitmap copy could also contain a resized/edited but not saved version of the image.

If pasting to GIMP does not work, maybe the regular action could be changed to also support that, i.e. by advertising multiple mimetypes?

What should we do if multiple images are selected? Copy all urls + the bitmap of the current one? Only the urls in this case?

Are there any other apps around which provide two separate actions?

I found none supporting both copy styles.

In D12871#262255, @ngraham wrote:

However, I wonder what the balance of usage for these two copy styles is. Is it roughly 50/50, or do people want to copy the path or the image far more frequently than the other. I'm guessing that copying the image itself is maybe more likely given Gwenview's use cases, so perhaps we should give the regular ctrl+C shortcut to it? Or perhaps we could have a single Copy menu item, with the default behavior (path vs image) being selectable in Settings, and temporarily overridable by holding down the shift key, with a little label indicating this in the settings window? Just some thoughts, feel free to ignore if none of them seem sane and sensible.

If we find a good way to handle the single/multiple selection I would also prefer using just one action.

I think we should change the current Copy action to copy the bitmap, but only in View mode (if we're in compare mode, then only the selected image). There is no ambiguity with multiple selection here.
I think when viewing a single image, Copy is naturally associated with copying the bitmap (not the file), so one can paste in a browser or image editor. But in Browse, the images are more like files, so copying there should behave more like a file (just like dragging and dropping).

It's worth noting that one problem this solves (copying image to a separate editor for example) is partially solved by D11877: Allow dragging from View mode to external applications (which FYI does not setImageData, but still works when dragging to Chromium and GIMP - exactly how Spectacle works). Of course, I think both dragging and copying should be implemented.

In D12871#262544, @huoni wrote:

I think we should change the current Copy action to copy the bitmap, but only in View mode (if we're in compare mode, then only the selected image). There is no ambiguity with multiple selection here.

That's a good idea. If there's no veto I'll do it this way.

which FYI does not setImageData, but still works when dragging to Chromium and GIMP - exactly how Spectacle works).

Using the url without setting the bitmap only works if the destination app accesses the file.
If the image was e.g. resized but not saved and then copied/dragged the original version of the image would appear.
Or am I wrong?

In D12871#262626, @muhlenpfordt wrote:

Or am I wrong?

Doubt it :)

Doesn't matter anyway, if we want the ability to copy an edited and unsaved image, then we need to set the image data, so ignore this.

All except LibreOffice Writer worked as expected (pasted the bitmap). Writer presumably used the URL since the original image was pasted.
It might be confusing in some situations, but I don't see how we could solve the issue without removing the URL from the mime data, which is a bad idea.

So all in all, this LGTM.

In D12871#263254, @huoni wrote:

All except LibreOffice Writer worked as expected (pasted the bitmap). Writer presumably used the URL since the original image was pasted.

Yes, LO prefers the text of the clipboard. You have to explicitly paste the bitmap with Edit → Paste Special....

Much better than before ;) I tested everything and could not find anything wrong, yet I wonder whether the patch is complete (or perhaps needs a follow-up):

• Is there any reason we don't do the same for copying a single image in Browse mode? Essentially we are working around a missing feature in GIMP and Inkscape (i.e. pasting from a URL), so we might as well apply the same workaround (or feature in case of modifications) there? After all, in Browse mode the modified image is displayed too (thumbnail and save icon). For the multi-selection case we could fall back to selectionMimeData().

• Also, for Cut I'm able to paste into LibreOffice, while I still cannot do the same for GIMP. Comparing the original cut and copy, the only difference seems to be in the second parameter of KIO::setClipboardDataCut, so we might as well be consistent here (i.e. refactor a bit).

• If I'm not mistaken, QMimeData supports arbitrary mimetypes, so maybe it would be possible to pass SVGs through the clipboard too?

• We might think about later adding Paste and Copy to the context menu for both view modes where appropriate.

Code LGTM, only one inline question.

In D12871#262544, @huoni wrote:

D11877 (which FYI does not setImageData, but still works when dragging to Chromium and GIMP - exactly how Spectacle works).

Spectacle and your patch use setPixmap, which is not all that different from setImage (which internally calls setImageData), is it ;)

In D12871#264329, @rkflx wrote:

In D12871#262544, @huoni wrote:

D11877 (which FYI does not setImageData, but still works when dragging to Chromium and GIMP - exactly how Spectacle works).

Spectacle and your patch use setPixmap, which is not all that different from setImage (which internally calls setImageData), is it ;)

If that's the case, then Qt documentation doesn't say it. setPixmap is a member of QDrag, whereas setImageData is a member of QMimeData. I'm not sure where setImage is? (I'm ready to be proven blind :)
I understood QDrag::setPixmap to simply set the thumbnail image that sticks to the cursor during the drag. This is what the documentation seems to suggest: "Sets pixmap as the pixmap used to represent the data in a drag and drop operation" (emphasis mine).

In D12871#264329, @rkflx wrote:

• Is there any reason we don't do the same for copying a single image in Browse mode? Essentially we are working around a missing feature in GIMP and Inkscape (i.e. pasting from a URL), so we might as well apply the same workaround (or feature in case of modifications) there? After all, in Browse mode the modified image is displayed too (thumbnail and save icon). For the multi-selection case we could fall back to selectionMimeData().

First I worried this could be a little confusing. But I just tried it and think it's useful this way. :)

• Also, for Cut I'm able to paste into LibreOffice, while I still cannot do the same for GIMP. Comparing the original cut and copy, the only difference seems to be in the second parameter of KIO::setClipboardDataCut, so we might as well be consistent here (i.e. refactor a bit).

Since the deletion of the source file seems to be the destination apps job this should be save. I found only file managers like dolphin/krusader doing a delete. Cutting and pasting to e.g. gimp does not touch the source file.

In D12871#264417, @huoni wrote:

In D12871#264329, @rkflx wrote:

Spectacle and your patch use setPixmap, which is not all that different from setImage (which internally calls setImageData), is it ;)

If that's the case, then Qt documentation doesn't say it. setPixmap is a member of QDrag, whereas setImageData is a member of QMimeData. I'm not sure where setImage is? (I'm ready to be proven blind :)

There's also a QClipboard::setPixmap, but I don't know if there's a difference to setting the image by QMimeData?

@muhlenpfordt Thanks for the quick update, I'll test it out later ;)

In D12871#264417, @huoni wrote:

In D12871#264329, @rkflx wrote:

In D12871#262544, @huoni wrote:

D11877 (which FYI does not setImageData, but still works when dragging to Chromium and GIMP - exactly how Spectacle works).

Spectacle and your patch use setPixmap, which is not all that different from setImage (which internally calls setImageData), is it ;)

If that's the case, then Qt documentation doesn't say it. setPixmap is a member of QDrag, whereas setImageData is a member of QMimeData. I'm not sure where setImage is? (I'm ready to be proven blind :)
I understood QDrag::setPixmap to simply set the thumbnail image that sticks to the cursor during the drag. This is what the documentation seems to suggest: "Sets pixmap as the pixmap used to represent the data in a drag and drop operation" (emphasis mine).

Ah, we are talking about different setPixmaps (for Spectacle), and I was wrong about your patch. I read the docs this way:

• QClipboard::setPixmap: "Note that this is slower than setImage() because it needs to convert the QPixmap to a QImage first."
• QClipboard::setImage: "This is shorthand for:"

QMimeData *data = new QMimeData;
data->setImageData(image);
clipboard->setMimeData(data, mode);

• QMimeData::setImageData is used by Gwenview, and Spectacle is doing QApplication::clipboard()->setPixmap in ExportManager::doCopyToClipboard, therefore doing essentially the same.

As for the drag-and-drop handling in both Spectacle and D11877, that seems to use QDrag::setPixmap which is about the preview image attached to the cursor, but not about the actual data (which is still the URL). The reasons dropping to GIMP or Inkscape works is because both apps implement dropping of URLs, but not pasting of URLs, which can be seen when testing dropping vs. pasting from Dolphin.

Maybe it would make sense to change Dolphin too, to support the "paste instead of drag" workflow? OTOH, it might make more sense to fix the receiving apps.

Yay, pasting SVGs into Inkscape works great, as well as cutting and copying from Browse.

Would it be easy to special-case comparing images in View mode, e.g. via mThumbnailView->isVisible(), and then use contextManager()->currentUrl()? There, copying the pixmap of the focused image would probably be what users are expecting. If not, that would also be okay.

In D12871#264875, @rkflx wrote:

Would it be easy to special-case comparing images in View mode, e.g. via mThumbnailView->isVisible(), and then use contextManager()->currentUrl()? There, copying the pixmap of the focused image would probably be what users are expecting. If not, that would also be okay.

Uh, I forgot about the compare mode. It's no problem to handle this (as in a previous version of the diff).

In D12871#265075, @rkflx wrote:

Hm, it's not working very reliably in Browse mode, because depending on what you've done before, the focus (i.e. currentUrl()) might not correspond to the first (or even any) selected image. It's working great for Compare, though.

Right - e.g. using the hover +/- does not update the current url.