281877 - Thunderbird doesn't have "Copy Image" on context menu

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Jerry Baker]

20050207 trunk

Open a message containing an attached image. Right-click it and select "Copy
Image." Now, try to paste it into another application.

Result: Nothing.

Comment 1

[Mike Cowperthwaite]

It does copy the image URL to the clipboard, as text, which is pasteable.  
Version 1.0, in fact, displays that menu item as "Copy Image Location".

I don't know if the different text is inadvertant, or an early change in support 
of actual image copying.

Comment 2

[Jerry Baker]

I would assume that it would do the same thing as "Copy Image" in Firefox (which
puts the image onto the clipboard).

PS - Why would you want to copy the URL of an image attached to an email?

Comment 3

[Mike Cowperthwaite]

xref bug 279270 -- there was a regression in Camino that occurred in January 
with a similar symptom, apparently triggered by a patch for bug 135300; perhaps 
the same change affected the TB UI as well?

Comment 4

[Jerry Baker]

Now the "Copy Image" context menu item has been removed altogether.

Comment 5

[xolaware.llc]

can Hardware be changed to all and OS be changed to all?  because this is a problem on mac as well.

Comment 7

[rsx11m]

Indeed, Thunderbird only copies the location but not the content onto the clipboard. This was fixed in SeaMonkey per bug 135300 years ago and works just fine in their 2.0 release candidates, thus wouldn't it be straight-forward to port this to Thunderbird as well? It's rather a basic functionality, too late for 3.0 due to the string freeze in effect now, but should be a target for 3.1.

Comment 8

[rsx11m]

