332912 - Need Cocoa nsIClipboard implementation

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stuart Parmenter]

We need to have a Cocoa nsIClipboard impl so that we don't have to rely on the old APIs.

Comment 1

[Henri Sivonen (:hsivonen)]

To self per discussion with Josh.

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Initial .h

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Initial .mm

Just backing up code so I don't lose it.

I've dealt with plain text copy paste and bitmap copy. 

Yesterday and today, I've been looking into doing HTML in an interoperable way. The lack of docs in this area totally sucks, so I've gone to other people's computers with a pasteboard dumper to find out what different apps do.

I think supporting pasting from Word is reasonable and I should try to make it work. Also, I think I can make copying and pasting between Gecko and Dreamweaver work. (In fact, I think I should just use the Dreamweaver flavor in Gecko-to-Gecko transfers.) I think copying from Gecko to GoLive is easy enough to throw in. Pasting from GoLive to Gecko isn't as easy, and I am not keen on supporting it. I don't want to support HTML copy paste to and from Carbon Gecko, but if someone convinces me that copying from Cocoa Firefox/Camino to Carbon Nvu is important, I might do it in that direction.

Also, I think supporting the Safari-compatible Web Archive format would make sense, which would require linking with WebKit and require OS X 10.3.9 or later. But that shouldn't be a problem, since we are not supporting 10.2 anymore and we shouldn't support people who don't bother installing the free system updates.

Comment 4

[Henri Sivonen (:hsivonen)]

Noting down that bug 137450 affects Mac when CF_HTML export is supported.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Diff for existing files

(Will attach new file separately. I am having problems with cvs diff.)

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: mozilla/widget/src/cocoa/nsClipboard.h

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: mozilla/widget/src/cocoa/nsClipboard.mm

Comment 8

[Henri Sivonen (:hsivonen)]

As far as I can tell, this works for Cut/Copy/Paste. However, this breaks drag&drop, so it doesn't make sense to check this in without a companion implementation for bug 332913. However, I am unable to fix that bug, so I am giving this bug back to Josh.

Documentation: http://hsivonen.iki.fi/kesakoodi/clipboard/

Comment 9

[Josh Aas]

Attachment: fix v1.0

I don't really like trying to special case apps - its a game we can't win and it complicates things a lot.

When I thought hsivonen had stopped working on his patch, I started my own and it is done now. This implementation works really well for me and it is easy to understand. I think we should go this simpler route now.

There are things like cairo image copy support that can be added, but we can do it later. I'd like to get this in to serve as something to build on.

Comment 10

[Henri Sivonen (:hsivonen)]

I am quite surprised by the response. I said I wasn't working on drag&drop. I was working on clipboard as previously discussed.

As for the code itself, my implementation achieves platform parity with Windows in terms of MS Office compatibility. It also achieves compatibility within the platform by supporting Apple’s Web Achive flavor. Is that really something we don’t want? (And being able to copy to Dreamweaver and GoLive would be a nice extra for Web developers.)

Comment 11

[Josh Aas]

I don't think we should have to write code to support specific applications here. If an app wants to deal in HTML, we should implement support for exporting and importing NSHTMLPboardType and its carbon equivs (as should office). What happens when the next version of office doesn't use CF_HTML and does some totally different thing? I think we're safer and more predictable by not special casing. Another thing about special casing is that in most people's eyes it makes it our bug every time something doesn't work as you'd expect with copy/paste.

Comment 12

[Henri Sivonen (:hsivonen)]

For all practical purposes, NSHTMLPboardType *is* CF_HTML. Gecko supports CF_HTML on Windows. Why not on Mac? The Web Achive format is not specific to WebKit. It is the new platform "standard".

And if there's Office and WebKit interop, what's wrong about having Dreamweaver and GoLive interop? Isn't interop good?

> Another thing about special casing is that in most people's
> eyes it makes it our bug every time something doesn't work as 
> you'd expect with copy/paste.

I guess that depends on whether people expect the HTML structure to be preserved on copy/paste or not. Since Safari does export Web Archive to the clipboard, with time Mac user might come to expect HTML copy/paste to work--just like Windows users today expect it to work.

Comment 13

[Josh Aas]

I'm all for interoperability, who isn't. But I'm not for encouraging applications to make up their own clipboard formats and expect everyone else to add a bunch of code and complexity to their own applications to support it (which then becomes a maintenance burden and legacy baggage). I'd be happy to add support some form of HTML on the clipboard, but I think we should have a discussion about what the best way to do that is and we can add it more easily to my patch than we can yours. I wish we had talked more before you wrote that entire patch!

Comment 14

[Josh Aas]

Attachment: fix v1.1

Comment 15

[Josh Aas]

+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 1998

Yeah yeah, I gotta fix that.

Comment 16

[Stuart Morgan]

I've started looking at this, but I'll need to ping you offline about testing it.  In general, is the reason that there's so much less futzing around with putting things on the clipboard just that vending ASCII text data isn't really important in the modern world like it was under classic?

Comment 17

[Josh Aas]

Exactly - throw a unicode NSString on the clipboard and it'll do the right thing for carbon apps that ask for carbon string types.

