Closed
Bug 132019
Opened 23 years ago
Closed 21 years ago
Download manager context menu
Categories
(SeaMonkey :: Download & File Handling, enhancement)
SeaMonkey
Download & File Handling
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.5alpha
People
(Reporter: ian, Assigned: iannbugzilla)
References
Details
Attachments
(1 file, 11 obsolete files)
(deleted),
patch
|
neil
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
The download manager should have a context menu for the items in the list.
+------------------+
| Pause | if active and not paused
| Resume | if paused
| Restart |
| Show in browser |
+------------------+
| Cancel | if active
| Remove from list | if not active
+------------------+
| Properties |
+------------------+
Comment 1•23 years ago
|
||
What exactly would "Show in Browser" do for a half-downloaded file?
Reporter | ||
Comment 2•23 years ago
|
||
Same as it does now. None of those options are new features, they are just
context menu access to existing features.
Updated•23 years ago
|
Summary: Download manager context menu → [RFE] Download manager context menu
This rfe is quite reasonable for all platforms/operating systems.
OS: Linux → All
Hardware: PC → All
Comment 4•22 years ago
|
||
This patch uses all items of the toolbar and a 'pause' item as a context menu.
The items will only show if they are enabled on the toolbar (pause currently
never shows, there is no backend available). There are no commands defined for
'resume' and 'restart', so I couldn't implement them. I couldn't figure out how
to call downloadViewerController.isCommandEnabled, so I've made that function
global.
The patch also fixes three strict warnings (bug 145699 and 2/3 of bug 147842).
The patch doesn't fix the 'minValue is not defined'-warning (bug 147842).
The whitespace changes were inserted by my editor and I don't know how to get
them out of the patch without breaking it, sorry.
This is my first patch, can someone please tell me how to get r/sr/a for it?
Comment 5•22 years ago
|
||
This patch doesn't have the white space changes, doesn't make the
isCommandEnabled function global and fixes the minValue warning.
Attachment #92111 -
Attachment is obsolete: true
Comment 6•22 years ago
|
||
*** Bug 147842 has been marked as a duplicate of this bug. ***
Comment 7•22 years ago
|
||
This is more than an enhancement as the patch also fixes two JS Errors (and no,
I don't mean JS warnings). Thus, I'm changing the Severity setting
Severity: enhancement → minor
Comment 8•22 years ago
|
||
Attachment #92447 -
Attachment is obsolete: true
Comment 9•22 years ago
|
||
[RFE] is deprecated in favor of severity: enhancement. They have the same meaning.
Severity: minor → enhancement
Updated•22 years ago
|
QA Contact: sairuh → petersen
Updated•22 years ago
|
Summary: [RFE] Download manager context menu → Download manager context menu
Comment 10•22 years ago
|
||
*** Bug 176927 has been marked as a duplicate of this bug. ***
Comment 11•22 years ago
|
||
What will the Properties item in the context menu show? One thing i think is
needed is a way to see the location to which the file was downloaded. So often,
i download not noticing what directory the file is being downloaded to. So, i
hope that the destination file will be shown in properties.
Assignee | ||
Comment 12•21 years ago
|
||
I wrote this patch and then found this bug. Not sure why I don't need a
onpopupshowing function but everything seems to be correctly enabled/disabled
within the context menu without it.
Attachment #98714 -
Attachment is obsolete: true
Assignee | ||
Comment 15•21 years ago
|
||
Adding Blake to cc
Attachment #122971 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #122971 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 16•21 years ago
|
||
I have a revision in testing at the moment which should fix bug 146681, I'll
hopefully be adding that later today.
Assignee | ||
Comment 17•21 years ago
|
||
Same as above patch but also including patch to add functionality asked for in
bug 146681 - Double clicking on a downloaded entry in Download Manager should
open the file
Attachment #122971 -
Attachment is obsolete: true
Attachment #123050 -
Flags: superreview?(jaggernaut)
Comment 18•21 years ago
|
||
Comment on attachment 123050 [details] [diff] [review]
Combined patch v1.1
>+function onDoubleClick() {
>+ var selectedItem = getSelectedItem();
>+ if (selectedItem && gDownloadManager.getDownload(selectedItem.id))
>+ return goDoCommand('cmd_properties');
>+ else
>+ return goDoCommand('cmd_openfile');
>+}
I don't like the logic here. Personally I would query the download view
controller to see which command, if any, to do.
Attachment #123050 -
Flags: superreview?(jaggernaut)
Assignee | ||
Comment 19•21 years ago
|
||
Now tests using isCommandEnabled
Removed test for gDownloadManager.getDownload(selectedItem.id) in
cmd_properties part of doCommand as it's no longer needed
Added test for existance of the file in cmd_openfile part of doCommand as the
following error is generated in the JS console otherwise:
Error: [Exception... "Component returned failure code: 0x80520012
(NS_ERROR_FILE_NOT_FOUND) [nsILocalFile.isExecutable]" nsresult: "0x80520012
(NS_ERROR_FILE_NOT_FOUND)" location: "JS frame ::
chrome://communicator/content/downloadmanager/downloadmanager.js ::
dVC_doCommand :: line 216" data: no]
Source File: chrome://communicator/content/downloadmanager/downloadmanager.js
Line: 209
Attachment #123050 -
Attachment is obsolete: true
Attachment #123197 -
Flags: superreview?(jaggernaut)
Attachment #123197 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 20•21 years ago
|
||
Comment on attachment 123197 [details] [diff] [review]
Patch v1.2
>+function onDoubleClick() {
>+ if (downloadViewController.isCommandEnabled('cmd_properties'))
>+ return goDoCommand('cmd_properties');
>+ if (downloadViewController.isCommandEnabled('cmd_openfile'))
>+ return goDoCommand('cmd_openfile');
>+ return true;
goDoCommand doesn't return a value, just use if and else if.
> switch (aCommand) {
> case "cmd_openfile":
>- if (isDownloading)
>- return false;
>+ return !isDownloading;
> case "cmd_showinshell":
This code is bad but you made it worse :-( There was no break before!
>- if (selectedItem && gDownloadManager.getDownload(selectedItem.id))
>+ if (selectedItem)
IMHO there's no point removing this belt and braces
>+ // If file doesn't exist break out
>+ if (!file.exists())
>+ break;
You should also disable both commands if the file does not exist.
Attachment #123197 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #123197 -
Flags: superreview?(jaggernaut)
Assignee | ||
Comment 21•21 years ago
|
||
Corrections made per Neil's comments also corrected a problem with the
buttons/status line not updating when an entry is deleted from the list of
downloaded files.
Attachment #123197 -
Attachment is obsolete: true
Attachment #123278 -
Flags: superreview?(jaggernaut)
Attachment #123278 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 22•21 years ago
|
||
The additional fix is for bug 167528
Attachment #123278 -
Flags: superreview?(jaggernaut)
Attachment #123278 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 23•21 years ago
|
||
Corrects a small typo
Attachment #123278 -
Attachment is obsolete: true
Attachment #123383 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #123383 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 24•21 years ago
|
||
Addresses issues raised by Neil on IRC
Attachment #123383 -
Attachment is obsolete: true
Attachment #123499 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 25•21 years ago
|
||
Comment on attachment 123499 [details] [diff] [review]
Patch v1.3b
>+ var fileExists = selectedItem && getFileForItem(selectedItem).exists();
>
> switch (aCommand) {
> case "cmd_openfile":
>- if (isDownloading)
>- return false;
> case "cmd_showinshell":
> // we can't reveal until the download is complete, because we have not given
> // the file its final name until them.
>- return selectionCount == 1 && !isDownloading;
>+ return selectionCount == 1 && fileExists && !isDownloading;
Please inline the fileExists check because you only use it once.
Attachment #123499 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Comment 26•21 years ago
|
||
Eh, why was I removed from cc list on this bug.
Assignee | ||
Comment 27•21 years ago
|
||
Inlining done as suggested
Attachment #123499 -
Attachment is obsolete: true
Attachment #123503 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•21 years ago
|
Attachment #123503 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #123503 -
Flags: superreview?(jaggernaut)
Comment 28•21 years ago
|
||
Comment on attachment 123503 [details] [diff] [review]
Patch v1.3c
>Index: downloadmanager.xul
>===================================================================
>@@ -111,6 +111,31 @@
> command="cmd_properties" modifiers="accel"/>
> </keyset>
>
>+ <popup id="downloadPaneContext">
>+ <menuitem id="downloadPaneContext-properties"
>+ label="&cmd.properties.label;"
>+ accesskey="&cmd.properties.accesskey;"
>+ command="cmd_properties"/>
Indent second, third, etc. lines to have attributes line up with the first
attribute on the first line, like the rest of the file.
sr=jag with that nit fixed.
Attachment #123503 -
Flags: superreview?(jaggernaut) → superreview+
Assignee | ||
Comment 29•21 years ago
|
||
same as patch v1.3c except with indentions as requested
Attachment #123503 -
Attachment is obsolete: true
Attachment #123804 -
Flags: superreview+
Attachment #123804 -
Flags: review+
Attachment #123804 -
Flags: superreview?(jaggernaut)
Attachment #123804 -
Flags: superreview+
Attachment #123804 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #123804 -
Flags: review+
Attachment #123804 -
Flags: approval1.4?
Comment 30•21 years ago
|
||
Comment on attachment 123804 [details] [diff] [review]
Patch v1.3d
Carrying over r= and sr=
Attachment #123804 -
Flags: superreview?(jaggernaut)
Attachment #123804 -
Flags: superreview+
Attachment #123804 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #123804 -
Flags: review+
Assignee | ||
Comment 31•21 years ago
|
||
Seeking driver approval.
The patch fixes two bugs (bug 132331 - JS console error after double clicking on
a file in download manager that no longer exists, and bug 167528 - status bar
not updating after removing entry in download manager) and adds two low risk
enhancements (this one and bug 146681 - making double click on an entry do
different things depending on if the file is being downloaded or not).
I have tested the part of the patch to do with the enhancement in this bug for
over 2 weeks and the other parts for about 1 week on both Linux and Win32
platforms with no problems. (The other fixes became part of this patch because
they were highlighted during my testing and touched the same part of the code).
From the nature of the code and the additions there is little to no risk of
regression.
Comment 32•21 years ago
|
||
ian, this is good stuff.
but can you come up with a patch that is just the bug fixes (especially 167528 -
status bar not updating after removing entry in download manager), and save the
feature work for 1.5 alpha?
trying to be risk averse.
Whiteboard: [wanting to see a fix only patch before a=]
Assignee | ||
Comment 33•21 years ago
|
||
Comment on attachment 123804 [details] [diff] [review]
Patch v1.3d
Canceling approval request having spun off bug fixes into patch on bug 132331.
I'll generate a new patch for the enhancements once that has landed.
Attachment #123804 -
Flags: approval1.4?
Assignee | ||
Comment 34•21 years ago
|
||
Pushing out 1.5a as requested by drivers
Target Milestone: mozilla1.4final → mozilla1.5alpha
Comment 35•21 years ago
|
||
thanks for understanding, ian.
Whiteboard: [wanting to see a fix only patch before a=]
Assignee | ||
Comment 36•21 years ago
|
||
This patch just implements the enhancements in this bug and bug 146681
Attachment #123804 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #124013 -
Flags: review+
Updated•21 years ago
|
Attachment #124013 -
Flags: superreview+
Comment 37•21 years ago
|
||
Comment on attachment 124013 [details] [diff] [review]
Enhancements only patch v1.4
Oops.
Attachment #124013 -
Flags: superreview+
Attachment #124013 -
Flags: superreview?(jaggernaut)
Comment 38•21 years ago
|
||
*** Bug 206931 has been marked as a duplicate of this bug. ***
Comment 39•21 years ago
|
||
Comment on attachment 124013 [details] [diff] [review]
Enhancements only patch v1.4
sr=jag
Attachment #124013 -
Flags: superreview?(jaggernaut) → superreview+
Comment 40•21 years ago
|
||
Sorry to post on this thread, but seeing as my bug got closed I thought I'd post
here. Could you also add the context menu you get when you right-click a file in
Explorer (in Windows). See bug 206931 for my original idea.
Assignee | ||
Comment 41•21 years ago
|
||
Patch checked in by timeless - thanks
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 42•21 years ago
|
||
Could someone review bug 137359 comment 11, for a possible "Duplicate" report ?
Thanks.
Assignee | ||
Comment 43•21 years ago
|
||
*** Bug 137359 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: Browser → Seamonkey
Verified FIXED using SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060104 Mozilla/1.0
Properties
================
Cancel
Remove from List
Copy URL
================
Launch File
Show File Location
These context menu items match the menu bar in the window itself
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•