(In reply to comment #3)
> xref bug 279270 -- there was a regression in Camino that occurred in January 
> with a similar symptom, apparently triggered by a patch for bug 135300;

I see, should have read the earlier comments here more thoroughly... :-\

However, there is Core/DOM bug 291110 which fixed the issues caused by the SeaMonkey patch and made it work for Camino again. Checkin was 2005-05-05, thus strange that Copy Image nevertheless was removed from Thunderbird (comment #4).

Comment 9

[rsx11m]

As a proof of concept, I have replaced command="cmd_copyImageLocation" in mailWindowOverlay.xul line #752 with command="cmd_copyImageContents" and it correctly copies the image as DIB now onto the clipboard (using Windows XP). Thus, this shouldn't involve more than adding a respective menu item to the context menu to also make this possible for Thunderbird.

Comment 11

[rsx11m]

Attachment: Patch without string changes

I'm giving this a try for Thunderbird 3.0, it should still work even without string changes. The current version of "Copy Image Location" without the actual capability of transferring the contents is not useful at all, in any way other than for web addresses.

This patch synchronizes mailWindowOverlay.xul and utilityOverlay.xul in mail/ with its suite/ counterparts, using cmd_copyImage to copy both location (plain text and as HTML link) and the image data onto the clipboard, thus allowing the pasting application to select the desired flavor. On Windows, it will paste an image into MS Paint, by default a link into Wordpad (or as image when using Paste As), and allows to pick DIB in MS Word in order to get the image.

The string should more correctly read "Copy Image with Location" now, thus representing that extended functionality, but given the gain of actually copying the image itself, it appears that a slightly incomplete label is a justifiable trade-off for 3.0. If this patch goes through, the label can be easily taken care of in a follow-up patch for the new 3.1/trunk after comm-1.9.1 has branched off.

Comment 12

[rsx11m]

Attachment: String change for trunk

This patch is incremental to attachment 406964 [details] [diff] [review] and adjusts the label.
I made it just "Copy Image" now so that functionality and label are consistent
among applications.

Comment 13

[rsx11m]

Bryan suggested to do some more testing on other platforms, where I don't have Mac OSX available though. To verify that this also works on Linux (KDE 3.5), I've tested the patch with both 3.0pre and 3.1a1pre builds. Images either as attachment or being part of the message body copy into Gimp as images, so far
it works as intended (tested for local mailboxes and IMAP folders). I have noticed a difference though when it comes to copying the location URI. For the 3.1a1pre trunk build, it copies as text into an xterm, and Paste Special in OpenOffice gives me both choices for text/HTML location or bitmap contents, as desired. For 3.0pre, it appears that copyImage behaves actually more like copyImageContents (but only on Linux, worked fine on Windows 3.0pre builds), thus no text or HTML component comes with the image on the clipboard. 

I have also double-checked that cmd_copyImageLocation and cmd_copyImageContents are not used anywhere else in mail/, thus it is safe to replace them with cmd_copyImage in utilityOverlay.xul. I realize that this patch is coming in a bit late for 3.0, but it seems to be a straightforward fix with little risk (especially given that there are not much use cases for copying "mailbox:" or
"imap:" locations, and the other applications are using it in the same way).

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

This seems like a good change to me.  My only concern was the consistency across platforms.  I know Linux doesn't really have the best clipboard system.  If you wanted to ifdef the Linux part out so it continued to have the old command I think I would be more comfortable with the change at this point.

Comment 15

[rsx11m]

Did you get a chance to try it on Mac OSX for a 3.0 build? If not, and since there are no problems on Windows or with the 3.1 builds, feasible conditions would be to use cmd_copyImage for Windows but cmd_copyImageLocation else in the 3.0 patch, then undoing that for the 3.1 follow-up to have it active in all platforms. Again though, I didn't see any negative impact when testing this on Linux, other than the Location missing on the clipboard (image data intact).

Comment 16

[rsx11m]

If I'm not mistaken, #ifdef XP_UNIX covers Mac OSX anyway, thus I can use that in the 3.0 patch to ensure that the old command applies.

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

It seems to work for me as expected on the Mac.  I haven't tested it out on Linux yet.

Comment 18

[rsx11m]

Ok, I have a variant of the patch ready to apply attachment 406964 [details] [diff] [review]
for non-UNIX and Mac OSX only, but leave the current location command intact for UNIX other than Mac OSX. If you stick with not doing the change for Linux, I'll post those for r? instead. For now, I'm waiting for your decision.

Comment 19

[rsx11m]

Comment on attachment 406964 [details] [diff] [review]
Patch without string changes

Magnus, while waiting for Bryan to make up his mind if the copyImageContents behavior is acceptable for TB 3.0 on Linux, I'm flagging this for your review to make some progress. As said, copyImage works as intended on Windows and Mac OSX on branch, and for all platforms on trunk.

Now that comm-1.9.1 has branched off, do you want a consolidated patch for comm-central rather than the incremental branch-to-trunk one posted? I assumed that some bake time on trunk may be needed before it may go into the 3.0 branch, therefore the 2-step patch.

Comment 20

[Magnus Melin [:mkmelin]]

On linux the image isn't copied to the clipboard afaikt. Or maybe it is, but e.g. trying to paste in a new mail doesn't do the same as Copy Image in firefox - it just pastes the img location.

It's a bit redundancy, but i think i'd like best to have both commands in there.

Comment 21

[rsx11m]

Are you talking about applying it to 1.9.1 or trunk builds?

- On the 1.9.1 branch, bug 21747 changed the behavior of copyImage to copy the
  pixel data only onto the clipboard, not the location (that's for TB 3.0).

- On trunk, there is also bug 518249, which has modified copyImage to include
  all representations (location and pixel data, TB 3.1), from which the pasting
  application can pick which representation is wanted. Something like Gimp will
  paste the pixel data, if you paste into an xterm, it would paste the URI.

Thus, you may need to pick the right flavor from the clipboard using "Paste As" instead of the default paste. If you paste into a mail-composition window, it would take the location rather than the explicit pixel data.

Comment 22

[rsx11m]

To disambiguate my last statement, pasting from the copyImage into an HTML message will take the location in an HTML <img src="mailbox:..."> off the clipboard and resolve it (i.e., attaching the pixel data) when encoding.

(In reply to comment #20)
> It's a bit redundancy, but i think i'd like best to have both commands in
> there.

This wouldn't work for TB 3.0 any more due to the string freeze, thus we'd have to decide one way or the other. For 3.1, I could add copyImageContents instead and leave copyImage unconsidered, though an explicit copyImageLocation only has limited use cases.

Comment 23

[Magnus Melin [:mkmelin]]

This was a trunk build. Are you saying that was expected to work or not?

Comment 24

[rsx11m]

Yes, on trunk it should work on all platforms. The way how copyImage functions is that it offers all flavors (text location, HTML link, and pixel data) on
the clipboard and the pasting application picks one. Thus, it may paste the location as text by default, insert a link, or prefer the image pixel data.
I see that this may be confusing, but that was the reason for introducing it (to avoid the redundancy of having two menu entries).

Options on trunk are:
 [ ] Copy Image   (text + HTML + data)

or per your suggestion:
 [ ] Copy Image Location (text + HTML)
 [ ] Copy Image Contents (pixel data)

Comment 25

[rsx11m]

Attachment: Alternative branch/3.0 patch

This seems to be a good time to also post the alternative patches for comparison.

The first patch addresses Bryan's comment #14 to not change the behavior on Linux for the 3.0 release due to the missing fix for bug 518249. Thus, for Windows and Mac OSX (where we know that it works), copyImage is used and adds the pixel data to the clipboard as well. This should be fairly transparent but allows the user to also paste the image data. Linux retains copyImageLocation.

Unfortunately the XUL preprocessor didn't understand the cpp definition of
#if defined(XP_UNIX) && !defined(XP_MACOSX) and therefore two nested #ifdef clauses are necessary.

Comment 26

[rsx11m]

Attachment: Alternative trunk/3.1 patch

The second patch adds copyImageContents to the context menu for 3.1/trunk per comment #20. This avoids any ambiguity what is copied to the clipboard by introducing two distinct menu options. As a convenient side effect, this also provides a workaround for bug 417204 on Windows by allowing to explicitly transfer the image data over the clipboard only without a reference URI.

Though not necessary, I have also renamed the copyImageCmd.label to split it up into copyImageUrlCmd and copyImageDataCmd, this makes it clearer and should prompt localizers to have another look at that label to possibly adjust it as well.

Only gnomestripe adds icons to the context menu items, the respective entry has been adjusted.

Bryan, Magnus: Please comment on and flag the patches as you see fit.

Comment 27

[Magnus Melin [:mkmelin]]

If it would work i think Copy Image would be enough.

I just tried the patch again on linux (trunk build). There's no image data on the clipboard after i copy an image from a mail using the context menu. Paste in a new mail just gives the url, and trying paste in gimp it just says there is no image data on clipboard.

Comment 28

[rsx11m]

This is weird, attachment 406964 [details] [diff] [review] pastes as image to Gimp with either
1.9.1-branch or trunk builds. Can you try attachment 408605 [details] [diff] [review]?

Comment 29

[Magnus Melin [:mkmelin]]

Looks like I have some build related trouble so the patch didn't "take" for some reason. Will try to sort it out tomorrow.

Comment 30

[rsx11m]

Bryan, any results from your testing on Linux?

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 406964 [details] [diff] [review]
Patch without string changes

Yeah this works fine after all. 

I used to do "make -C mail" in the obj-dir to rebuild tb-only changes, but apparently that doesn't update things properly anymore on trunk. After "make -s tier_app" things worked though. 

Since this works i agree for trunk we just need the one menu item Copy Image.

Comment 32

[rsx11m]

Attachment: Patch complete with strings for trunk

Thanks Magnus. This is the same patch as attachment 406964 [details] [diff] [review], but with
the strings from attachment 406967 [details] [diff] [review], to allow a single checkin on trunk.

Comment 33

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 406964 [details] [diff] [review]
Patch without string changes

I didn't get a chance to try this on Linux but if it works for Magnus then that's more than good enough for me.

Comment 34

[rsx11m]

Comment on attachment 406964 [details] [diff] [review]
Patch without string changes

Thanks, requesting branch approval for checkin (minimal stability risk if any, high usage gain).

Comment 35

[rsx11m]

(In reply to comment #33)
> (From update of attachment 406964 [details] [diff] [review])
> I didn't get a chance to try this on Linux but if it works for Magnus then
> that's more than good enough for me.

Bryan, does your ui-review+ for the branch extend to attachment 408890 [details] [diff] [review]
for trunk? It's the same patch, just with the string changes.

Comment 36

[Magnus Melin [:mkmelin]]

Forgot to mention, i just tried it on trunk. Will get a branch build going soon.

Comment 37

[rsx11m]

Ok, please see comment #13 for the different behavior on branch with Linux.
Windows and Mac OSX behave identical for branch and trunk builds.

Comment 38

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 408890 [details] [diff] [review]
Patch complete with strings for trunk

Yes, those strings look fine.  I was a little concerned that we might confuse people just by breaking tradition of "Copy Image Location" in Firefox but after checking out a couple different browsers there isn't a solid theme for text here.

Comment 39

[rsx11m]

Comment on attachment 408603 [details] [diff] [review]
Alternative branch/3.0 patch

I'm marking this alternative patch obsolete after no more reviewer comments came in regarding any platform-specific handling on branch.

Comment 40

[rsx11m]

Please check in (r=mkmelin, ui-r=clarkbw):
 - attachment 408890 [details] [diff] [review] for comm-central (this one is ready to go)
 - attachment 406964 [details] [diff] [review] for comm-1.9.1 (pending a-tb3+, comment #34)

Comment 41

[Mark Banner (:standard8)]

Checked in:
http://hg.mozilla.org/comm-central/rev/f661950c2191
http://hg.mozilla.org/releases/comm-1.9.1/rev/b666bc3310c0