132019 - Download manager context menu

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, enhancement)

Description

[Hixie (not reading bugmail)]

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

[Håkan Waara]

What exactly would "Show in Browser" do for a half-downloaded file?

Comment 2

[Hixie (not reading bugmail)]

Same as it does now. None of those options are new features, they are just
context menu access to existing features.

Comment 3

[Dimitrios]

This rfe is quite reasonable for all platforms/operating systems.

Comment 4

[Markus Niederlechner]

Attachment: patch to downloadmanager.(js|xul)

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

[Markus Niederlechner]

Attachment: better patch

This patch doesn't have the white space changes, doesn't make the
isCommandEnabled function global and fixes the minValue warning.

Comment 6

[David G King]

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

Comment 7

[David G King]

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

Comment 8

[Markus Niederlechner]

Attachment: updated patch

Comment 9

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.

Comment 10

[Daniel Wang]

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

Comment 11

[Leston Buell]

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.

Comment 12

[Ian Neal]

Attachment: New patch

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.

Comment 13

[Ian Neal]

Taking

Comment 14

[Ian Neal]

Accepting

Comment 15

[Ian Neal]

Adding Blake to cc

Comment 16

[Ian Neal]

I have a revision in testing at the moment which should fix bug 146681, I'll
hopefully be adding that later today.

Comment 17

[Ian Neal]

Attachment: Combined patch v1.1

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

Comment 18

[[e-mail]]

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.

Comment 19

[Ian Neal]

Attachment: Patch v1.2

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

Comment 20

[[e-mail]]

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.

Comment 21

[Ian Neal]

Attachment: Patch v1.3

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.

Comment 22

[Ian Neal]

The additional fix is for bug 167528

Comment 23

[Ian Neal]

Attachment: Patch v1.3a

Corrects a small typo

Comment 24

[Ian Neal]

Attachment: Patch v1.3b

Addresses issues raised by Neil on IRC

Comment 25

[[e-mail]]

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.

Comment 26

[José Jeria]

Eh, why was I removed from cc list on this bug. 

Comment 27

[Ian Neal]

Attachment: Patch v1.3c

Inlining done as suggested

Comment 28

[jag (Peter Annema)]

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.

Comment 29

[Ian Neal]

Attachment: Patch v1.3d

same as patch v1.3c except with indentions as requested

Comment 30

[jag (Peter Annema)]

Comment on attachment 123804 [details] [diff] [review]
Patch v1.3d

Carrying over r= and sr=

Comment 31

[Ian Neal]

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

[(not reading, please use [e-mail] instead)]

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.

Comment 33

[Ian Neal]

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.

Comment 34

[Ian Neal]

Pushing out 1.5a as requested by drivers

Comment 35

[(not reading, please use [e-mail] instead)]

thanks for understanding, ian.

Comment 36

[Ian Neal]

Attachment: Enhancements only patch v1.4

This patch just implements the enhancements in this bug and bug 146681

Comment 37

[[e-mail]]

Comment on attachment 124013 [details] [diff] [review]
Enhancements only patch v1.4

Oops.

Comment 38

[Matthias Versen [:Matti]]

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

Comment 39

[jag (Peter Annema)]

Comment on attachment 124013 [details] [diff] [review]
Enhancements only patch v1.4

sr=jag

Comment 40

[Rick Bull]

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.

Comment 41

[Ian Neal]

Patch checked in by timeless - thanks

Comment 42

[Serge Gautherie (:sgautherie)]

Could someone review bug 137359 comment 11, for a possible "Duplicate" report ?
Thanks.

Comment 43

[Ian Neal]

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

Comment 44

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

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