332913 - Need Cocoa drag and drop implementation

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stuart Parmenter]

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

Comment 1

[Henri Sivonen (:hsivonen)]

Is anyone actively working on this? I think it would make sense to introduce a constant (like the selection clipboard constant on Linux) for the drag clipboard so that the nsIClipboard impl could be used for moving stuff between nsITransferable and NSPasteboard for dragging as well.

Comment 2

[Henri Sivonen (:hsivonen)]

It turns out that Cocoa does not guarantee that the clipboard designated for drag operations is actually used for such operations, so the constant won't work. :-(

Comment 3

[Josh Aas]

Attachment: fix v0.2

This is a milestone backup of my work in progress. With this patch, dragging files and text from other applications *into* Mozilla basically works. Almost nothing else works. This is just here as a safe place for me to store and reference it, but if anyone wants to comment on the general approach so far go for it.

Comment 4

[Josh Aas]

Attachment: fix v0.7

This is a milestone backup of my work in progress. Everything works with this patch (under regular Cocoa widgets and Cairo-Cocoa). All I have left to do is more testing, cleanup, and code auditing.

Comment 5

[Josh Aas]

Attachment: camino fix v0.7

Fix v0.7 alone will not work with Camino. This is the Camino-specific part of the patch.

Comment 6

[Josh Aas]

Attachment: fix v1.0

This should be ready to go. I plan to follow this patch up with one that make the drag service and the clipboard share some code.

If you apply this patch to Camino, you'll need to also apply the Camino patch on this bug (camino fix v0.7). I'll request review on that once this patch has all of its reviews and then land them at the same time.

Comment 7

[Samuel Sidler (old account; do not CC)]

Comment on attachment 241139 [details] [diff] [review]
fix v1.0

Targeting the right smorgan.

Comment 8

[Josh Aas]

Note to self - need to appply fix for 355311 to this patch as well.

Comment 9

[Joel Craig]

With these 2 patches applied: if I drag an image from a webpage to the Desktop, the resulting file is a Picture Clipping rather than the actual jpg or gif image file. Right-click/Save Image downloads the proper image file.

Comment 10

[Wevah (Nate Weaver)]

Needs to implement promised file dragging.

Comment 11

[Neil Deakin]

Comment on attachment 241139 [details] [diff] [review]
fix v1.0

>+  // create the image for the drag, it isn't awesome but it'll do for now
>+  NSRect dragRect = GetDragRect(aDOMNode);
>+  NSImage* image = [[NSImage alloc] initWithSize:dragRect.size];
>+  [image lockFocus];
>+  [[NSColor grayColor] set];
>+  NSBezierPath* path = [NSBezierPath bezierPath];
>+  [path setLineWidth:2.0];
>+  [path moveToPoint:NSMakePoint(0, 0)];
>+  [path lineToPoint:NSMakePoint(0, dragRect.size.height)];
>+  [path lineToPoint:NSMakePoint(dragRect.size.width, dragRect.size.height)];
>+  [path lineToPoint:NSMakePoint(dragRect.size.width, 0)];
>+  [path lineToPoint:NSMakePoint(0, 0)];
>+  [path stroke];
>+  [image unlockFocus];
>+

This needs to also take into account the aDragRgn passed in, otherwise you'll get a drag rectangle the size of a tree rather than the specific row you are dragging. You'll see this when dragging bookmarks in the tree around.

Comment 12

[Josh Aas]

Good to know that is a case where it matters - I couldn't find any case that actually gave a drag region. I'll add that.

Comment 13

[Josh Aas]

Attachment: fix v1.1

This includes the following fixes:
- fix for bug 355311
- fix for custom type drag functionality (e.g. bookmarks)
- fix for obtaining drag rect from drag region passed into drag invocation

I still need to add promised file type support.

Comment 14

[Joel Craig]

