Last Comment Bug 192728 - Add possibility to drag downloaded files from download window and drop/move them to desktop (as example)
: Add possibility to drag downloaded files from download window and drop/move t...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: seamonkey2.1a2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on: 132024 462172 554418
Blocks:
  Show dependency treegraph
 
Reported: 2003-02-11 07:33 PST by Ralf Hauser
Modified: 2010-06-03 16:32 PDT (History)
7 users (show)
jh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
basic functionality (2.68 KB, patch)
2010-03-05 11:57 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
test (8.53 KB, patch)
2010-03-23 12:56 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (2.77 KB, patch)
2010-05-16 10:18 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v3 [Checkin: comment 21] (3.58 KB, patch)
2010-05-20 15:19 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review
test v2 (8.84 KB, patch)
2010-05-24 16:18 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review
test v2a, rs=neil [Checkin: comment 21] (8.82 KB, patch)
2010-05-26 07:12 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Splinter Review
move handlers (2.09 KB, patch)
2010-06-03 13:37 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Splinter Review
move handlers v2 [Checkin: comment 27] (1.92 KB, patch)
2010-06-03 14:03 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review

Description Ralf Hauser 2003-02-11 07:33:51 PST
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 2005-09-19 11:57:38 PDT
The corresponding bug for Firefox is Bug 242204.
Comment 2 Robert Kaiser 2009-08-06 16:14:35 PDT
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 2010-01-25 10:24:56 PST
bug 242204 has been duped to Bug 462172
Comment 4 Phil Lacy 2010-01-25 12:20:13 PST
I'll add this to my todo list of bugs already fixed in TB
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-03-05 11:57:07 PST
Created attachment 430687 [details] [diff] [review]
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) 2010-03-20 16:12:13 PDT
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) 2010-03-23 12:56:24 PDT
Created attachment 434312 [details] [diff] [review]
test

Test, pending bug 554418. Not requesting review until that one is fixed.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2010-03-24 12:08:44 PDT
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 neil@parkwaycc.co.uk 2010-05-16 08:48:22 PDT
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) 2010-05-16 10:18:18 PDT
Created attachment 445611 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-05-16 12:25:59 PDT
(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) 2010-05-16 13:31:02 PDT
(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 neil@parkwaycc.co.uk 2010-05-19 15:57:07 PDT
After all that, saveURL isn't defined, so dropping on DM fails anyway...
Comment 14 Jens Hatlak (:InvisibleSmiley) 2010-05-20 15:19:25 PDT
Created attachment 446587 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-05-21 02:26:13 PDT
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) 2010-05-21 09:58:58 PDT
(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) 2010-05-24 14:51:32 PDT
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) 2010-05-24 16:18:57 PDT
Created attachment 447226 [details] [diff] [review]
test v2

Now that was easy. And tests the Linux requirements. :-)
Comment 19 neil@parkwaycc.co.uk 2010-05-26 05:21:07 PDT
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) 2010-05-26 07:12:57 PDT
Created attachment 447515 [details] [diff] [review]
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) 2010-05-26 08:33:21 PDT
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) 2010-05-26 08:47:17 PDT
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 neil@parkwaycc.co.uk 2010-06-03 09:52:58 PDT
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) 2010-06-03 13:37:17 PDT
Created attachment 449089 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-06-03 13:44:08 PDT
Comment on attachment 449089 [details] [diff] [review]
move handlers

I specifically excluded the onselect handler ;-)
Comment 26 Jens Hatlak (:InvisibleSmiley) 2010-06-03 14:03:02 PDT
Created attachment 449101 [details] [diff] [review]
move handlers v2 [Checkin: comment 27]
Comment 27 Jens Hatlak (:InvisibleSmiley) 2010-06-03 16:32:31 PDT
Comment on attachment 449101 [details] [diff] [review]
move handlers v2 [Checkin: comment 27]

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

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