227749 - Add "Copy Source Location" to download manager context menus

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Ben]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031203 Firebird/0.7+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031203 Firebird/0.7+

In the new download manager window, one can click with the right mouse button on
a downloaded file and a short menu appears. I suggest to add the following
option to that short menu (it will be longer then!):

Copy link location (or "Copy web address" or something like this)

This way it is easy to download the file again or browse the parent directory in
the browser.

(BTW: I love the new download manager!)

Reproducible: Always

Steps to Reproduce:

Comment 1

[José Jeria]

You can achieve this by right clicking on the file and selecting Properteis,
there you can copy the url

Comment 2

[Ben]

Regarding comment #1: That is true. However, the properties window is very small
and not resizable, which makes it hard to copy the complete URL. Since the menu
is so very small, adding one or two options should not be cumbersome.

Furthermore, an option to reload the same file would be handy as well. To
download the latest Firebird build, for example.

Comment 3

[Ali Ebrahim]

In the interests of reducing steps required for end-users to do common tasks,
I'm setting this to NEW. I think it's a good idea.

Comment 4

[Jon Henry]

Attachment: Proposed patch

Add "Copy File Address" to the download manager context menu. I'm open to
suggestions on the wording, if anyone has a better idea.

Comment 5

[Jon Henry]

Comment on attachment 156224 [details] [diff] [review]
Proposed patch

Mike, could you take a look at this when you have the time? It adds a new
string for localizers, so I guess this would need to go in soon to make 1.0.
Short patch.

Comment 6

[Ben]

