Closed
Bug 227749
Opened 21 years ago
Closed 17 years ago
Add "Copy Source Location" to download manager context menus
Categories
(Toolkit :: Downloads API, enhancement)
Toolkit
Downloads API
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha8
People
(Reporter: pufiad, Assigned: jhenry)
References
Details
Attachments
(1 file, 7 obsolete files)
4.79 KB,
patch
|
jhenry
:
review+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
You can achieve this by right clicking on the file and selecting Properteis, there you can copy the url
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•20 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
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
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)
(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.)
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•20 years ago
|
||
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?
I think Ben Goodger said somewhere he planned on this, but "copy file address" is weird.
Assignee | ||
Comment 10•20 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.
Comment 12•19 years ago
|
||
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•19 years ago
|
||
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)
Attachment #156224 -
Flags: review?(mconnor)
Comment 15•19 years ago
|
||
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•19 years ago
|
||
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 17•19 years ago
|
||
Comment on attachment 207674 [details] [diff] [review] Updated patch moving request from jhenry to ui-review
Attachment #207674 -
Flags: superreview?(beltzner) → ui-review?(beltzner)
Comment 18•19 years ago
|
||
Not a blocker, but if we decide to take this, it'll make Firefox2
Flags: blocking-firefox2? → blocking-firefox2-
Comment 19•19 years ago
|
||
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•19 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•19 years ago
|
||
Like so? Added a separator before the new menu item.
Attachment #208971 -
Flags: ui-review?(beltzner)
Attachment #208971 -
Flags: review?(mconnor)
Comment 22•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
QA Contact: ali → download.manager
Comment 24•17 years ago
|
||
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)
Comment 25•17 years ago
|
||
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•17 years ago
|
Flags: in-litmus?
Flags: blocking-firefox3?
Version: unspecified → Trunk
Assignee | ||
Comment 27•17 years ago
|
||
Tested and working.
Attachment #230137 -
Attachment is obsolete: true
Attachment #279605 -
Flags: review?(comrade693+bmo)
Comment 28•17 years ago
|
||
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•17 years ago
|
||
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•17 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 32•17 years ago
|
||
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•17 years ago
|
||
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+
Keywords: checkin-needed
Comment 34•17 years ago
|
||
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
Closed: 17 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+
Updated•17 years ago
|
Target Milestone: Firefox 3 M9 → Firefox 3 M8
Comment 37•17 years ago
|
||
Why was the 'Properties' option dropped?
Comment 38•17 years ago
|
||
I don't know why it was dropped but it's Bug 394571
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•