142798 - Should be able to copy url from download manager

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, enhancement)

Description

[Petr Cerny [:hrosik]]

1) download manager should be able to copy URL of a selected file into the
clipboard.
2) DM doesn't show destination path of downloaded files.
3) properties window should open for downloaded (finished) files also not only
for the ones being downloaded.
4) sorting doesn't work, files are always sorted by progress

(Win95; rv:1.0rc2; Gecko/20020506)

Comment 1

[Robert Wall]

Please do not file more than one issue per bug and try to do a thorough search
for duplicates before filing the bugs.  

2) is covered by bug 135805
3) is covered by bug 139606
4) is covered by bug 134824

If you want to do 1), it is possible to copy the downloaded file name from the
properties box during a download.  Highlight the name, right click with the
mouse and choose Copy from the menu.  

Comment 2

[Boris Zbarsky [:bzbarsky]]

Focusing on issue #1.

Comment 3

[Dimitrios]

clarifying summary a bit.

Comment 4

[Peter Tandler]

*** Bug 209159 has been marked as a duplicate of this bug. ***

Comment 5

[Christian Franke]

Attachment: Patch: Copy URL from "Source" col to clipboard

Function copyToClibboard is based on copyBookmark from bookmarks.js. A
text/unicode and a text/html version is copied.

The key_copy (^C on Win) mapping is there, but does not WFM. This is possibly
related to Bug 198247 or Bug 202005 (Haven't tried the patches attachment
128325 [details] [diff] [review] or attachment 128326 [details] [diff] [review])

Comment 6

[[e-mail]]

I don't see how the html version is useful...

Comment 7

[R.K.Aa.]

*** Bug 241308 has been marked as a duplicate of this bug. ***

Comment 8

[Dimitrios]

If the html-copy is omitted, would the patch be ready for approval? Not
necessary for Firefox branch but I think it would be useful for the trunk. The
alternative solution is a bit cubersome (needs some keyboard & mouse clicking in
order to mark and copy the entire url).

Comment 9

[Christian Franke]

