518249 - Allow cmd_copyImage to copy all three representations of the image

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[[e-mail]]

+++ This bug was initially created as a clone of Bug #21747 +++

Prior to bug 21747, cmd_copyImage copied the URL of the image as text (which could be pasted into any text field) and the HTML source of the image (which could be pasted into an HTML editor, such as SeaMonkey Composer or a new Thunderbird HTML message). However bug 21747 changed the action of cmd_copyImage to only copy the pixbuf of the image. It was therefore necessary to add an extra menuitem to allow the URL of the image to be copied (bug 469481), and the ability to copy the HTML source of the image was lost.

Note that on (say) Windows, all three formats are copied simultaneously.

Comment 1

[[e-mail]]

Attachment: Possible patch

* Instead of special-casing images, simply advertise PNG and JPEG formats
* Only when those formats are requested, call gtk_selection_data_set_pixbuf

With this patch, I can use the "Copy Image" menuitem in SeaMonkey and paste in to Leafpad, Composer and the GIMP at the same time.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 402233 [details] [diff] [review]
Possible patch

Only creating the pixbuf when requested is good.

We can still support all GTK's image format targets though and should continue
to do so, as it sounded on irc like you are hoping to do.

In nsClipboard::SetData(), this is something like
(mostly copied from GTK code):

  GtkTargetList *list;
  GtkTargetEntry *targets;
  gint n_targets;

  list = gtk_target_list_new (NULL, 0);
  gtk_target_list_add_image_targets (list, 0, TRUE);
  targets = gtk_target_table_new_from_list (list, &n_targets);

  gtk_selection_add_targets(mWidget, selectionAtom, targets, n_targets);

  gtk_target_table_free (targets, n_targets);
  gtk_target_list_unref (list);

In nsClipboard::SelectionGetEvent(), you can test whether the requested target
is an image type with

  gtk_targets_include_image(&aSelectionData->target, 1, TRUE);

(The return code of gtk_selection_data_set_pixbuf would also tell you but
we don't really want to create the GdkPixbuf unless requested.)

Comment 3

[[e-mail]]

Attachment: Proposed patch

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 402313 [details] [diff] [review]
Proposed patch

>             if (!strcmp(flavorStr, kNativeImageMime) || !strcmp(flavorStr, kPNGImageMime) ||
>                 !strcmp(flavorStr, kJPEGImageMime) || !strcmp(flavorStr, kGIFImageMime)) {
>+                // Accept any writable image type
>+                GtkTargetList *list = gtk_target_list_new(NULL, 0);
>+                gtk_target_list_add_image_targets(list, 0, TRUE);
>+                gint n_targets;
>+                GtkTargetEntry *targets = gtk_target_table_new_from_list(list, &n_targets);
>+                gtk_selection_add_targets(mWidget, selectionAtom, targets, n_targets);

Would be nice to only add these targets once even if all 4 flavors are supported by aTransferable.
A boolean variable maybe.

>+        rv = trans->GetTransferData(kNativeImageMime, getter_AddRefs(item), &len);
>+        nsCOMPtr<nsISupportsInterfacePointer> ptrPrimitive(do_QueryInterface(item));
>+        if (!ptrPrimitive)
>+            return;
>+
>+        nsCOMPtr<nsISupports> primitiveData;
>+        ptrPrimitive->GetData(getter_AddRefs(primitiveData));
>+        nsCOMPtr<imgIContainer> image(do_QueryInterface(primitiveData));
>+        if (!image) // Not getting an image for an image mime type!?
>+            return;
>+
>+        GdkPixbuf* pixbuf = nsImageToPixbuf::ImageToPixbuf(image);
>+        if (!pixbuf)
>+            return;

Shouldn't this be tried for all 4 flavors tested in SetData()?

Or does this only work for kNativeImageMime anyway?
If so, GetData() should only add all the pixbuf targets if kNativeImageMime is supported,
and SelectionGetEvent() should fallback to the general CreateDataFromPrimitive if the transferable doesn't support kNativeImageMime.

Comment 5

[[e-mail]]

Attachment: Only add image targets once

I don't know why the code requests kNativeImageMime, that's just cut'n'paste.

Comment 6

[[e-mail]]

Attachment: Addressed review comments

As per discussion on IRC, although internally we only ever copy native images to the clipboard the other platforms support using other image MIME types, so now I loop through the supported types looking for the image data.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 402558 [details] [diff] [review]
Addressed review comments

Thanks, can you just rename n_targets to something more image specific, please?  n_image_targets or nImageTargets, maybe.

Comment 8

[[e-mail]]

Attachment: With variable renamed

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 402565 [details] [diff] [review]
With variable renamed

>+        // Look through our transfer data for the image
>+        static const char *imageMimeTypes[] = {
>+            kNativeImageMime, kPNGImageMime, kJPEGImageMime, kGIFImageMime };
>+        nsCOMPtr<nsISupports> item;
>+        PRUint32 len;
>+        nsCOMPtr<nsISupportsInterfacePointer> ptrPrimitive;
>+        for (PRUint32 i = 0; !ptrPrimitive && i < NS_ARRAY_LENGTH(imageMimeTypes); i++) {
>+            rv = trans->GetTransferData(kNativeImageMime, getter_AddRefs(item), &len);
>+            ptrPrimitive = do_QueryInterface(item);
>+        }

Sorry, I should have spotted this before: I think you mean "GetTransferData(imageMimeTypes[i],"

Comment 10

[[e-mail]]

Attachment: Actually using the array

I also noticed that I was a "const" short on my array's declaration.

Comment 11

[[e-mail]]

Pushed changeset 8398d82dba41 to mozilla-central.

Comment 12

[[e-mail]]

Comment on attachment 402704 [details] [diff] [review]
Actually using the array

This patch provides two benefits a) defers generation of pixel data until clipboard actually pasted b) fixes regression in cmd_copyImage.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 402704 [details] [diff] [review]
Actually using the array

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