(In reply to comment #13)
> Created an attachment (id=241803) [edit]
> fix v1.1
> 
> This includes the following fixes:
> - fix for bug 355311
> - fix for custom type drag functionality (e.g. bookmarks)
> - fix for obtaining drag rect from drag region passed into drag invocation
> 
> I still need to add promised file type support.
> 

Just confirming, is fix v0.7 still a prerequisite for this new patch?

Comment 15

[Josh Aas]

"camino fix v0.7" is still required if you want to build Camino with Cocoa drag'n'drop.

Comment 16

[Joel Craig]

(In reply to comment #15)
> "camino fix v0.7" is still required if you want to build Camino with Cocoa
> drag'n'drop.
> 

Thanks Josh. I just posted an Intel build with the new patch if anyone needs something to hammer on.

http://homepage.mac.com/joelcraig23/FileSharing2.html
Its the 10 Oct 06 i686.dmg build

Comment 17

[Josh Aas]

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

Comment 18

[Wayne Woods]

(In reply to comment #13)
> Created an attachment (id=241803) [edit]
> fix v1.1

For Firefox, should the 1.1 patch require anything else to be landed concurrently? I patched the latest trunk on my cvs with it, and Fx now gives build errors. I've updated and tried several times. The error begins with:
  /usr/bin/ld: Undefined symbols:
  CreateStylFromScriptRuns(ScriptCodeRun*, unsigned long, char**, int*)
etc.

I'm using MacOSX10.4u.sdk, in case that makes a difference.

Comment 19

[Josh Aas]

My guess is that you applied the patch to an already-built tree and then tried to rebuild. The patch will break if you leave certain dependencies in place, so here is what I suggest you do to build this patch if you don't want to go into your objdir and start deleting object files and .deps files:

1. Pull source code only via "make -f client.mk checkout"
2. Apply fix v1.1
3. Build via "make -f client.mk build"

That just breaks up a normal "make -f client.mk" command into two steps so you can apply a patch in the middle and not have any deps created to deal with.

Comment 20

[Wayne Woods]

(In reply to comment #19)
> 1. Pull source code only via "make -f client.mk checkout"
> 2. Apply fix v1.1
> 3. Build via "make -f client.mk build"

Actually, that's exactly how I did it (and have tried again a few times since, with no luck). I'm not sure what's going wrong. But I don't want to spam this bug further with my build problems... I just wanted to be sure the patch was a standalone. I'll keep trying to work out what the problem is. Thanks for your help :)

Comment 21

[philippe (part-time)]

Using Camino and these 2 patches.
Dragging a .webloc file from the desktop to an open window/tab results in a blank page, the url bar shows: 'file:///Users/phiw/Desktop/filename'. Expected: load the page.
Dragging the same .webloc file to the Dock icon works as expected.

Comment 22

[Stuart Morgan]

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

>+  globalDragPboard = [sender draggingPasteboard];

Storing an autoreleased variable in a global across functions scares me.

>+  NSPoint dragLocation = [aSender draggingLocation];
>+  dragLocation = [[self window] convertBaseToScreen:dragLocation];
>+  FlipCocoaScreenCoordinate(dragLocation);

Move this down to after the action stuff, so it's set up where it's used

>+    PRUint32 action = nsIDragService::DRAGDROP_ACTION_MOVE;
>+    // force copy = option, alias = cmd-option, default is move
>+    if (modifierFlags & NSAlternateKeyMask) {
>+      if (modifierFlags & NSCommandKeyMask)
>+        action = nsIDragService::DRAGDROP_ACTION_LINK;
>+      else
>+        action = nsIDragService::DRAGDROP_ACTION_COPY;
>+    }

This might be a bit clearer with DRAGDROP_ACTION_MOVE as an else

>+  // globalDragPboard must be non-null at this point since it was set
>+  // when this drag session started. Make sure it is up to date by
>+  // setting it with the current pasteboard. Just being paranoid.

Per IRC, I think these should go. The added work doesn't really seem to buy any safety in the case of someone changing what pasteboard they are using in mid-drag--and that seems really improbable.

>+// Note that for text drags, this returns an incorrect rect. This bug
>+// exists in the old carbon implementation too.

It might be nice to expand on this a bit, or just reference a bug number that has details.

>+    // printf("drag region is x=%d, y=%d, width=%d, height=%d\n", x, y, width, height);

I'm guessing this and the other commented out printfs should go?

>+static nsresult SetUpDragClipboard(nsISupportsArray* aTransferableArray)

Ideally, this should all be shared with the recent clipboard code, but that could be done in a follow-up if it's a pain.

>+  NSImage* image = [[NSImage alloc] initWithSize:dragRect.size];

This is leaked.

+      NSString* filePath = [pFiles objectAtIndex:aItemIndex];

What's going on here? The header says that parameter is "the clipboard to which this operation applies", which doesn't seem like what it's used for.  I'm assuming this is right, but a comment explaining the apparent discrepancy with the header would be good.

>+      unsigned int dataLength = [filePath length] * sizeof(PRUnichar); // in bytes
>+      PRUnichar* clipboardDataPtr = (PRUnichar*)malloc(dataLength + sizeof(PRUnichar));

clipboardDataPtr is leaked.  It also seems like it would be cleaner to have something more like
  unsigned int stringLength = [filePath length];
  PRUnichar* clipboardDataPtr = (PRUnichar*)malloc((stringLength + 1) * sizeof(PRUnichar));

>+      clipboardDataPtr[dataLength / 2] = 0;

What's the magic /2 here?  Is (assuming the above code)
  clipboardDataPtr[stringLength] = 0; // or maybe '\0'
the goal here?

>+      NSData* stringData = [pString dataUsingEncoding:NSUnicodeStringEncoding];
>+      unsigned int dataLength = [stringData length];
>+      unsigned char* clipboardDataPtr = (unsigned char*)malloc(dataLength);
>+      [stringData getBytes:(void*)clipboardDataPtr];

Why is the explicit null terminator (assuming I was understanding) needed above but not here?


I haven't had time to build and test yet, but I didn't want the above to have to wait on that.

Comment 23

[Josh Aas]

Attachment: fix v1.2

So the three things we know we still need to do:

1. Promised file dragging
2. Consolidate some code with clipboard
3. Support for webloc files

I'd rather do that stuff after this lands because the size of this patch makes it difficult to work with and makes changes to it hard to review.

Comment 24

[Josh Aas]

> >+    // printf("drag region is x=%d, y=%d, width=%d, height=%d\n", x, y, width, height);
> 
> I'm guessing this and the other commented out printfs should go?

I'd like to leave them in there so in the future you can debug that stuff by just uncommenting the printfs.

> +      NSString* filePath = [pFiles objectAtIndex:aItemIndex];
> 
> What's going on here? The header says that parameter is "the clipboard to which
> this operation applies", which doesn't seem like what it's used for.  I'm
> assuming this is right, but a comment explaining the apparent discrepancy with
> the header would be good.

I don't understand this comment.

> clipboardDataPtr is leaked.

I forgot to look into this when I posted the updated patch. I'll get it on the next rev or on checkin.

> >+      NSData* stringData = [pString dataUsingEncoding:NSUnicodeStringEncoding];
> >+      unsigned int dataLength = [stringData length];
> >+      unsigned char* clipboardDataPtr = (unsigned char*)malloc(dataLength);
> >+      [stringData getBytes:(void*)clipboardDataPtr];
> 
> Why is the explicit null terminator (assuming I was understanding) needed above
> but not here?

It appears as though its just not needed. Things work fine without it, they don't work fine without it in the other case.

Comment 25

[Stuart Morgan]

>+  globalDragPboard = [[sender draggingPasteboard] retain];

It would be good to precede that with a [globalDragPboard release], just to make sure there's no leak if something weird happens.

Other than that and the leak, the code looks fine (although it would be good to clean up the inconsistencies between |Foo* bar|, |Foo *bar|, and |Foo * bar|).


I built Camino with this and played around a bit, and noticed some regressions:
1) I can't drag files into the content area to open them anymore
2) If I drag a bookmark/favicon into the content area, the drag freaks out when I hit a text field; the current drag image freezes in place and a new one appears at the drag start location.  Dropping at that point places the URL into the text field twice
3) Dragging into the content area doesn't give a + icon anymore (any reason you are using NSDragOperationGeneric instead of Copy?)

3 isn't a big deal in the short term, but 1 and 2 are significant regressions that it would be nice to fix before landing this.  I still haven't built it with Firefox to know if those issues are something that need addressing in Camino or the main code, but I'm guessing from the Camino changes that the expectation is that everything should Just Work from Camino's standpoint.

Comment 26

[Josh Aas]

Dragging files into the content area to open them works fine for me. You can't drag files from the Desktop to a text area, but you couldn't do that before this patch either.

As for the messed up dragging into text fields in Camino, you don't have to be over a text field. I can go to slashdot and drag a bookmark from the bookmark bar and wave it back and forth between the bookmark bar and the heading banner on slashdot and get it to happen. I have no idea what is going on, working on it.

Comment 27

[Josh Aas]

Attachment: camino fix v1.0

Same as v0.7 but kills some code I forgot to kill in v0.7.

Comment 28

[Josh Aas]

Attachment: fix v1.3

Addresses the memory leak I forgot to fix in v1.2 and releases the drag clipboard in when dragging enters in case something goes wrong.

Comment 29

[Joel Craig]

Intel build, new tree, and the 2 new patches; some observations:

1. Dragging a file (tried with webloc, jpg and txt files) into either a blank tab or window does not always open the file, although file path appears in URL bar and file name appears in heading. Closing tab or window and re-dragging file into content area opens the file (cached?)

2. RE: file dragging: dragging picture off a page to the Desktop still not working and leaves a picture clipping rather than the file; only now, the rectangle freezes over the drop area when click is released, and stays stuck until cursor is moved.

3. First patch applied cleanly; the second patch was successful but with offsets (whatever that means ;-) )

