Closed
Bug 393646
Opened 14 years ago
Closed 14 years ago
Support reading image data off the clipboard
Categories
(Core :: Widget: Cocoa, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: cbarrett, Assigned: cbarrett)
Details
Attachments
(1 file, 4 obsolete files)
10.97 KB,
patch
|
jaas
:
review+
roc
:
superreview+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
Apps like Thunderbird would like it if we could get image data right off the clipboard for pasting. There's currently a stubbed out section of nsClipboard.mm that has a comment saying that "we've never supported this on Mac, maybe someday." We probably should implement this. I talked to Josh and he agrees.
Flags: blocking1.9?
Comment 1•14 years ago
|
||
Colin, you might find it helpful to check out the last patch in Bug 223909. For windows, it shows how we get the clipboard data, convert it to RGB packed format and then pass it to the jpg encoder using something like: + rv = ConvertColorBitMap((unsigned char *) (pGlobal + header->bmiHeader.biSize), header, rgbData); + // if that succeeded, encode the bitmap as a JPG. 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); + 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); Consumers in the code base know what to do with the jpeg stream.
Assignee | ||
Comment 2•14 years ago
|
||
Scott, does it need to be a JPEG? All of the image data coming off the clipboard is not lossily compressed (TIFF, PICT or PDF). Do consumers know what to do with a PNG stream as well?
Assignee | ||
Comment 3•14 years ago
|
||
(From reading bug 223909 it looks like no)
Comment 4•14 years ago
|
||
Bug 384116 is investigating to see if outlook and outlook express can render PNGs.
Assignee | ||
Comment 5•14 years ago
|
||
Here's what I get when I try pasting in an image: Hello, world! Signaling it's OK to paste looking for clipboard data of type application/x-moz-nativehtml looking for clipboard data of type text/html looking for clipboard data of type application/x-moz-file looking for clipboard data of type text/unicode These are all debugging messages -- the first one is in HasDataMatchingFlavors. The rest are coming from GetNativeClipboardData. It doesn't seem like callers are looking for kJPEGImageMime. Is there a flag I need to turn on elsewhere as well?
Comment 6•14 years ago
|
||
Colin, if you are far enough along, you'll want to relax this ifdef in nsHTMLDataTransfer.cpp +#ifdef XP_WIN32 + // image pasting from the clipboard is only implemented on Windows right now. + (*aTransferable)->AddDataFlavor(kJPEGImageMime); +#endif
blocking1.9-, this is wanted but not a blocker
Flags: blocking1.9? → blocking1.9-
Assignee | ||
Comment 8•14 years ago
|
||
One thing I noticed in the windows clipboard code is that it gives a meaningless argument string: > rv = encoder->InitFromData(rgbData, 0, width, height, 3 * width /* RGB * # pixels in a row */, > imgIEncoder::INPUT_FORMAT_RGB, NS_LITERAL_STRING("transparency=none")); http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsImageClipboard.cpp#235 For one thing, the correct option is "usetransparency", not just "transparency". And secondly, the JPEG decoder doesn't support that as an option. Should I make a new bug or just attach a patch here. My patch is ready to go, will attach next.
Assignee | ||
Comment 9•14 years ago
|
||
This patch supports decoding: images with the TIFF or PNG pboard type. images that are RGBA or RGB. These are the two most common types of bitmap data. We don't try to decode images we don't know how to, and I've added assertions to try and catch bitmap images we can't decode now, but could in the future. I fixed the windows decoder as well, to not pass a bad format string to the JPEG decoder. Scott, is there any reason to have the #ifdef in nsHTMLDataTransfer.cpp at all, btw? Is there something wrong with asking and not getting anything?
Attachment #278658 -
Flags: review?(joshmoz)
Assignee | ||
Comment 10•14 years ago
|
||
BTW I haven't had the chance to test this on a big endian machine. I have no reason to believe that the logic is wrong there (simple RGBA->ARGB). I'll try to test on my big endian PowerBook soon.
Comment 11•14 years ago
|
||
Comment on attachment 278658 [details] [diff] [review] fix v1.0 Big endian ARGB conversion in one line: result = ((src << 24) | (src >> 8));
Comment 12•14 years ago
|
||
Comment on attachment 278658 [details] [diff] [review] fix v1.0 If I take a screenshot and paste part of that from Preview into a message, that works. But if I open the image "mozilla/widget/src/cocoa/cursors/zoomIn.tiff" in Preview and copy that I can't paste that into the message. Why not?
Assignee | ||
Comment 13•14 years ago
|
||
I rewrote this to use ImageIO. Works with everything I threw at it, and doesn't deal with raw bitmap data anymore (since the data on the clipboard is encoded, usually in TIFF format). MUCH less code and much cleaner too. I also did some digging on NULLs and CoreFoundation. The rule, near as I can tell, is that NULLs propagate freely through CF/CG functions unless otherwise noted (as in ObjC), including functions ending in Release, *except* CFRelease. Very annoying. (Considering just writing a helper macro/inline to take care of that). I fixed a couple of issues (just NULL and success checks) in the code 'Ili recently put in here as well.
Attachment #278658 -
Attachment is obsolete: true
Attachment #279028 -
Flags: review?(joshmoz)
Attachment #278658 -
Flags: review?(joshmoz)
Comment 14•14 years ago
|
||
Comment on attachment 279028 [details] [diff] [review] fix v2.0 - if (flavorStr.EqualsLiteral(kPNGImageMime) || flavorStr.EqualsLiteral(kJPEGImageMime) || - flavorStr.EqualsLiteral(kGIFImageMime)) { - // We have never supported this on Mac OS X, we could someday but nobody does this. + if (flavorStr.EqualsLiteral(kJPEGImageMime)) { You simply removed png and gif here, please leave them in a commented-out "if" block so we don't forget about them. Or implement them, whichever. + CGImageDestinationAddImage(destRef, imageRef, NULL); Please add a comment somewhere around here explaining the null propagation stuff. It looks terrible if you don't know about that (looks like you mistakenly didn't null check destRef). ----------- #ifdef MOZILLA_1_8_BRANCH #define imgIEncoder imgIEncoder_MOZILLA_1_8_BRANCH #endif ----------- In the Windows nsImageClipboard.cpp, you can remove the following lines at the top of the file. Just a little more cleanup. -#ifdef XP_WIN32 - // image pasting from the clipboard is only implemented on Windows right now. +#if defined(XP_WIN32) || defined(XP_MACOSX) + // image pasting from the clipboard is only implemented on Windows & Mac right now. Please just kill off that ifdef. It is OK to request that and just not get it. libeditor shouldn't have to know such details about widget implementations and the ifdef is one more thing to have to remember to maintain.
Attachment #279028 -
Flags: review?(joshmoz) → review-
Comment 15•14 years ago
|
||
"In the Windows nsImageClipboard.cpp, you can remove the following lines" - I mean remove the lines above that comment...
Assignee | ||
Comment 16•14 years ago
|
||
Address review comments. I ended up just making us potentially support PNG and GIF as well, since it was easy.
Attachment #279028 -
Attachment is obsolete: true
Attachment #279622 -
Flags: review?(joshmoz)
Comment 17•14 years ago
|
||
Comment on attachment 279622 [details] [diff] [review] fix v2.1 + else if (flavorStr.EqualsLiteral(kJPEGImageMime) || There is an extra line above this line, after the above "if" block ends. + // Figure out what type we're converting to + CFStringRef outputType = flavorStr.EqualsLiteral(kJPEGImageMime) ? CFSTR("public.jpeg") : + (flavorStr.EqualsLiteral(kGIFImageMime) ? CFSTR("public.png") : + CFSTR("com.compuserve.gif")); I don't really like this use of ?:. Please use if/else if/else. Harder to read than if. + // Use ImageIO to interpret the data on the clipboard and transcode to JPEG. You're not necessarily converting to JPEG - please fix this comment and the name of the "jpegData" variable. + } else if (flavorStr.EqualsLiteral(kJPEGImageMime)) { This is in HasDataMatchingFlavors. Shouldn't you be doing this for png and gif too?
Attachment #279622 -
Flags: review?(joshmoz) → review-
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #279622 -
Attachment is obsolete: true
Attachment #279675 -
Flags: review?(joshmoz)
Comment 19•14 years ago
|
||
Comment on attachment 279675 [details] [diff] [review] fix v2.2 You didn't address my first comment, please fix on checkin. Requesting review from mscott for the editor change.
Attachment #279675 -
Flags: review?(mscott)
Attachment #279675 -
Flags: review?(joshmoz)
Attachment #279675 -
Flags: review+
Assignee | ||
Comment 20•14 years ago
|
||
d'oh sorry. Will fix on checkin.
Comment 21•14 years ago
|
||
Comment on attachment 279675 [details] [diff] [review] fix v2.2 r=bienvenu for the htmleditor.cpp part...
Attachment #279675 -
Flags: review?(mscott) → review+
Attachment #279675 -
Flags: superreview?(roc)
Attachment #279675 -
Flags: approval1.9?
+ else if (flavorStr.EqualsLiteral(kGIFImageMime)) + outputType = CFSTR("public.png"); + else + outputType = CFSTR("com.compuserve.gif"); Is this correct? If so, why are we turning GIFs into PNGs and everything else into GIFs?
Assignee | ||
Comment 23•14 years ago
|
||
(In reply to comment #22) > + else if (flavorStr.EqualsLiteral(kGIFImageMime)) > + outputType = CFSTR("public.png"); > + else > + outputType = CFSTR("com.compuserve.gif"); > > Is this correct? If so, why are we turning GIFs into PNGs and everything else > into GIFs? The first part is a mistake, it should be > + else if (flavorStr.EqualsLiteral(kPNGImageMime)) > + outputType = CFSTR("public.png"); The second part is because the only mime types we handle in that block are JPEG, PNG, GIF: + else if (flavorStr.EqualsLiteral(kJPEGImageMime) || + flavorStr.EqualsLiteral(kPNGImageMime) || + flavorStr.EqualsLiteral(kGIFImageMime)) { I'll post a new patch with the fix for the GIF/PNG mixup. I didn't catch it because right now, nothing handles anything other than JPEG streams :(
Attachment #279675 -
Flags: superreview?(roc)
Attachment #279675 -
Flags: approval1.9?
Assignee | ||
Comment 24•14 years ago
|
||
address roc's sr comments. I didn't change the libeditor portion, so his review should carry over (I think?)
Attachment #279675 -
Attachment is obsolete: true
Attachment #280932 -
Flags: review?(joshmoz)
Updated•14 years ago
|
Flags: in-litmus?
Attachment #280932 -
Flags: superreview?(roc)
Attachment #280932 -
Flags: review?(joshmoz)
Attachment #280932 -
Flags: review+
Attachment #280932 -
Flags: approval1.9?
Attachment #280932 -
Flags: superreview?(roc) → superreview+
Comment 25•14 years ago
|
||
Comment on attachment 280932 [details] [diff] [review] fix v2.2.1 a=mconnor on behalf of drivers
Attachment #280932 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 26•14 years ago
|
||
Checking in editor/libeditor/html/nsHTMLDataTransfer.cpp; /cvsroot/mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp,v <-- nsHTMLDataTransfer.cpp new revision: 1.129; previous revision: 1.128 done Checking in widget/src/cocoa/nsClipboard.mm; /cvsroot/mozilla/widget/src/cocoa/nsClipboard.mm,v <-- nsClipboard.mm new revision: 1.15; previous revision: 1.14 done Checking in widget/src/windows/nsImageClipboard.cpp; /cvsroot/mozilla/widget/src/windows/nsImageClipboard.cpp,v <-- nsImageClipboard.cpp new revision: 1.12; previous revision: 1.11 done
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•