192728 - Add possibility to drag downloaded files from download window and drop/move them to desktop (as example)

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, enhancement)

Description

[Ralf Hauser]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030114
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030114

The above URL is a complementary RFE.

It would be great to be able to move/copy a downloaded file from the download
window to any location (i.e. directory in MS file explorer, {emtpy} MS-Word
windows, etc.) where one normally can drop files...

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Hans-Andreas Engel]

The corresponding bug for Firefox is Bug 242204.

Comment 2

[Robert Kaiser]

bug 132024 and bug 242204 are not real blockers but closely related, so let's add them as dependencies to have anyone on the CC list here notified of status changes on the others.

Comment 3

[Philip Chee]

bug 242204 has been duped to Bug 462172

Comment 4

[Phil Lacy]

I'll add this to my todo list of bugs already fixed in TB

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: basic functionality

This implements/ports the basic functionality from FF Bug 462172. No tests, thus not requesting review and not changing assignee or bug status. I hope this helps who ever is brave enough to add tests (e.g. Phil who did the FF bug).

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Looking more closely at the checkin for bug 462172 I found that in fact only one test file was actually specifically for download drag testing [1]. The test uses utils.js which we don't have but we have replacements for the few functions called from there (getDMUI(), getDMWindow()). I was almost done adapting the test for our setup when I realized that synthesizeDragStart() [2] requires passing in an element. For Toolkit's DM that's easy because there each download relates to a richlistitem which has an "id" attribute with a value of "dl" + dlid [3] so that document.getElementById() can be used. For us it's hard (impossible?) because we have a tree and the nsITreeView interface provides no way to get a row element.

To cut a long story short: Unless there's a way to somehow get a XUL treerow we'll have to write our own synthesizeDragStart() that issues synthesizeMouse() calls with configurable x/y coordinates, similar to mouseDblClickOnCell() [4], or extend the existing Toolkit version with optional x/y coordinate start parameters (in which case we could pass in aTree.body as the element).

[1] toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul
[2] testing/mochitest/tests/SimpleTest/EventUtils.js
[3] toolit/mozapps/downloads/content/downloads.js: createDownloadItem()
[4] suite/common/downloads/test/chrome/test_enter_dblclick_opens.xul

Now I've invested several times as much time into porting the test for this in comparison to how long it took to port the basic functionality and the test is not even working yet. Sigh... :-(

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: test

Test, pending bug 554418. Not requesting review until that one is fixed.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 430687 [details] [diff] [review]
basic functionality

Note:
1. If you select multiple downloads, only one is actually handled. Same in FF.
2. I couldn't find a program on Linux that serves as a valid drop target. I tried Nautilus (Gnome), Thunar (XFce), Dolphin (KDE 4), Konqueror (KDE 4), ROX Filer. Works fine on WinXP. Same in FF. Possible hint: ROX Filer says "Sorry - I need a target of type text/uri-list or XdndDirectSave0".

Comment 9

[[e-mail]]

Comment on attachment 430687 [details] [diff] [review]
basic functionality

>+    var selectionCount = 0;
>+    if (gDownloadTreeView && gDownloadTreeView.selection)
>+      selectionCount = gDownloadTreeView.selection.count;
>+    if (!selectionCount)
>+      return;
Nit: Since you don't actually use the selection here, you can test this inline without actually saving the count. (Or are you planning multiple files?)

>+      saveURL(url, name ? name : url, null, true, true);
You don't actually need to pass anything for the second parameter, do you?
Note that download manager doesn't load the script that defines saveURL...

>+        ondragover="gDownloadDNDObserver.onDragOver(event);event.stopPropagation();"
Why here and not in onDragOver?

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to comment #9)
> (From update of attachment 430687 [details] [diff] [review])
> >+    var selectionCount = 0;
> >+    if (gDownloadTreeView && gDownloadTreeView.selection)
> >+      selectionCount = gDownloadTreeView.selection.count;
> >+    if (!selectionCount)
> >+      return;
> Nit: Since you don't actually use the selection here, you can test this inline
> without actually saving the count. (Or are you planning multiple files?)

Not really planning. It'd surely be nice to have in the future, though. Anyway, whoever does that then should be able to figure out how to get the selection count I guess so I inlined it for now.

> >+      saveURL(url, name ? name : url, null, true, true);
> You don't actually need to pass anything for the second parameter, do you?
> Note that download manager doesn't load the script that defines saveURL...

Looking at other examples it seems you're right but I feel that it's cleaner to actually use the information we have. onDrop could be called independent from the corresponding onDragStart, i.e. when the drag is not coming from the download manager (in theory), right? In that case name could be different from what can be extracted from the url.

> >+        ondragover="gDownloadDNDObserver.onDragOver(event);event.stopPropagation();"
> Why here and not in onDragOver?

The usual answer: FF had it like that. Changed (AFAICS they use onDragOver only once, too).


I also found why d&d didn't work on Linux (the ROX error message was really helpful!). Implementing text/uri-list and text/plain in addition is the key. That makes it work in all applications listed in comment 8. I even tried SciTE and KWrite (both text editors) successfully.