Comment 30

[philippe (part-time)]

(In reply to comment #29)

> 1. Dragging a file (tried with webloc, jpg and txt files) into either a blank
> tab or window does not always open the file, although file path appears in URL
> bar and file name appears in heading. Closing tab or window and re-dragging
> file into content area opens the file (cached?)

(tested with patched Camino on PPC, 10.4.8)
* Dragging an image (tested: gif/jpg/png) works on my side, but Camino suffers from bug 350331 (resizing the window fixes it, at second try works correctly, probably because the image is cached).
* Dragging a .txt file works correctly.
* dragging a .webloc file fails (same as in my comment #21). On second try, Camino displayed an unstyled xml file.

For some reason I was hoping this would fix bug 354371 in Camino, but I still could repro some of those glitches.

Now I'll try to build Minefield with that patch.

Comment 31

[philippe (part-time)]

Patched Minefield build (non-cairo) running on 10.4.8 PPC
Build made with GCC 4.0, MacOSX10.4u.sdk

* dragging of .webloc files from Desktop to tab/window: no problems
* dragging of .txt files: no problems

But
1. dragging of images (.jpg, .png, .gif) from the Finder (Desktop of a folder somewhere on the HD): I see the same problem as with Camino (image does not appear, resize the window and it shows up - OR - drag image, click in location bar and hit return). Once the image has been viewed (cached) no problems anymore.
2. .html files
a. files containing only text: no problems
b. files containing text and an embedded image (using the <img /> element): blank page, same solution as with images above.
c. files containing text and a background image: no problems.

Comment 32

[Stuart Morgan]

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

Code looks good; r=me.

So we need new bugs for:
1. Promised file dragging
2. Consolidate some code with clipboard
3. Support for webloc files
4. Using the copy cursor for copy drags
5. Camino's weird DnD issues.

Comment 33

[Stuart Morgan]

Comment on attachment 243353 [details] [diff] [review]
camino fix v1.0

>-  nsIDragHelperService* mDragHelper;
>   NSPoint               mLastTrackedLocation;
>   NSWindow*             mLastTrackedWindow;

Axe those other two variables as well (they are only used in the removed code) and r=me.

Comment 34

[Mike Pinkerton (not reading bugmail)]

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

rs=pink

Comment 35

[Josh Aas]

Fixed on trunk, also landed Camino fix. I will file bugs for the issues listed in comment #32. Big thanks to smorgan for the thorough review!

Comment 36

[Josh Aas]

I filed bug 358093, bug 358094, and bug 358095 for various cocoa drag and drop issues.

Can somebody who understands the webloc issues better please file a bug on that?

Comment 37

[Wayne Woods]

Josh,

One thing, other than what's been mentioned... I still can't drag the favicon from the location bar to the tab bar and have it load the url into a tab. Is this actually covered by any other spin-off bugs that I'm not aware of, or should I open a new one? Cheers :)

Comment 38

[philippe (part-time)]

I filed bug 358340 for the .webloc issue.

Comment 39

[Gulliver]

Next week we see the 2 months anniversary for this bug (Built from Sept. 27th is the last where drag&drop works).

Any news here or in any other of the 20 associated/dependant bugs?

Comment 40

[Stuart Morgan]

The news is that this bug was fixed three weeks ago. If you have specific issues, you should be watching the bugs that were filed about them, or file new bugs if there aren't bugs for your issues.

Comment 41

[Timothy Huang]

As I check the latest built (Nov. 18, 2006), two major bugs were not fixed at all:

1) Missing menu items in SeaMonkey Menu, missing Hide SeaMonkey, etc.

2) Can NOT drage a letter into subfolder.