(In reply to comment #8)
> ... (needs some keyboard & mouse clicking in
> order to mark and copy the entire url).

This is (at least on windows) only possible in progress dialog but not in
download manager.

Comment 10

[Christian Franke]

Attachment: Patch: Copy as plaintext only (for 1.7 Branch, tested with 1.7RC2)

Comment 11

[Christian Franke]

Attachment: Patch: Same for Trunk (not tested yet)

Comment 12

[[e-mail]]

Comment on attachment 149832 [details] [diff] [review]
Patch: Same for Trunk (not tested yet)

>+    var url = selectedItems[i].firstChild.lastChild.getAttribute("label"); 
This doesn't sound like the right way to do this... try
gDownloadManager.getDownload(selectedItems[i].id).source.spec

>+  const kXferableContractID = "@mozilla.org/widget/transferable;1";
>+  const kXferableIID = Components.interfaces.nsITransferable;
>+  var xferable = Components.classes[kXferableContractID].createInstance(kXferableIID);
>+
>+  xferable.addDataFlavor("text/unicode");
>+  unicodestring.data = sTextUnicode;
>+  xferable.setTransferData("text/unicode", unicodestring, sTextUnicode.length*2);
>+
>+  const kClipboardContractID = "@mozilla.org/widget/clipboard;1";
>+  const kClipboardIID = Components.interfaces.nsIClipboard;
>+  var clipboard = Components.classes[kClipboardContractID].getService(kClipboardIID);
>+  clipboard.setData(xferable, null, kClipboardIID.kGlobalClipboard);
As this is now text you can use the clipboard helper component instead, which
simplifies things trememdously.

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Seeing as this bug hasn't been touched in 2 months, I'll see about updating the
patch with Neil's recommendations.

Comment 14

[[e-mail]]

(In reply to comment #12)
>(From update of attachment 149832 [details] [diff] [review])
>>+    var url = selectedItems[i].firstChild.lastChild.getAttribute("label"); 
>This doesn't sound like the right way to do this...
Sorry, my previous suggestion only worked for current downloads :-[

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch (for trunk)

<Jan> CTho|away: I'd say use DOM since download manager uses tree content view
that is DOM based anyway
<Jan> CTho|away: tell Neil I'm ok with that

Comment 16

[[e-mail]]

Comment on attachment 157335 [details] [diff] [review]
Patch (for trunk)

>+        try {
>+          gStatusBar.label = copyToClipboard(selectedItems);
>+        }
>+        catch (ex) {
>+        }
Which exception are you expecting? You shouldn't try to catch unexpected
exceptions, they may be fallout from other people's bustage.

>+  const kSuppWStringContractID = "@mozilla.org/supports-string;1";
>+  const kSuppWStringIID = Components.interfaces.nsISupportsString;
>+  var unicodestring = Components.classes[kSuppWStringContractID].createInstance(kSuppWStringIID);
You don't use these variables.

>+  var sTextUnicode = ""; var sStatusText = "";
One statement per line, or use "," instead of "; var", or see below

>+  for (var i = 0; i < selectedItems.length; ++i) {
>+      var url = selectedItems[i].firstChild.lastChild.getAttribute("label"); // XXX assumes "Source" is last cell
While there is a way to get the text from the view which works in the case
where the cells are reordered the way the rest of the download manager works
makes it tricky to access, so DOM is fine here.

>+      
No trailing spaces please.

>+      else
>+          sTextUnicode += "\n";
>+      sTextUnicode += url;
Rather than concatenating the text by hand, put the text in an array and join
it together.

>+  if (selectedItems.length > 1)
>+      sStatusText += " ...";
The status bar knows all about ellipsing strings, you could just return a
space-separated list.

>+  const kClipboardHelperCID = "@mozilla.org/widget/clipboardhelper;1";
>+  clipboardHelper = Components.classes[kClipboardHelperCID]
Inline this if it will fit in 80 characters.

>+      .getService(Components.interfaces["nsIClipboardHelper"]);
Should be .nsIClipboardHelper

>+
>+
Erroneous blank line.

>+    <menuitem id="downloadPaneContext-copyurl"
>+              label="&cmd.copyurl.label;"
>+              accesskey="&cmd.copyurl.accesskey;"
>+              command="cmd_copyurl"/>
>   </popup>
I'm not sure this is the best place to put it. Try before the last separator.

>+      <toolbarbutton id="btn_copyurl" label="&cmd.copyurl.label;" accesskey="&cmd.copyurl.accesskey;"
>+                     command="cmd_copyurl"/>
>     </toolbar>
Also here, and any other similar lists.

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Revised patch

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 157977 [details] [diff] [review]
Revised patch

Oops.

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Revised patch that actually works

Comment 20

[jag (Peter Annema)]

Comment on attachment 157978 [details] [diff] [review]
Revised patch that actually works

>Index: xpfe/components/download-manager/resources/downloadmanager.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/download-manager/resources/downloadmanager.js,v
>retrieving revision 1.37
>diff -u -p -9 -r1.37 downloadmanager.js
>--- xpfe/components/download-manager/resources/downloadmanager.js	19 Aug 2004 00:46:31 -0000	1.37
>+++ xpfe/components/download-manager/resources/downloadmanager.js	6 Sep 2004 02:51:41 -0000
>@@ -305,34 +308,40 @@ var downloadViewController = {

>+    case "cmd_copyurl":
>+      selectedItems = getSelectedItems();
>+      if (selectedItems.length > 0) {
>+          gStatusBar.label = copyToClipboard(selectedItems);

Use two space indent like the surrounding code.

>@@ -420,9 +429,26 @@ function doSort(node)

>+function copyToClipboard(selectedItems) // Based on bookmarks.js:copyBookmark
>+{
>+  var unicodeText, statusText;

Just put |var| on the lines below where you initialize the variables.

>+  var urlArray = new Array(selectedItems.length);
>+  for (var i = 0; i < selectedItems.length; ++i) {
>+      urlArray[i] = selectedItems[i].firstChild.lastChild.getAttribute("label");
>+  }

Same indentation nit here.

>+  unicodeText = urlArray.join("\n");
>+  statusText = urlArray.join(" ");
>+
>+  clipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>+      .getService(Components.interfaces.nsIClipboardHelper);

Missing |var| from clipboardHelper.

Personally I prefer to line it up like this:

>+  var clipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>+                                  .getService(Components.interfaces.nsIClipboardHelper);

Or at least indent the second/wrapped line to beyond the equals sign.

>+  clipboardHelper.copyString(unicodeText);
>+
>+  return statusText;
>+}

Actually, looks like you could just change this to

  clipboardHelper.copyString(urlArray.join("\n"));

  return urlArray.join(" "); // for status text

and be rid of the temporary variables altogether.

Comment 21

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Re-revisd patch

>and be rid of the temporary variables altogether.
I wasn't sure if that would be clear enough to get an r+

Comment 22

[jag (Peter Annema)]

Comment on attachment 157985 [details] [diff] [review]
Re-revisd patch

+  return urlArray.join(" ");

The comment I added to that line in my review would've helped clarify why
you're doing that.

sr=jag if you add that comment.

Comment 23

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch with jag's comment

Comment 24

[[e-mail]]

Comment on attachment 157988 [details] [diff] [review]
Patch with jag's comment

Review cancelled due to incomplete patch - no .dtd changes!

>     case "cmd_properties":
>     case "cmd_pause":
>     case "cmd_cancel":
>     case "cmd_remove":
>     case "cmd_openfile":
>     case "cmd_showinshell":
>     case "cmd_selectAll":
>+    case "cmd_copyurl":
You put the command in the right order in the xul... would be nice to do it
here as well.

>     var cmds = ["cmd_properties", "cmd_pause", "cmd_cancel", "cmd_remove",
>-                "cmd_openfile", "cmd_showinshell"];
>+                "cmd_openfile", "cmd_showinshell", "cmd_copyurl"];
And here too. And in the .dtd of course ;-)

>+function copyToClipboard(selectedItems) // Based on bookmarks.js:copyBookmark
How much of copyBookmark is left? The comment might be misleading.

Comment 25

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Comment 26

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

I'm assuming I need a new sr.  If not, please unset the request.

Comment 27

[jag (Peter Annema)]

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

Heh, yeah, having the DTD changes is a good thing :-) Sorry I missed their
absence.

sr=jag

Comment 28

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch to check in

Sorry, I forgot to move the addition to the DTD :(.  If the other patch is
otherwise ok, this is the one to check in.

Comment 29

[[e-mail]]

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

r=me for attachment 158079 [details] [diff] [review].

Comment 30

[[e-mail]]

Attachment 158079 [details] [diff] checked in.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

+<!ENTITY cmd.copyurl.label                "Copy URL">

do users know what a URL is? maybe "Copy Source Address" or something would be
better? Or do I just underestimate users? :-)

Comment 32

[Vidar Haarr (not reading bugmail)]

We already use "Location", so why not use it here?
"Copy Location" is already a wellknown term among the users that would otherwise
use this feature, and the string is already there.

Comment 33

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #32)
> We already use "Location", so why not use it here?
> "Copy Location" is already a wellknown term among the users that would otherwise
> use this feature, and the string is already there.

"Show file location" shows where you saved it.  "Copy Location" would imply to
me that it would copy the location you saved to, rather than the source location.

Comment 34

[Vidar Haarr (not reading bugmail)]

Good point. I'd vote for "Copy Source Location", then.