The default bug view has changed. See this FAQ.

Add "Copy Source Location" to download manager context menus

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Toolkit
Downloads API
--
enhancement
VERIFIED FIXED
14 years ago
9 years ago

People

(Reporter: Ben, Assigned: Jon Henry)

Tracking

Trunk
mozilla1.9alpha8
Points:
---
Bug Flags:
blocking-firefox2 -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

14 years ago
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

13 years ago
You can achieve this by right clicking on the file and selecting Properteis,
there you can copy the url
(Reporter)

Comment 2

13 years ago
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

13 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
QA Contact: aebrahim
Hardware: PC → All
Summary: Download manager window: add right-click menu options → Add "Copy web location" to download manager context menus
(Assignee)

Comment 4

13 years ago
Created attachment 156224 [details] [diff] [review]
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.
Assignee: bugs → jhenry
Status: NEW → ASSIGNED
(Assignee)

Comment 5

13 years ago
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.
Attachment #156224 - Flags: review?(mconnor)
(Reporter)

Comment 6

13 years ago
(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.)
(Assignee)

Comment 7

13 years ago
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 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

13 years ago
I think Ben Goodger said somewhere he planned on this, but "copy file address"
is weird.
(Assignee)

Comment 10

13 years ago
*** Bug 258453 has been marked as a duplicate of this bug. ***
See also bug 142798 (seamonkey version).  The patch there has been SR'ed and is
just waiting on a review.
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)?
(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".
(Assignee)

Comment 14

12 years ago
Created attachment 180010 [details] [diff] [review]
New patch

Ask and ye shall receive. Unbitrotted patch with "Copy URL" instead of whatever
I had before.
Attachment #156224 - Attachment is obsolete: true
Attachment #180010 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #156224 - Flags: review?(mconnor)

Updated

11 years ago
Flags: blocking-aviary2?
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.
Attachment #180010 - Flags: superreview?(beltzner)
Attachment #180010 - Flags: review?(mconnor)
Attachment #180010 - Flags: review+
(Assignee)

Comment 16

11 years ago
Created attachment 207674 [details] [diff] [review]
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.
Attachment #180010 - Attachment is obsolete: true
Attachment #207674 - Flags: superreview?(beltzner)
Attachment #207674 - Flags: review?(mconnor)
Attachment #180010 - Flags: superreview?(beltzner)
Comment on attachment 207674 [details] [diff] [review]
Updated patch

moving request from jhenry to ui-review
Attachment #207674 - Flags: superreview?(beltzner) → ui-review?(beltzner)
Not a blocker, but if we decide to take this, it'll make Firefox2
Flags: blocking-firefox2? → blocking-firefox2-
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.
Attachment #207674 - Flags: ui-review?(beltzner) → ui-review-
(Assignee)

Comment 20

11 years ago
Comment on attachment 207674 [details] [diff] [review]
Updated patch

Thanks, Mike. Canceling my code review until I have time to post a revised patch.
Attachment #207674 - Attachment is obsolete: true
Attachment #207674 - Flags: review?(mconnor)
(Assignee)

Comment 21

11 years ago
Created attachment 208971 [details] [diff] [review]
Patch with separator

Like so? Added a separator before the new menu item.
Attachment #208971 - Flags: ui-review?(beltzner)
Attachment #208971 - Flags: review?(mconnor)
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?
Attachment #208971 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 23

11 years ago
Created attachment 230137 [details] [diff] [review]
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.
Attachment #208971 - Attachment is obsolete: true
Attachment #230137 - Flags: review?(mconnor)
Attachment #208971 - Flags: review?(mconnor)

Updated

10 years ago
QA Contact: ali → download.manager
Comment on attachment 230137 [details] [diff] [review]
Revised patch with less suck

update this and I can do the review.
Attachment #230137 - Flags: review?(mconnor)
Also, please change the string to "Copy Source Location" (as per #ux team)
Summary: Add "Copy web location" to download manager context menus → Add "Copy Source Location" to download manager context menus
Target Milestone: --- → Firefox 3 M9

Updated

10 years ago
Duplicate of this bug: 391896
Flags: in-litmus?
Flags: blocking-firefox3?
Version: unspecified → Trunk
(Assignee)

Comment 27

10 years ago
Created attachment 279605 [details] [diff] [review]
Updated patch

Tested and working.
Attachment #230137 - Attachment is obsolete: true
Attachment #279605 - Flags: review?(comrade693+bmo)
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.
Attachment #279605 - Flags: review?(comrade693+bmo) → review+
(Assignee)

Comment 29

10 years ago
Created attachment 279617 [details] [diff] [review]
Updated to comments

Fixed nits.

If this passes muster, I'd appreciate someone checking in for me when they have time.
Attachment #279617 - Flags: review?(comrade693+bmo)
(Assignee)

Comment 30

10 years ago
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 :(
(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 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.
Attachment #279617 - Flags: review?(comrade693+bmo) → review+
(Assignee)

Comment 33

10 years ago
Created attachment 279669 [details] [diff] [review]
Patch for checkin

Thank you for the review, Shawn! Carrying forward r+ and adding checkin-needed.
Attachment #279605 - Attachment is obsolete: true
Attachment #279617 - Attachment is obsolete: true
Attachment #279669 - Flags: review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking-firefox3?
Keywords: checkin-needed
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
http://litmus.mozilla.org/show_test.cgi?id=4636

in-litmus+
Flags: in-litmus? → in-litmus+
Target Milestone: Firefox 3 M9 → Firefox 3 M8

Comment 37

10 years ago
Why was the 'Properties' option dropped?

Comment 38

10 years ago
I don't know why it was dropped but it's Bug 394571
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.