<http://mdn.beonex.com/En/DragDrop/Recommended_Drag_Types>
(Note the following under Dragging Files: "If possible, you may also include the file URL of the file using both the text/uri-list and/or text/plain types. These types should be added last so that the more specific application/x-moz-file type has higher priority.")

<http://www.rfc-editor.org/rfc/rfc2483.txt>
(Notes that CRLF is required. Something that ROX will happily tell you, too.)

With that we're better than FF! :-)

I guess I'll file a bug for the red panda later. ;-)

Comment 11

[[e-mail]]

(In reply to comment #10)
> (In reply to comment #9)
> > >+      saveURL(url, name ? name : url, null, true, true);
> > You don't actually need to pass anything for the second parameter, do you?
> > Note that download manager doesn't load the script that defines saveURL...
> Looking at other examples it seems you're right but I feel that it's cleaner to
> actually use the information we have. onDrop could be called independent from
> the corresponding onDragStart, i.e. when the drag is not coming from the
> download manager (in theory), right? In that case name could be different from
> what can be extracted from the url.
Sorry, I misled you here - I was thinking of the case when we have no name.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #11)
> Sorry, I misled you here - I was thinking of the case when we have no name.

Changed to saveURL(url, name, null, true, true); locally.

Comment 13

[[e-mail]]

After all that, saveURL isn't defined, so dropping on DM fails anyway...

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3 [Checkin: comment 21]

(In reply to comment #13)
> After all that, saveURL isn't defined, so dropping on DM fails anyway...

Fixed by including contentAreaUtils.js (cf. FF's downloads.xul). Also added ondrop attribute to the tree element.

Comment 15

[[e-mail]]

Comment on attachment 446587 [details] [diff] [review]
patch v3 [Checkin: comment 21]

[I wonder why the special save link as filename guessing code doesn't live in contentAreaUtils.js]

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #10)
> I guess I'll file a bug for the red panda later. ;-)

Filed bug 567377.

Neil, since your review queue is pretty long, should I move the test review request to IanN?

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 434312 [details] [diff] [review]
test

Test doesn't pass anymore:

failed | Checking for Real file match - got [object DataTransfer], expected null
passed | Drag start did not return item for missing file

Back to the drawing board, canceling review request until I have something better. :-(

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Attachment: test v2

Now that was easy. And tests the Linux requirements. :-)

Comment 19

[[e-mail]]

Comment on attachment 447226 [details] [diff] [review]
test v2

rs=me (I just skimmed the patch, I didn't actually try it or anything...)

>+      var win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
Shouldn't have to do this, as windows have class info.

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

Attachment: test v2a, rs=neil [Checkin: comment 21]

(In reply to comment #19)
> (From update of attachment 447226 [details] [diff] [review])
> rs=me (I just skimmed the patch, I didn't actually try it or anything...)

Fortunately I did. ;-)

> >+      var win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> Shouldn't have to do this, as windows have class info.

It works without the QI, as in the other tests, so I removed it. Also added the two new MIME types to the first comment ("Assure download manager...").

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 446587 [details] [diff] [review]
patch v3 [Checkin: comment 21]

http://hg.mozilla.org/comm-central/rev/6ca437a04a93

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Note: While testing this locally with my self-compiled build (VS2005/SP1, -O2, among others) I ran into a repeatable crash:
<http://crash-stats.mozilla.com/report/index/808bc513-f9e2-4201-a4fc-c384d2100526>
This might be the same as described in this forum thread:
<http://forums.mozillazine.org/viewtopic.php?f=42&t=1708215>
which points to Bug 526038 which is still open.

I checked in anyway since official nightly builds are not affected by that somewhat unrelated issue (verified by replacing the downloadmanager.* files in the current nightly build's comm.jar and d&d'ing to and from the DM successfully).

Comment 23

[[e-mail]]

Comment on attachment 446587 [details] [diff] [review]
patch v3 [Checkin: comment 21]

>   <tree id="downloadTree"
>         flex="1" type="downloads"
>         class="plain"
>         context="downloadContext"
>         enableColumnDrag="true"
>         ondblclick="goDoCommand('cmd_open');"
>+        ondragstart="gDownloadDNDObserver.onDragStart(event);"
>+        ondragover="gDownloadDNDObserver.onDragOver(event);"
>+        ondrop="gDownloadDNDObserver.onDrop(event);"
We need to move these handlers to the <treechildren> element, since currently I can't drag the scrollbar :-(

Comment 24

[Jens Hatlak (:InvisibleSmiley)]

Attachment: move handlers

(In reply to comment #23)
> We need to move these handlers to the <treechildren> element, since currently I
> can't drag the scrollbar :-(

Strangely I could only reproduce this once per profile. Anyway, with this patch it's fixed for me for new profiles.

Comment 25

[[e-mail]]

Comment on attachment 449089 [details] [diff] [review]
move handlers

I specifically excluded the onselect handler ;-)

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

Attachment: move handlers v2 [Checkin: comment 27]

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 449101 [details] [diff] [review]
move handlers v2 [Checkin: comment 27]

http://hg.mozilla.org/comm-central/rev/49017fdba4ba