Closed Bug 567377 Opened 10 years ago Closed 7 years ago

Drag&drop of download items to desktop/external applications doesn't work on Linux

Categories

(Toolkit :: Downloads API, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: InvisibleSmiley, Assigned: jhorak)

Details

Attachments

(1 file, 3 obsolete files)

While working on bug 192728 (which is about porting FF's download manager drag&drop) I found that FF's download drag&drop doesn't work on Linux.

From bug 192728 comment 8:
"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"."

From bug 192728 comment 10:
"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.)"


Effectively you just need to replace this:
    var dt = aEvent.dataTransfer;
    dt.mozSetDataAt("application/x-moz-file", f, 0);
    dt.effectAllowed = "copyMove";

by this:
    var url = Services.io.newFileURI(f).spec;
    var dt = aEvent.dataTransfer;
    dt.mozSetDataAt("application/x-moz-file", f, 0);
    dt.setData("text/uri-list", url + "\r\n");
    dt.setData("text/plain", url + "\n");
    dt.effectAllowed = "copyMove";

I'll leave creating a patch and driving this through to you guys since you'll probably require a test and I have no intention to go through the review hassle. FF has many more devs than SM and I already did the analysis + solution. Have fun.
Attached patch d&d patch 1 (obsolete) — Splinter Review
This is rather old bug, still valid and mentioned fix still works. Attaching patch, please have a look. I'm not sure who to ask for review, feel free to reassign. Part is in browser code and part in toolkit. It might have to be splitted.
Attachment #658045 - Flags: review?(ted.mielczarek)
Comment on attachment 658045 [details] [diff] [review]
d&d patch 1

I'm definitely not the right reviewer here. Punting to Marco, maybe he can review this.
Attachment #658045 - Flags: review?(ted.mielczarek) → review?(mak77)
Assignee: nobody → jhorak
Comment on attachment 658045 [details] [diff] [review]
d&d patch 1

Review of attachment 658045 [details] [diff] [review]:
-----------------------------------------------------------------

There are various traling spaces in the patch that should be removed, please have a look for them.

::: toolkit/mozapps/downloads/content/downloads.js
@@ +726,5 @@
>  
>      var dt = aEvent.dataTransfer;
>      dt.mozSetDataAt("application/x-moz-file", f, 0);
> +    var url = Services.io.newFileURI(f).spec;
> +    dt.setData("text/uri-list", url + "\r\n");

I think the spec refers to CRLF in the sense of "where line termination is needed it should be a CRLF", though that's quite unclear. Also HTML5 dnd specs uses "separated by CRLF", not "terminated by CRLF", there's nothing to separate here.
What I can say for sure is we never added newline terminators, and our documentation does the same, so if it works regardless I'd left them out.
Same comment is valid for the other changes, so could you please verify if it works without the trailing terminators?

@@ +747,5 @@
>      var dt = aEvent.dataTransfer;
> +    // If dragged item is from our source, do not try to 
> +    // redownload already downloaded file.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;

This looks like trying to fix a different bug... Care to explain? Eventually should be split apart to its own bug.
Attachment #658045 - Flags: review?(mak77)
Attached patch d&d patch 2 (obsolete) — Splinter Review
Added fix for test and removed CRLF. D&D is working without additional CRLF.
> @@ +747,5 @@
> >      var dt = aEvent.dataTransfer;
> > +    // If dragged item is from our source, do not try to 
> > +    // redownload already downloaded file.
> > +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> > +      return;
> 
> This looks like trying to fix a different bug... Care to explain? Eventually
> should be split apart to its own bug.

When you start to drag from download manager and you drop also on download manager file is "downloaded" from file://... again because Mozilla accepts drops of text/uri-list or text/plain.
Attachment #658045 - Attachment is obsolete: true
Attachment #666507 - Flags: review?(mak77)
Comment on attachment 666507 [details] [diff] [review]
d&d patch 2

Review of attachment 666507 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please verify if DownloadsIndicatorView.onDrop needs the same kind of fix? It's managing DND on the Downloads panel (the panel opened from the new downloads button in Nightly), you fixed the creation in both the panel and the window, but drop only in the window. So I suspect it's still possible to drag & drop to self in the panel.

::: toolkit/mozapps/downloads/content/downloads.js
@@ +747,5 @@
>      var dt = aEvent.dataTransfer;
> +    // If dragged item is from our source, do not try to
> +    // redownload already downloaded file.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;

I wonder if there's some internal source that may allow drops here with x-moz-file but off-hand I can't think of any... So I think we can take this and if someone knocks at our door we will just add some more data to distinguish.

::: toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul
@@ +66,5 @@
>      state: Ci.nsIDownloadManager.DOWNLOAD_FINISHED}
>  ];
>  
> +function compareStrFunc(actualData, expectedData) {
> +  return expectedData == actualData;

please use ===
Comment on attachment 666507 [details] [diff] [review]
d&d patch 2

just feedback for now, waiting for answer to the previous comment
Attachment #666507 - Flags: review?(mak77) → feedback+
Attached patch d&d patch 3 (obsolete) — Splinter Review
> Could you please verify if DownloadsIndicatorView.onDrop needs the same kind
> of fix? It's managing DND on the Downloads panel (the panel opened from the
> new downloads button in Nightly), you fixed the creation in both the panel
> and the window, but drop only in the window. So I suspect it's still
> possible to drag & drop to self in the panel.

Your right, dragging to button from download manager triggers download again. I've attached fixed patch.
Attachment #666507 - Attachment is obsolete: true
Attachment #670733 - Flags: review?(mak77)
Comment on attachment 670733 [details] [diff] [review]
d&d patch 3

Review of attachment 670733 [details] [diff] [review]:
-----------------------------------------------------------------

It looks fine now, just a small comment.
Thank you for looking into this.

::: browser/components/downloads/content/indicator.js
@@ +517,5 @@
> +    let dt = aEvent.dataTransfer;
> +    // If dragged item is from our source, do not try to
> +    // redownload already downloaded file.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;

I'd put your code at the beginning of the method, mostly cause we don't need name and url until later, so keep vars definition near their first use.
Attachment #670733 - Flags: review?(mak77) → review+
Attached patch d&d patch 4Splinter Review
Patch fixed, thanks for review, copying review+ from previous patch.
Attachment #670733 - Attachment is obsolete: true
Attachment #675025 - Flags: review+
Attachment #675025 - Flags: checkin?
In future please use checkin-needed keyword when requesting a checking (I'm adding it for you now)
Keywords: checkin-needed
Pushed to Try (all tests on Linux & Linux64, and one Mac platform just in case there are unintended non-Linux consequences):
  https://tbpl.mozilla.org/?tree=Try&rev=ec17db31c5b8
Attachment #675025 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/79154a91fb15
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.