Allow cmd_copyImage to copy all three representations of the image

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla1.9.3a1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

10 years ago
+++ 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.
(Assignee)

Comment 1

10 years ago
Posted patch Possible patch (obsolete) — Splinter Review
* 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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #402233 - Flags: review?(mozbugz)
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.)
Attachment #402233 - Flags: review?(mozbugz)
(Assignee)

Comment 3

10 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
Attachment #402233 - Attachment is obsolete: true
Attachment #402313 - Flags: review?(mozbugz)
Attachment #402313 - Flags: review?(mozbugz)
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.
(Assignee)

Comment 5

10 years ago
Posted patch Only add image targets once (obsolete) — Splinter Review
I don't know why the code requests kNativeImageMime, that's just cut'n'paste.
Attachment #402313 - Attachment is obsolete: true
Attachment #402545 - Flags: review?(mozbugz)
(Assignee)

Comment 6

10 years ago
Posted patch Addressed review comments (obsolete) — Splinter Review
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.
Attachment #402545 - Attachment is obsolete: true
Attachment #402558 - Flags: review?(mozbugz)
Attachment #402545 - Flags: review?(mozbugz)
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.
Attachment #402558 - Flags: review?(mozbugz) → review+
(Assignee)

Comment 8

10 years ago
Posted patch With variable renamed (obsolete) — Splinter Review
Attachment #402565 - Flags: review+
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],"
(Assignee)

Comment 10

10 years ago
I also noticed that I was a "const" short on my array's declaration.
Attachment #402558 - Attachment is obsolete: true
Attachment #402565 - Attachment is obsolete: true
Attachment #402704 - Flags: review+
(Assignee)

Comment 11

10 years ago
Pushed changeset 8398d82dba41 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

10 years ago
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.
Attachment #402704 - Flags: approval1.9.2?
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

10 years ago
Duplicate of this bug: 488506
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.
Attachment #402704 - Flags: approval1.9.2?
(Assignee)

Comment 15

10 years ago
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).
Attachment #402704 - Flags: approval1.9.2?
(Assignee)

Comment 16

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

Pushing approval request for this regression fix to the next branch release.
Attachment #402704 - Flags: approval1.9.2.1?
Attachment #402704 - Flags: approval1.9.2?
Comment on attachment 402704 [details] [diff] [review]
Actually using the array

Moving approval flag - what's the testing coverage like, here?
Attachment #402704 - Flags: approval1.9.2.2? → approval1.9.2.3?
(Assignee)

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
Posted patch Basic testSplinter Review
This test fails on Linux branch builds as they still have the bug, but it passes everywhere else.
Attachment #432334 - Flags: review?(karlt)
Comment on attachment 432334 [details] [diff] [review]
Basic test

Thanks.  (I didn't know about hg copy.)
Attachment #432334 - Flags: review?(karlt) → review+
(Assignee)

Comment 21

9 years ago
Comment on attachment 432334 [details] [diff] [review]
Basic test

Pushed changeset befd1c73e3f1 to mozilla-central.
(Assignee)

Comment 22

9 years ago
The trunk test doesn't apply cleanly to the branch, so I regenerated the patch.
Attachment #432535 - Flags: approval1.9.2.3?

Comment 24

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 552449
(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 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!
Attachment #432535 - Flags: approval1.9.2.4? → approval1.9.2.4-
You need to log in before you can comment on or make changes to this bug.