223909 - copy and paste windows clipboard images

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Scott MacGregor]

This is really just a dupe of 47838 but that bug has a lot of traffic. I'll be
landing this in thunderbird first and then landing on the Mozilla Trunk since my
initial work for thunderbird is inappropriate for the mozilla trunk. We'll have
to figure out where the right places are for all the code required to implement
this.

This bug is for thunderbird tracking only.

Comment 1

[OstGote!]

Because this feature is in TB 0.4, I wonder why this TB bug is not fixed.

Comment 2

[Scott MacGregor]

Attachment: the patch that thunderbird uses to do this

needs cleaned up, proper module owners consulted to make sure the pieces land
in the right modules before it could be checked in for everyone to use.

Comment 3

[Scott MacGregor]

Attachment: additional fix so we don't break static builds

Comment 4

[Bogdan Stroe]

Was this checked in? Is this bug fixed?
See bug 248760 for a problem related to this.

Comment 5

[Scott MacGregor]

when this lands on the trunk, don't forget:

Bug #249593

Comment 6

[Mike Cowperthwaite]

A problem with this patch is that it converts everything to JPEG.  I guess 
it's not easy to figure out from the content whether PNG would be better, but 
sometimes it would.  Attachments follow...


(In reply to comment #5)
> when this lands on the trunk, don't forget:
> 
> Bug #249593

That's the FF version of TB's bug 228034.  228034 has a comment that indicates 
it should be reopened (I can't test that stuff).


(In reply to comment #4)
> See bug 248760 for a problem related to this.

Not really related -- that problem (and its dupe) has to do with converting HTML 
to plain, not with editing HTML.

Comment 7

[Mike Cowperthwaite]

Attachment: Zoom-in view of original graphic

The original was a GIF generated by MapQuest; this PNG image is a screen
capture of a zoom-in program to show the pixels of the original detail.

Comment 8

[Mike Cowperthwaite]

Attachment: Zoom-in view of JPEG conversion

This image is the same thing showing the same part of the image as displayed in
the received email; lossiness is quite visible.

To be clear, the map is still quite legible as displayed.  However, if I insert
the GIF with Insert|Image, the message is actually smaller -- in this case, JPG
is not only lossy, it's bigger than necessary.	(Maybe the compression in use
isn't so good?)

I suspect that pasting an already-lossy JPG could result in noticeable image
degradation, and may also be larger.  I don't know that this is solveable when
the clipboard contains a bitmap, tho.

Comment 9

[Peter Lairo]

(In reply to comment #6)
> A problem with this patch is that it converts everything to JPEG.  I guess 
> it's not easy to figure out from the content whether PNG would be better, but 
> sometimes it would.

<stab in the dark>
If we paste as PNG, does the recipient have to have a PNG capable e-mail client?
If so, could Outlook render these pics? IMO, that would be a must.
</stab in the dark>

Comment 10

[Peter Lairo]

I seem to recall that pasting screenshots worked (for a while?) in Thunderbird.
Unfortunately, it doesn't seem to be working in the recent trunk nightlies
(version  (20050417)). True?

Since this seems something people do often (e.g., paste vacation photos) and
there seems to be at least a partial fix, I'm requesting blocking 1.1.

Comment 11

[Peter Lairo]

Another reason this should block 1.1 is that this feature *appears* to already
exist in the 1.0.x branch (i'm on the trunk), so users will expect it to
continue to work once they upgrade to 1.1. (see bug 47838 comment 146)

Comment 12

[Scott MacGregor]

Peter, there's no need to nominate a bug for 1.1 that's already in the 1.1 bucket.

Comment 13

[Scott MacGregor]

Attachment: patch for the 1.8 Branch

Comment 14

[Scott MacGregor]

Background: Stuart and I talked about this patch back in April when we were
figuring out how to land it on the trunk. He has a plan for a new architecture
for making it easy to support image encoders, see: 
http://wiki.mozilla.org/Mozilla2:Image_Encoding. We were hopeful that these
pieces would be in place in time for the Aviary 1.5 release, but that didn't happen.

So this brings us back to keeping this out off the trunk again until the new
image lib pieces are in place. However, we still need this functionality in 1.0.
Here's the patch that went into the Aviary 1.0 branch, but now applied to the
MOZILLA_1_8_BRANCH.

Comment 15

[Scott MacGregor]

Comment on attachment 193095 [details] [diff] [review]
patch for the 1.8 Branch

Can you look over the nsJPEGEncoder.cpp.h files again? This is the same code
that went into the Aviary 1.0 branch. I figure I'll have an easier time making
my case if I can tell them that you looked over the changes again :)

Comment 16

[Stuart Parmenter]

Comment on attachment 193095 [details] [diff] [review]
patch for the 1.8 Branch

r=stuart

Comment 17

[Scott MacGregor]

Comment on attachment 193095 [details] [diff] [review]
patch for the 1.8 Branch

This patch is identical to what we landed on the aviary 1.0 branch. Stuart has
looked through the patch again to make sure it looks like what we already took
for 1.0.

See comment 14 for why we don't want to check this patch into the trunk, branch
only (as we did with aviary 1.0). 

The risks with this patch are lower than you would think because it was already
part of 1.0. Also, this functionality is Windows only. When this landed for
1.0, it did break some 3rd party Windows compilers (like MINGW), so there were
a couple follow up fixes to address that. Worse case scenario here is that I
missed one of those follow up patches (I believe I found them all)  causing
MINGW compiler problems. Note: none of our release machines use that compiler.

Why we need this: this restores 1.0 functionality for Windows users for copying
and pasting images and is a requirement for several enterprise deployments.

The risk to Firefox should be minimal. The new code is called by HTML mail
compose when pasting images into an HTML editor instance.

Comment 18

[Scott MacGregor]

The patch has one mistake in it. The Makefile.in change for
modules\libpr0n\decoders\jpeg should only be building the jpeg encoder for
windows. So it should be wrapped in an ifdef like we did on the 1.0 branch:

ifeq ($(OS_ARCH),WINNT)
CPPSRCS         += nsJPEGEncoder.cpp
endif

I will fix that before I checkin.

Comment 19

[Asa Dotzler [:asa]]

Please land on the trunk and then if things look good, we'll approve for 1.8b4.

Comment 20

[Scott MacGregor]

I think you guys missed my comment. This change is branch only. We don't want it
on the trunk. Hence the reason we waited until we had a branch.

>See comment 14 for why we don't want to check this patch into the trunk, branch
>only (as we did with aviary 1.0). 

Comment 21

[Scott MacGregor]

Brendan and the other product drivers would like to see this in the trunk first,
and then when the new image encoder APIs come online we can re-work it to fit
into the new image lib encoding architecture. With that new information, I'm
going to break the patch down into some smaller pieces to make sure I get
individual module owner approval.

Comment 22

[Scott MacGregor]

Attachment: breaking out just the HTML editor change for module owner approval

Hi Daniel,

This is a change from the aviary 1.0 branch to make HTML editors able to handle
Windows clipboard image data, pasting the image into the text document. I
wanted to get module owner approval from you on this change before checking
into the trunk. Thanks!

Comment 23

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 193731 [details] [diff] [review]
breaking out just the HTML editor change for module owner approval

Absolutely. I have used that code a lot. While we are here, I have to say I
extended a bit nsHTMLEditor::InsertFromTransferable to allow drag and drop of
image files from unix tools like KDE file manager. Will be in  Nvu landing
bugs.
r/moa=[e-mail]

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 193731 [details] [diff] [review]
breaking out just the HTML editor change for module owner approval

+      rv = InsertHTMLWithContext(htmlstr, nsString(), nsString(), flavor,
aSourceDoc, aDestinationNode, aDestOffset, aDoDeleteSelection);


this could use EmptyString() instead of nsString()


+	       stuffToPaste.Assign(NS_LITERAL_STRING("<IMG src=\""));
+	       stuffToPaste.Append(NS_ConvertUTF8toUCS2(urltext));
+	       stuffToPaste.Append(NS_LITERAL_STRING("\" alt=\"\" >"));

maybe AssignLiteral/AppendLiteral and AppendUTF8toUTF16?

Comment 25

[Scott MacGregor]

Christian, I made your suggested string changes. If you see anything else in
https://bugzilla.mozilla.org/attachment.cgi?id=193095 feel free to speak up
before I put this on the trunk. Thanks!

Comment 26

[Scott MacGregor]

Attachment: breaking out just the libpr0n encoder changes

carrying forward stuart's r=

Comment 27

[Scott MacGregor]

Attachment: [checked into 1.8] the final 1.8 patch

attaching for historical reference, This is the patch that actually got checked
into the 1.8 branch.

Comment 28

[Scott MacGregor]

Stuart tells me that someone just started working on the new image lib APIs
today and he'd really like us to wait until that starts to develop before trying
to land this on the trunk. 

Comment 29

[Scott MacGregor]

clearing the blocking flag since this is on the branch since we are leaving this
bug open for trunk work.

Comment 30

[Glenn Randers-Pehrson]

Checkin seems to be causing a double definition of INT32 on win32 platform
due to the new inclusion of the jpeg header file which leads to inclusion of
mozilla/jpeg/jmorecfg.h that defines INT32.  See bug #18574, comments 576-579.

One fix would be to surround the typedef with #ifndef INT32/#endif in jmorecfg.h

Comment 31

[Glenn Randers-Pehrson]

The double definition of INT32 seems to occur only after the MNG patch
has been applied.  My suggested fix doesn't work because the JPEG library
is compiled before the definition is SDK is encountered.  So, I'll have
to take care of the conflict within the MNG patch.

Comment 32

[Adam Guthrie]

*** Bug 306459 has been marked as a duplicate of this bug. ***

Comment 33

[Scott MacGregor]

moving off the 1.1 radar since this has been taken care of for 1.5.

Comment 34

[Peter Lairo]

(In reply to comment #33)
> this has been taken care of for 1.5.

This is fixed in the upcoming Tb 1.5 (branch)? Is it working in the *trunk* nightlies? (mine* doesn't)

* Thunderbird version 1.6a1 (20051025)

Comment 35

[wbayer]

Confirmed regression in Thunderbird 1.6a1 (20051029). Paste (Shift Insert in Windows) in compose window no longer works. 

Comment 36

[Scott MacGregor]

How can a bug that's still open be a confirmed regression? This has never landed on the trunk. Branch only. :)

Comment 37

[Mike Cowperthwaite]

*** Bug 324593 has been marked as a duplicate of this bug. ***

Comment 38

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ouch; this or any variation of this never touched the trunk, and we've had a rework of the image encoders on the trunk for a few months now.  I'm working on uplifting the image encoders onto the branch for firefox alpha3 now, and will probably regress this temporarily (see bug 338407).  We should reimplement the nsHTMLDataTransfer pieces in terms of the new interfaces on the trunk so that it can get some baking there and then move that new code to the branch.

Comment 39

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I just regresed this on 1.8.1, but I'll be on the hook for fixing it on both trunk and branch ASAP.  (I'm on vacation next week, but it'll be done shortly after that.)

Comment 40

[Scott MacGregor]

(In reply to comment #39)
> I just regresed this on 1.8.1, but I'll be on the hook for fixing it on both
> trunk and branch ASAP.  (I'm on vacation next week, but it'll be done shortly
> after that.)
> 

I just noticed this was broken on 1.8.1 as I was doing some smoketests for the Thunderbird 2 alpha. We need to fix this before Thunderbird 2 final release.

FYI, this patch never landed on the trunk because of some discussions I had with Stuart where the image encoders were going to be changing a lot and he didn't want this stuff going in there since it would need to be re-written anyway.

Any tips you can point me to start making this work on the branch vlad? 

Comment 41

[Peter Lairo]

(In reply to comment #38)
> we've had a rework of the image encoders on the trunk for a few months now.

(In reply to comment #39)
> I'll be on the hook for fixing it on both trunk and branch ASAP.

(In reply to comment #40)
> FYI, this patch never landed on the trunk because of some discussions I had
> with Stuart where the image encoders were going to be changing a lot and he
> didn't want this stuff going in there since it would need to be re-written
> anyway.

Comment 38 said that the image encoders have been on the trunk for months now. What's keeping this bug from the trunk? Are the image decoders still going to change significantly? Is a later adaption of the bug fix to possible future changes in the image decoders a bigger price than having the trunk testers/users not have this bug fixed for 3 years and counting? Is there a "plan"(ETA)? Thanks!

Comment 42

[Jed B]

Yeah, this is broken in <editor widgets on Firefox 2.0b1 as well.
In firefox 1.5 you can paste an image from clipboard into an <editor (after you've called edtorElement.makeEditable) and it would show the image grabbing it from your OS's temp dir ..path/moz-screenshot-#.jpeg

I need this bad for my extension and app, and I can only assume others do as well.

I guess my request is, if this gets fixed for TB 2.0, can we make sure it works in Firefox 2.0 nsIEditor interfaces as well?

Comment 43

[Scott MacGregor]

Attachment: trunk patch using the new encoders

Here's a patch that uses the new encoder APIs on the trunk. I had a meta question for vlad & stuart though:

I could make the clipboard pass out a gfxImageFrame to clipboard consumers and the consumer would be responsible for extracting the RGB pixel data out of the gfxImageFrame, creating a JPG or PNG encoder and then encoding the image.

Or I could let the clipboard image converter code actually encode the clipboard image into a PNG/JPEG and have it hand out the nsIInputStream to clipboard consumers.

I implemented it both ways and then decided I liked the later approach, since it seemed like I was abusing a gfxImageFrame object just so I could pass RGB data between the clipboard and editor and the encoder. And I liked not having to have editor deal with encoding images. And the consumer has already asked for PNG/JPEG anyway so we should give them what they really want. If you strongly object, I can implement the first approach.

Onto the details:

1) Make editor's nsHTMLDataTransfer object pay attention to the PNG image format on the clipboard for paste operations. Get the nsIInputStream to the PNG and write it out to a temporary file, then insert a link that refers to the file we saved the PNG to. (The saving to a temp file and then inserting a link to the image is what we used to do in Fx/tb 1.0/1.5)

2) I choose to use the new PNG encoder instead of the JPEG encoder we used in 1.0/1.5 for pasting clipboard images into html editors.

3) The bulk of the clipboard changes involve a private helper class, nsImageFromClipboard. This class understands all of the various formats for bitmaps on the windows clipboard. I moved most of the code I wrote for 1.0/1.5 into this helper class so it's been tested on various screen resolutions, color depths etc in 1.0/1.5 (i.e. InvertRows, ConvertColorBitMap, CalcBitmask and CalcBitShift).

4) nsImageFromClipboar ::GetEncodedImageStream calls ConvertColorBitMap which converts the bitmap into RGB pixels. GetEncodedImageStream then passes that onto the PNG encoder, returning the resulting nsIInputStream.

Comment 44

[Scott MacGregor]

It would help if I cc'ed vlad if I expect him to answer my questions. 

In particular, see comment 43. Thanks :)

Comment 45

[Kathleen :Brade]

A few questions about the trunk patch:
 * Why does the editor need to depend on exthandler?  I thought there was a different interface for dealing with temp files or has that been deprecated?
 * It seems odd to me that the temp file is deleted on exit.  Is that really desirable?  It seems horrible for Composer.  I'm guessing mail compose does the right thing if you save the message before exiting so it may not be impacted in the same way.  I'm not sure if or how "Midas" widgets would be affected by this.
 * Why aren't there any changes for Mac?  Does the Mac widget code still work?
 * This change makes no sense to me; please do not land it:
-      else if ( strcmp(flavorStr, kPNGImageMime) == 0 || strcmp(flavorStr, kJPEGImageMime) == 0 ||
+      else if ( strcmp(flavorStr, kPNGImageMime) == 0 || strcmp(flavorStr, kPNGImageMime) == 0 ||

 * I don't think you should be removing the "JPG" flavor from the Windows clipboard code.  Why remove functionality?  The editor isn't the only code that can put images on the clipboard or read image data off of the clipboard.

I really like the idea of the clipboard holding an nsIInputStream of the image data but I wish it could be more general.  Ideally I could put JPEG (or GIF or BMP or ...) data in an nsIInputStream on the clipboard and that would work too.

Comment 46

[Scott MacGregor]

Thanks for the comments Kathy.

(In reply to comment #45)
> A few questions about the trunk patch:
>  * Why does the editor need to depend on exthandler?  I thought there was a
> different interface for dealing with temp files or has that been deprecated?

I'm not aware of another interface for doing this. 

>  * It seems odd to me that the temp file is deleted on exit.  Is that really
> desirable?  It seems horrible for Composer.  I'm guessing mail compose does the
> right thing if you save the message before exiting so it may not be impacted in
> the same way.  I'm not sure if or how "Midas" widgets would be affected by
> this.

This code is unchanged behavior wise with regards to the editor code we shipped in 1.0 / 1.5 which deleted the temp file on exit for the image. I didn't change that part of the patch when porting to the trunk. I'm open to suggestions to improving it for composer use case. I haven't seen any negative feedback from 1.0/1.5 users but maybe they aren't trying to paste images in composer, just in mail?


>  * Why aren't there any changes for Mac?  Does the Mac widget code still work?

No one has every implemented this code on the Mac, it's only been implemented on Windows. Feel free to contribute a patch for getting the pixels from the mac clipboard and feeding them into an image encoder. Although I'd suggest waiting until we see if Vlad / Stu approve of the approach I'm taking here for the updated Windows code. I think it should be much easier to hook up than it was with the old implementation of this feature.

>  * This change makes no sense to me; please do not land it:
> -      else if ( strcmp(flavorStr, kPNGImageMime) == 0 || strcmp(flavorStr,
> kJPEGImageMime) == 0 ||
> +      else if ( strcmp(flavorStr, kPNGImageMime) == 0 || strcmp(flavorStr,
> kPNGImageMime) == 0 ||

This was a typo, I've already fixed it. good catch!
> 
>  * I don't think you should be removing the "JPG" flavor from the Windows
> clipboard code.  Why remove functionality?  The editor isn't the only code that
> can put images on the clipboard or read image data off of the clipboard.

I can add back the JPG flavor, but it wouldn't be hooked up to anything unless we do something like below:

> I really like the idea of the clipboard holding an nsIInputStream of the image
> data but I wish it could be more general.  Ideally I could put JPEG (or GIF or
> BMP or ...) data in an nsIInputStream on the clipboard and that would work too.

I'm not sure I follow. Images on the clipboard are always stored as bitmaps. Are you saying you'd like to get a nsIInputStream back for a JPG version of the image or for the PNG version of the image? We should be able to support both flavors by passing in the flavor to the converter class so it invokes the correct encoder. That could be interesting. 

Comment 47

[Mike Cowperthwaite]

(In reply to comment #45)
>  * Why aren't there any changes for Mac?  Does the Mac widget code still work?

Bug 310948.

(In reply to comment #46)
> Are you saying you'd like to get a nsIInputStream back for a JPG version of
> the image or for the PNG version of the image? We should be able to support
> both flavors by passing in the flavor to the converter class so it invokes
> the correct encoder. That could be interesting. 

See comment 6 thru 8.  I dunno if it's possible to make an autodetector to pick the "best" format (a choice between lossy JPEG or lossless PNG, essentially), but I'm having difficulty imagining a useful interface for the user to pick.

Is it possible to get the original image type stored in the clipboard, at least for copies-from-Mozilla?  (Or is that info already available?)

Comment 48

[Kathleen :Brade]

I think we need a mechanism where the editor can handle temporary files.  Creating and deleting temp files doesn't work for HTML editing because the temporary image files will be removed when the user exits the app.  The mechanism we need should let whatever application know about "temp" files so the app can handle it in an appropriate way (attach to email, create permanent file near parent file, disallow or whatever).  I haven't given this much thought; has Daniel discussed it with anyone?

I've considered a data url but that doesn't feel quite right (and doesn't really meet the expectations of most Composer users).  I keep coming back to the need for the app to know about the file being "linked" to the inserted data.

I don't think that 4.x had such a mechanism; I think 4.x just created a file nearby (which probably didn't get removed in the mail compose case).

Maybe it would work if the app had a way to specify where files should be added by default for a particular editor instance (a temp location for mail compose and the directory for the file in the case of Composer).  This might be cleaner/easier.  [Note to self or whomever works on Composer-side of this: make sure that the location gets changed when a user chooses "save as" and other scenarios]

Comment 49

[Scott MacGregor]

Attachment: updated trunk patch

Comment 50

[Scott MacGregor]

This updated patch goes back to using JPEG instead of PNG on the trunk. I want to get the exact behavior we shipped with 1.0 and 1.5 again for 2.0 and we want to get that working on the trunk first. I think it would be interesting to explore supporting the new PNG encoder as well and this should be straight forward given the new implementation. I'll file a bug on that.

Stu graciously volunteered to review the widget specific changes. I'll go through Brade/Daniel for the editor changes.

Comment 51

[Scott MacGregor]

Comment on attachment 237228 [details] [diff] [review]
updated trunk patch

Stu, can you review the widget changes in this patch? 

Most of the new code in nsImageFromClipboard is code that's migrating from the old way I implemented this in libpr0n.

Comment 52

[Scott MacGregor]

Comment on attachment 237228 [details] [diff] [review]
updated trunk patch

Vlad, would you be willing to sr the widget changes in this patch? This code uses the new image encoder APIs. See comment 43 for more details about the patch.

Thanks!

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 237228 [details] [diff] [review]
updated trunk patch



Just some comments below.. looks ok otherwise.

> nsresult 
>-nsImageFromClipboard :: GetImage ( nsIImage** outImage )
>+nsImageFromClipboard ::GetEncodedImageStream (unsigned char * aClipboardData, nsIInputStream** aInputStream )
> {
>-  NS_ASSERTION ( outImage, "Bad parameter" );
>-  *outImage = nsnull;
>+  NS_ENSURE_ARG_POINTER (aInputStream);
>+  nsresult rv;
>+  *aInputStream = nsnull;
>   
>-  static NS_DEFINE_IID(kCImageCID, NS_IMAGE_CID);
>-  nsresult rv = CallCreateInstance(kCImageCID, outImage);
>-  if ( NS_SUCCEEDED(rv) ) {
>-    // pull the size informat out of the BITMAPINFO header and
>+  // pull the size information out of the BITMAPINFO header and
>     // initialize the image
>-    PRInt32 width  = mHeader->bV4Width;
>-    PRInt32 height = mHeader->bV4Height;
>-    PRInt32 depth  = mHeader->bV4BitCount;
>-    PRUint8 * bits = GetDIBBits();
>-    if ( !bits )
>-      return NS_ERROR_FAILURE;
>-      
>-    // BUG 44369 notes problems with the GFX image code handling
>-    // depths other than 8 or 24 bits. Ensure that's what we have
>-    // before we try to work with the image. (pinkerton)
>-    if ( depth == 24 || depth == 8 ) {
>-      (*outImage)->Init(width, height, depth, nsMaskRequirements_kNoMask);
>-
>-      // Now, copy the image bits from the Dib into the nsIImage's buffer
>-      PRUint8* imageBits = (*outImage)->GetBits();
>-      depth = (depth >> 3);
>-      PRUint32 size = width * height * depth;
>-      ::CopyMemory(imageBits, bits, size);
>+  BITMAPINFO* header = (BITMAPINFO *) aClipboardData;
>+  PRInt32 width  = header->bmiHeader.biWidth;
>+  PRInt32 height = header->bmiHeader.biHeight;

height could be negative, indicating that the image is flipped -- this code should either check that and bail (as most apps tend to do), or should know to flip the Y axis of the image.

>+
>+  unsigned char * rgbData = new unsigned char[width * height * 3 /* RGB */];
>+
>+  if (rgbData) {
>+    BYTE  * pGlobal = (BYTE *) aClipboardData;
>+    // Convert the clipboard image into RGBA pixel data

Is it RGBA or RGB (packed?); ConvertColorBitMap seems to always return packed RGB, so comment shouldn't say RGBA

>+    rv = ConvertColorBitMap((unsigned char *) (pGlobal + header->bmiHeader.biSize), header, rgbData);
>+    // if that succeeded, encode the bitmap as a PNG. Don't return early or we risk leaking rgbData
>+    if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<imgIEncoder> encoder = do_CreateInstance("@mozilla.org/image/encoder;2?type=image/jpeg", &rv);

Hmm, is the intent to encode as jpg and not PNG?  The comment and the method documentation should be fixed to say jpeg, not PNG.

>+      if (NS_SUCCEEDED(rv)){
>+        rv = encoder->InitFromData(rgbData, 0, width, height, 3 * width /* RGB * # pixels in a row */, 
>+                                   imgIEncoder::INPUT_FORMAT_RGB, NS_LITERAL_STRING("transparency=none"));
>
>+        if (NS_SUCCEEDED(rv))
>+          encoder->QueryInterface(NS_GET_IID(nsIInputStream), (void **) aInputStream);
>     }
>   }
>+    delete [] rgbData;
>+  } 
>+  else 
>+    rv = NS_ERROR_OUT_OF_MEMORY;
> 
>   return rv;
>-
> } // GetImage

Comment 54

[Scott MacGregor]

Attachment: updated widget changes based on review feedback

Breaking out the widget changes.

Carrying forward stu's r and vlad's sr.

Next up, editor.

Comment 55

[Scott MacGregor]

Attachment: editor changes for the trunk

Kathy, are you interested in reviewing this code for the trunk? I went through Daniel for the branches last time, but am happy to mix it up since you've had a lot of comments on this stuff.

I removed the code for removing the file we generate for the screen shot, per your comments. That should make it a little more composer friendly. Although we didn't get any complaints about deleting it in 1.0 and 1.5 so either folks weren't using the functionality in composer or it wasn't coming up. 

Let's get the functionality we had in 1.0 and 1.5 back in the trunk. Then we can start spinning up new trunk only bugs for some of the ideas you were exploring for the composer side. How does that sound?

Comment 56

[Scott MacGregor]

Attachment: editor changes for the trunk

Kathy, are you interested in reviewing this code for the trunk? I went through Daniel for the branches last time, but am happy to mix it up since you've had a lot of comments on this stuff.

I removed the code for removing the file we generate for the screen shot, per your comments. That should make it a little more composer friendly. Although we didn't get any complaints about deleting it in 1.0 and 1.5 so either folks weren't using the functionality in composer or it wasn't coming up. 

Let's get the functionality we had in 1.0 and 1.5 back in the trunk. Then we can start spinning up new trunk only bugs for some of the ideas you were exploring for the composer side. How does that sound?

Comment 57

[[e-mail]]

Comment on attachment 238261 [details] [diff] [review]
editor changes for the trunk

At the risk of sounding like an echo, may I suggest changing nsString() to EmptyString()?

Comment 58

[Scott MacGregor]

Attachment: editor changes using EmptyString()

Changed nsString() to EmptyString per Neil (good catch!)

Trying Daniel for the code review for the editor change. Daniel, see comment 56 for details. This brings the image copy and paste functionality we had on the branches to the trunk.

Comment 59

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 240171 [details] [diff] [review]
editor changes using EmptyString()

Am I correct in the assumption you can only paste once in a single message using this code ?

If you have more than one screenshot, any new one will erase/override the
old one in $MOZTMPDIR/moz-screenshot.jpg

Test it : create a new mail, and four times in a row press on the printscreen key
and paste the clipboard into the message. Send the message. Check that the
received mail contains four times the same image instead of four different imgs.

I don't think this is acceptable from a user's point of view.

Prove me wrong and I'll r+. In the meantime, I have this feeling this is not ironed enough from a feature's perspective.

Comment 60

[Scott MacGregor]

Comment on attachment 240171 [details] [diff] [review]
editor changes using EmptyString()

Hi Daniel,

Thanks for looking at this patch. Actually we call CreateUnique on the file ensuring that the image file name is always unique, moz-screenshot-1.jpg, moz-screenshot-2.jpg, etc.

See:

+      fileToUse->Append(NS_LITERAL_STRING("moz-screenshot.jpg"));
+      nsCOMPtr<nsILocalFile> path = do_QueryInterface(fileToUse);
+      path->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);

Comment 61

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 240171 [details] [diff] [review]
editor changes using EmptyString()

scott: oh ! My bad !
r=me then

Comment 62

[Scott MacGregor]

Ok, the editor patch is now on the trunk too. Tomorrow's trunk builds should *finally* support copying and pasting images from the clipboard into HTML compose! 

Restoring the functionality we had in Thunderbird 1.5 is a blocker for TB2. But the clipboard changes contain code shared with Firefox and is locked down right now. 

I think the best approach is to put together a patch for the branch, and nominate that patch for 1.8.1.1. Changes in 1.8.1.1 will make it into Thunderbird 2.

Comment 63

[Scott MacGregor]

marking fixed since this is fixed on the trunk.

Comment 64

[Chef Chaudart]

(In reply to comment #63)
> marking fixed since this is fixed on the trunk.
> 

On version 3 alpha 1 (20061012), XP SP2, I got strange behaviour.

Sometimes it works,  sometimes, I get only a square with red round.

Comment 65

[Mike Cowperthwaite]

(In reply to comment #64)
> On version 3 alpha 1 (20061012), XP SP2, I got strange behaviour.
> 
> Sometimes it works,  sometimes, I get only a square with red round.

In the cases where you get the Broken Image icon, where were you copying the image from?  This symptom is a known issue when copying from another message: bug 224733.

Comment 66

[Chef Chaudart]

(In reply to comment #65)
> (In reply to comment #64)
> > On version 3 alpha 1 (20061012), XP SP2, I got strange behaviour.
> > 
> > Sometimes it works,  sometimes, I get only a square with red round.
> 
> In the cases where you get the Broken Image icon, where were you copying the
> image from?  This symptom is a known issue when copying from another message:
> bug 224733.
> 
I made a Print Screen of TB !  And I wanted to copy it in a message body.

Comment 67

[Chef Chaudart]

I noticed that when I want to re-dimension the broken image icon,  I see the initial content of the image !

But only while I change dimensions. When  finish, it's back to BII.

Comment 68

[Scott MacGregor]

(In reply to comment #64)
> (In reply to comment #63)
> > marking fixed since this is fixed on the trunk.
> > 
> 
> On version 3 alpha 1 (20061012), XP SP2, I got strange behaviour.
> 
> Sometimes it works,  sometimes, I get only a square with red round.
> 


I filed Bug 356327 to track this yesterday and marked this bug dependent on it. 

Comment 69

[Scott MacGregor]

Attachment: New Implementation Merged to the 1.8.1 Branch

I backed out the implementation for pasting clipboard images on the 1.8.1 branch. I then added the changes that went into the trunk. 

This 1.8.1 patch contains both a back out of the now broken changes for pasting clipboard images and the trunk patches that properly implemented this feature.

Comment 70

[Mike Schroepfer]

Can we get a rationale for why we need this on the 1.8.1 branch? 

Comment 71

[Scott MacGregor]

Comment on attachment 243156 [details] [diff] [review]
New Implementation Merged to the 1.8.1 Branch

Stu, do you mind reviewing this again for the branch just to make sure we've got all our bases covered. This patch backs out the old implementation and then applies the patch from the trunk (which applied cleanly). So you've seen it all before for the trunk review.

Comment 72

[Stuart Parmenter]

Comment on attachment 243156 [details] [diff] [review]
New Implementation Merged to the 1.8.1 Branch

I was just looking at this a few days ago!  looks the same as before..

Comment 73

[Scott MacGregor]

Comment on attachment 243156 [details] [diff] [review]
New Implementation Merged to the 1.8.1 Branch

I know this patch looks large but let me break it down.

History: You used to be able to copy and paste images out of the windows clipboard into HTML editor instances in 1.0 and 1.5. This regressed when we landed Bug 338407 on the 2.0 branch. So right now with Firefox 2 you can't paste clipboard images. 

This patch restores that 1.0/1.5 functionality by using the new image encoding APIs added as part of Bug 338407.

1) Much of the clipboard image processing code shipped with 1.0/1.5 and is just being moved to a new location in this patch. So the patch looks large, but much of the code isn't new.

2) The entry point for exercising this clipboard code is well isolated. You have to be pasting a clipboard image into a HTML editor instance for this code to be exercised. For Thunderbird, that means any HTML compose window. For Firefox, extension authors that use editable editor instances would be able to start pasting clipboard images again in  Fx 2.0.* (see Jed B's comment 42)

The patch has been on the trunk since 10/11/2006. Has been reviewed at various points by vlad, stu and daniel glazman.

Comment 74

[Jay Patel [:jay]]

Comment on attachment 243156 [details] [diff] [review]
New Implementation Merged to the 1.8.1 Branch

Approved for 1.8.1 branch, a=jay for drivers.  Please land this asap.  Thanks!

Comment 75

[Scott MacGregor]

This is now working on today's branch windows build.

Comment 76

[Mike Cowperthwaite]

I can confirm that.  I notice that the temp file created is left on disk (in the Temp directory) -- that's OK?  There are in fact several, from today's branch test and earlier trunk usage: moz-screenshot.jpg, moz-screenshot-1, 
-2, -3, etc.

Comment 77

[Mike Cowperthwaite]

*** Bug 361602 has been marked as a duplicate of this bug. ***

Comment 78

[Stewart Gordon]

Note that this was apparently only implemented for HTML email, in order to have the image displayed inline.  For pasting images as attachments more generally, we have bug 335783.