Comment 18

[Stuart Morgan]

Comment on attachment 240105 [details] [diff] [review]
fix v1.1

r=me. Looks good, and seems to work fine in Camino.  Minor stuff:

>+  if (aWhichClipboard != kGlobalClipboard || !mTransferable)

Could you parenthesize here and in the other uses of this condition? (I'd have to look this up to be sure which way the precedence goes here.)

>+  NSMutableDictionary* pasteboardOutputDict = [NSMutableDictionary dictionaryWithCapacity:2];

Why 2? I don't see a code path with more than 1 element.

>+        NSString* availableType = [generalPBoard availableTypeFromArray:[NSArray arrayWithObject:NSStringPboardType]];
>+        if (availableType && [availableType isEqualToString:NSStringPboardType]) {
...
>+        NSString* availableType = [generalPBoard availableTypeFromArray:[NSArray arrayWithObject:lookingForType]];
>+        if (availableType && [availableType isEqualToString:lookingForType]) {

Are the |isEqualToString:|s necessary? If you only give it one type, and it comes back with a match, isn't it guaranteed to be the one you gave?  I also wonder if it might be better to build an NSArray from the aFlavorList and call availableTypeFromArray: only once, but I can definitely see good arguments for doing it your way.

Comment 19

[Stuart Parmenter]

Comment on attachment 240105 [details] [diff] [review]
fix v1.1

Please add code to support MOZ_CAIRO_GFX for images and please implement the "// implement me" code.

what else is missing from this impl?  i haven't had a chance to try testing with it yet.

Comment 20

[Josh Aas]

There is nothing unimplemented in this patch that isn't unimplemented in Carbon.

Comment 21

[Josh Aas]

Attachment: fix v1.2

Same thing as v1.1 but also addresses smorgan's comments, fixes license headers, and adds support for Cairo.

Comment 22

[Josh Aas]

+        if (modulo == 0)
+          reorderedData[i + 3] = imageData[i];
+        else if (modulo == 1)
+          reorderedData[i - 1] = imageData[i];
+        else if (modulo == 2)
+          reorderedData[i - 1] = imageData[i];
+        else if (modulo == 3)
+          reorderedData[i - 1] = imageData[i];
+      }

Where I did this, obviously everything but the first case can just be handled in a single "else" statement. I just forgot to collapse that when I got it working.

Comment 23

[Stuart Parmenter]

You'd be _much_ better off changing that entire loop in to something that accesses the array as uint32* and uses shifts on the original data...

Comment 24

[Josh Aas]

Attachment: fix v1.3

Uses shifting to rearrange pixels.

Comment 25

[Stuart Parmenter]

Comment on attachment 240960 [details] [diff] [review]
fix v1.3

>+      // We have to reorder data to have alpha last because only Tiger can handle
>+      // alpha being first.
>+      PRUint32 wordLength = ((stride * height) / 4);
>+      for (PRUint32 i = 0; i < wordLength; i++) {
>+        PRUint32 pixel = imageData[i];
>+        reorderedData[i] = (pixel << 8) | (pixel >> 24);
>+      }
>+

Maybe rename wordLength to something like imageLength?  wordLength to me implies sizeof(WORD)

Comment 26

[Joel Craig]

Something's strange with BBCode now. Take a look at this forum post:

http://forums.mozillazine.org/viewtopic.php?p=2521539&highlight=#2521539

In keeping this thread updated, I "Copy Link Location" from Bonsai and paste into the response. 

Using homegrown Intel trunk build from this AM.

Comment 27

[Adam Guthrie]

For the record, "fix v1.3" was checked into trunk on 2006-10-02 13:28.

Comment 28

[Wevah (Nate Weaver)]

(In reply to comment #26)
> Something's strange with BBCode now. Take a look at this forum post:
> 
> http://forums.mozillazine.org/viewtopic.php?p=2521539&highlight=#2521539
> 
> In keeping this thread updated, I "Copy Link Location" from Bonsai and paste
> into the response. 
> 
> Using homegrown Intel trunk build from this AM.
> 

Looks like it's prepending the Unicode BOM, which is screwing up the BBCode, since it's the codepoint for "zero width no-break space".

Comment 29

[Joel Craig]

(In reply to comment #28)
> (In reply to comment #26)
> > Something's strange with BBCode now. Take a look at this forum post:
> > 
> > http://forums.mozillazine.org/viewtopic.php?p=2521539&highlight=#2521539
> > 
> > In keeping this thread updated, I "Copy Link Location" from Bonsai and paste
> > into the response. 


> > 
> > Using homegrown Intel trunk build from this AM.
> > 
> 
> Looks like it's prepending the Unicode BOM, which is screwing up the BBCode,
> since it's the codepoint for "zero width no-break space".
> 

Yeah, it's prepending something. I just copy/pasted a bug number into Bugzilla and got "355192"

Comment 30

[Josh Aas]

I forgot to note that I did check this in on trunk. Fixed.

Comment 31

[froodian (Ian Leue)]

Comments 26, 28, and 29 are now filed as bug 355311