Look like nobody cares about these very important issue.

Comment 42

[Wayne Woods]

(In reply to comment #41)
> As I check the latest built (Nov. 18, 2006), two major bugs were not fixed at
> all:
> 
> 1) Missing menu items in SeaMonkey Menu, missing Hide SeaMonkey, etc.
> 
> 2) Can NOT drage a letter into subfolder.

Read comment 40 again. You've already filed new bugs for the two things you mentioned, so it was redundant to complain here that they're not fixed yet (especially since one of them isn't a drag and drop issue, and is completely unrelated to this bug). If you really think they've been forgotten, politely commenting in the relevant bug pages is the way to go.

Please only comment further here if you discover a new bug that is related to this one, and want to add a block/dependency for it.

> Look like nobody cares about these very important issue.

The switch to Cocoa is a radical change and I'm sure Josh and co. are fixing related bugs as fast as they can. It doesn't mean that nobody cares about the unfixed bugs, it just means there are a lot of clean-up bugs to attend to, each no less important than the ones you filed.

Something else you should be aware of, that has been reiterated 1000s of times... the Mozilla trunk is expected to become unstable every now and then, and unpleasant bugs can last for many, many months during radical changes. If that's not acceptable to you, you really should be using a release or a branch (Moz 1.8) build, *not* a trunk (1.9) one.