Last Comment Bug 428876 - ondrop gets wrong results when dragging multiple files
: ondrop gets wrong results when dragging multiple files
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: mozilla1.9
Assigned To: Michael Ventnor
:
:
Mentors:
Depends on:
Blocks: 229327
  Show dependency treegraph
 
Reported: 2008-04-13 20:26 PDT by Justin Dolske [:Dolske]
Modified: 2008-05-14 12:10 PDT (History)
6 users (show)
ventnor.bugzilla: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.67 KB, patch)
2008-04-14 01:14 PDT, Michael Ventnor
roc: review+
roc: superreview+
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2008-04-13 20:26:03 PDT
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] 2008-04-13 20:40:28 PDT
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] 2008-04-13 20:42:11 PDT
This code was recently added by bug 229327.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-04-13 20:46:24 PDT
Yeah, Michael's regression.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-04-13 21:06:31 PDT
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] 2008-04-13 22:24:38 PDT
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 2008-04-14 01:14:02 PDT
Created attachment 315481 [details] [diff] [review]
Patch

The helper function makes fixing this slightly easier.
Comment 7 Michael Ventnor 2008-04-14 01:14:52 PDT
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 2008-04-14 01:54:29 PDT
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) 2008-04-14 02:11:58 PDT
This doesn't seem to handle multiple files. Does it?
Comment 10 Michael Ventnor 2008-04-14 02:33:47 PDT
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) 2008-04-14 02:37:45 PDT
Comment on attachment 315481 [details] [diff] [review]
Patch

ah right!
Comment 12 Michael Ventnor 2008-04-14 02:55:54 PDT
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] 2008-04-14 03:31:15 PDT
w00t. Tried it out, and fixes my problem. Thanks!
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-14 16:28:22 PDT
Comment on attachment 315481 [details] [diff] [review]
Patch

This definitely needs tests to land.
Comment 15 Michael Ventnor 2008-04-14 16:34:15 PDT
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) 2008-04-14 16:36:34 PDT
Dunno, ask Neil
Comment 17 Justin Dolske [:Dolske] 2008-04-14 16:56:45 PDT
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 2008-04-14 17:33:10 PDT
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] 2008-04-14 19:35:41 PDT
Comment on attachment 315481 [details] [diff] [review]
Patch

a=beltzner, that'll do!
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-04-16 13:29:20 PDT
mozilla/widget/src/gtk2/nsDragService.cpp 	1.25 

Note You need to log in before you can comment on or make changes to this bug.