Comment 15

[[e-mail]]

Comment on attachment 402704 [details] [diff] [review]
Actually using the array

This patch provides two benefits; it defers generation of pixel data until the clipboard actually pasted into a graphics editor, and it fixes the regression in cmd_copyImage caused by bug 21747, which SeaMonkey is currently having to hack around (at least when built against Gecko 1.9.1).

Comment 16

[[e-mail]]

Comment on attachment 402704 [details] [diff] [review]
Actually using the array

Pushing approval request for this regression fix to the next branch release.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 402704 [details] [diff] [review]
Actually using the array

Moving approval flag - what's the testing coverage like, here?

Comment 18

[[e-mail]]

A full test requires access to external editors that accept the appropriate clipboard formats. But I could possibly cons up a test that would at least verify that all the clipboard formats are being advertised.

Comment 19

[[e-mail]]

Attachment: Basic test

This test fails on Linux branch builds as they still have the bug, but it passes everywhere else.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 432334 [details] [diff] [review]
Basic test

Thanks.  (I didn't know about hg copy.)

Comment 21

[[e-mail]]

Comment on attachment 432334 [details] [diff] [review]
Basic test

Pushed changeset befd1c73e3f1 to mozilla-central.

Comment 22

[[e-mail]]

Attachment: Branch patch including test

The trunk test doesn't apply cleanly to the branch, so I regenerated the patch.

Comment 23

[(no longer active)]

(In reply to comment #21)
> (From update of attachment 432334 [details] [diff] [review])
> Pushed changeset befd1c73e3f1 to mozilla-central.

The test seems to be consistently crashing on Mac:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268660676.1268662202.16721.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268659343.1268660578.12400.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268658101.1268658643.6421.gz

Comment 24

[(no longer active)]

Test was disabled on Mac: http://hg.mozilla.org/mozilla-central/rev/2af2ea97801b

I think we need to file a bug about this and track the fix there, though.

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to comment #24)
> Test was disabled on Mac:
> http://hg.mozilla.org/mozilla-central/rev/2af2ea97801b
> 
> I think we need to file a bug about this and track the fix there, though.

Bug 552449

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 432535 [details] [diff] [review]
Branch patch including test

Please renominate when you get the crashing-test issue worked out; we'll take it then!