(In reply to comment #4 and comment #5):
> Add "Copy File Address" to the download manager context menu. I'm open to
> suggestions on the wording, if anyone has a better idea.

On web pages Firefox shows the right-click option "Copy Link Location": I
therefore suggest to use the same text to be consequent. Furthermore, it would
*not* introduce a new string for localizers!

(The string "Copy File Address" suggests to copy the local file name of the
downloaded file, not the URL.)

Comment 7

[Jon Henry]

I was thinking of Copy Link Location too, and you're right, that would save on
the new string. My hesitation was that what you're clicking on isn't really a
link, so it could be confusing. I also was kicking around "Copy Remote Location"
or "Copy File URL", but maybe Link Location is the best move since users are
already comfortable with it. I can always roll up a new patch.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 156224 [details] [diff] [review]
Proposed patch

Copy File Address just seems vague and could be easily construed as the local
file address, not the remote address.

I'm not sold on the value of this menuitem.  Opening properties lets you get
the original address, how often will you need this from the Download Manager?

Comment 9

[daneel]

I think Ben Goodger said somewhere he planned on this, but "copy file address"
is weird.

Comment 10

[Jon Henry]

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

Comment 11

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

See also bug 142798 (seamonkey version).  The patch there has been SR'ed and is
just waiting on a review.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The text should be "Copy file location" to match terminology used throughout the
rest of the app. See bug 241676.

Jon, can you submit an updated patch (to current trunk)?

Comment 13

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

(In reply to comment #12)
> The text should be "Copy file location" to match terminology used throughout the
> rest of the app. See bug 241676.
> 
> Jon, can you submit an updated patch (to current trunk)?

Comment 0 is looking for the URL the file was downloaded from.  Copy file
location implies the local path to the downloaded file.  SeaMonkey's button says
"Copy URL".

Comment 14

[Jon Henry]

Attachment: New patch

Ask and ye shall receive. Unbitrotted patch with "Copy URL" instead of whatever
I had before.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 180010 [details] [diff] [review]
New patch


>+<!ENTITY cmd.copyLocation.label           "Copy URL">
>+<!ENTITY cmd.copyLocation.accesskey       "U">

Copy Original Location would be better/more consistent

> <!ENTITY cmd.remove.label                 "Remove">
> <!ENTITY cmd.remove.accesskey             "e">
> <!ENTITY cmd.properties.label             "Properties">
>Index: mozilla/toolkit/mozapps/downloads/content/downloads.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v
>retrieving revision 1.42
>diff -u -r1.42 downloads.js
>--- mozilla/toolkit/mozapps/downloads/content/downloads.js	3 Apr 2005 15:48:30 -0000	1.42
>+++ mozilla/toolkit/mozapps/downloads/content/downloads.js	8 Apr 2005 02:51:01 -0000
>@@ -409,6 +409,18 @@
>   gDownloadViewController.onCommandUpdate();
> }


>+function onDownloadCopy(aEvent)

s/Copy/CopyLocation/ please

>+{
>+  var download = aEvent.target;
>+  if (download.localName == "download") {
>+    var clipboard = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>+                        .getService(Components.interfaces.nsIClipboardHelper );

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

is the typical style used, instead of randomish indentation for the split line

this seems ok, code-wise, and I'm up for adding this, though I'll call in beltzner as my dedicated second-guesser since I've been ambivalent for a long time about this.

Comment 16

[Jon Henry]

Attachment: Updated patch

Thanks for the review. Updated to mconnor's comments, and merged to tip.

I am re-requesting review because it has been a very long time since I wrote this patch, and I want to make sure I didn't screw anything up in updating it.

If and when this gets reviewed, I'd need someone else to land it for me, as I have no CVS privs.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 207674 [details] [diff] [review]
Updated patch

moving request from jhenry to ui-review

Comment 18

[Mike Connor [:mconnor]]

Not a blocker, but if we decide to take this, it'll make Firefox2

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 207674 [details] [diff] [review]
Updated patch

On the question of whether or not we should take it: after a lot of soul searching, I think that it *is* an appropriate function to be including on the context menu.

As for the review of the expression of the UI, I think that it needs to be in its own seperator. So:

 Open
 Open Containing Folder
 Remove
 ----------------------
 Copy Original Location
 ----------------------
 Properties

While a subtle change, I think the seperator will clarify the fact that the "Copy ..." function is talking about an edit action as opposed to a file activity (open/remove) action.

Comment 20

[Jon Henry]

Comment on attachment 207674 [details] [diff] [review]
Updated patch

Thanks, Mike. Canceling my code review until I have time to post a revised patch.

Comment 21

[Jon Henry]

Attachment: Patch with separator

Like so? Added a separator before the new menu item.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 208971 [details] [diff] [review]
Patch with separator

I don't see where the label and accesskey are actually declared. Anywhere in this patch? Forget to diff a file?

Comment 23

[Jon Henry]

Attachment: Revised patch with less suck

Thanks again, Beltzner. You're right, I forgot to diff the dtd file. This one should be good to go.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 230137 [details] [diff] [review]
Revised patch with less suck

update this and I can do the review.

Comment 25

[Shawn Wilsher :sdwilsh]

Also, please change the string to "Copy Source Location" (as per #ux team)

Comment 27

[Jon Henry]

Attachment: Updated patch

Tested and working.

Comment 28

[Shawn Wilsher :sdwilsh]

Comment on attachment 279605 [details] [diff] [review]
Updated patch

>+function copySourceLocation(aDownload)
>+{
>+  var uri;
>+
>+  if (aDownload.hasAttribute("referrer"))
>+    uri = aDownload.getAttribute("referrer");
>+  else
>+    uri = aDownload.getAttribute("uri");
You will always want to get the uri here, never the referrer

>+  var clipboard =
>+    Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>+              .getService(Components.interfaces.nsIClipboardHelper);
Nit: this file now uses the CC["..."].getService(Ci....) form

>+    <menuitem id="menuitem_copyLocation"
>+              label="&cmd.copyLocation.label;" accesskey="&cmd.copyLocation.accesskey;"
nit: line wrapping at 80 characters please

r=sdwilsh with these fixed.

Comment 29

[Jon Henry]

Attachment: Updated to comments

Fixed nits.

If this passes muster, I'd appreciate someone checking in for me when they have time.

Comment 30

[Jon Henry]

It occurs to me that I don't have a test case for this. If that's required, I won't have time to make one any time soon :(

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #30)
> It occurs to me that I don't have a test case for this. If that's required, I
> won't have time to make one any time soon :(

That's what QA (I), and the in-litmus? flag (+/-) are for :-) 

Comment 32

[Shawn Wilsher :sdwilsh]

Comment on attachment 279617 [details] [diff] [review]
Updated to comments

just line the getService up with the Cc.

When you upload the new patch, you don't need to ask for review - just add the ckeckin-needed keyword to the bug.

Comment 33

[Jon Henry]

Attachment: Patch for checkin

Thank you for the review, Shawn! Carrying forward r+ and adding checkin-needed.

Comment 34

[Reed Loden [:reed]]

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd,v  <--  downloads.dtd
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.87; previous revision: 1.86
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.30; previous revision: 1.29
done

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre

and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre

Comment 36

[Stephen Donner [:stephend] Not actively reading bugmail]

http://litmus.mozilla.org/show_test.cgi?id=4636

in-litmus+

Comment 37

[AndrewM]

Why was the 'Properties' option dropped?

Comment 38

[Przemyslaw Bialik]

I don't know why it was dropped but it's Bug 394571