428876 - ondrop gets wrong results when dragging multiple files

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Justin Dolske [:Dolske]]

Found this with my APNG Edit extension, which lets you drag files into the extension's window (eg, individual PNG frames for the animation). On Windows and OS X this works fine. Linux, not so much. I can see that multiple items are being dropped, but each has the same bogus path.

My nsDragAndDrop callback looks something like:

    onDrop : function (event, dropData, session) {
        var dataList = dropData.dataList;
        for (var i = 0; i < dataList.length; i++) {
            dump("loop")
            var item = dataList[i].first;
            if (item.flavour.contentType == "application/x-moz-file") {
                var file = item.data.QueryInterface(Ci.nsIFile);
                dump(file.path);
            }
        }
     }

Dragging in two files gives output like:

loop
/home/blah/file1file:///home/blah/file2
loop
/home/blah/file1file:///home/blah/file2

Something early on is creating the right number of nsIFiles, but isn't setting the path correctly. Almost looks like a JS array is being used as a string, resulting in all the paths being glued together in a blob.

Comment 1

[Justin Dolske [:Dolske]]

Looks like the problem is in widget/src/gtk2/nsDragService.cpp, nsDragService::GetData()...

473                         // Sometimes there are linebreaks in the file URLs. Yes, linebreaks in the file URLs.
474                         fileUrl.StripChars("\r\n");

The fileURL string here seems to be a new-line separated list of files being dragged. And maybe some extra junk, I get some garbage-like characters after the files, and an assert:

###!!! ASSERTION: Not a UTF-8 string. This code should only be used for converting from known UTF-8 strings.: 'Error', file ../../dist/include/string/nsUTF8Utils.h, line 110

Comment 2

[Justin Dolske [:Dolske]]

This code was recently added by bug 229327.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, Michael's regression.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, I guess it's not a regression, since Michael actually added the first support for dropping files with GTK, so this didn't work before either.

So this bug is really about adding support for multi-file dropping with GTK.

Comment 5

[Justin Dolske [:Dolske]]

Well, yes and no... The onDrop handler I have accepts both x-moz-url and x-moz-file. Without 229327, I can drag multiple files into the window, and they're handled as multiple x-moz-url items. With 229327, I get multiple x-moz-file items but their paths are broken.

And, hmm, I tried commenting out the the x-moz-file entry in my GetSupportedFlavors() callback as a workaround... Works on Linux, but not OS X. Ugh.

Comment 6

[Michael Ventnor]

Attachment: Patch

The helper function makes fixing this slightly easier.

Comment 7

[Michael Ventnor]

Comment on attachment 315481 [details] [diff] [review]
Patch

Hmm, I can't set multiple review. I'll have to ask vlad on IRC and compare his review workload with Roc's.

Comment 8

[Michael Ventnor]

vlad, if you get around to this before Roc, can you also mark sr and cancel my request for Roc?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This doesn't seem to handle multiple files. Does it?

Comment 10

[Michael Ventnor]

It certainly does. The GetData() function is actually called once per item, not once per drag, thats what the index parameter is for, as I found out just now.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 315481 [details] [diff] [review]
Patch

ah right!

Comment 12

[Michael Ventnor]

Comment on attachment 315481 [details] [diff] [review]
Patch

We should take this for platform parity regarding multiple-drag-and-drop. We cant deny dolske his APNG Edit! :-)

Comment 13

[Justin Dolske [:Dolske]]

w00t. Tried it out, and fixes my problem. Thanks!

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315481 [details] [diff] [review]
Patch

This definitely needs tests to land.

Comment 15

[Michael Ventnor]

dolske, roc, do you know of a good way to test this? Do we already have drag-and-drop tests?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Dunno, ask Neil

Comment 17

[Justin Dolske [:Dolske]]

Hmm, not really sure if it can be fully automated, since Mozilla can't really initiate dragging icons from the Gnome desktop. Might have to rely on Litmus for that. Simulating a DND within (between?) a Mozilla window might be possible, although I dunno if that goes through this code.

Comment 18

[Michael Ventnor]

The only way to test this is through Litmus, since it relies on external factors.

1. Open Nautilus, select multiple image files.
2. Drag it to an editor (Thunderbird or Composer works best since Firefox has restrictions on this action)
3. Verify that an <img> is created for each of the image files dragged.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315481 [details] [diff] [review]
Patch

a=beltzner, that'll do!

Comment 20

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/gtk2/nsDragService.cpp 	1.25