474620 - Re-Implement "Properties" (opening Progress Dialog) in new download manager UI

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Robert Kaiser]

As progress dialogs aren't implemented yet while I'm developing the new download manager in bug 472001, I couldn't implement the option to open a progress dialog as "Properties" or "More Information" for (active) downloads yet, but we probably should re-implement this once this new UI has landed.

Comment 1

[Robert Kaiser]

From bug 483241 Comment #11 by Neil:
> At the moment we can only launch progress dialogs for new downloads, and even
> the old download manager could only launch progress dialogs for active
> downloads, so even if you implemented that limited feature it wouldn't hurt to
> default gEndTime to Date.now() when the dialog opens. I'm thinking that perhaps
> the download manager could pass the end time as a second parameter so that you
> could open a progress dialog for a finished download (to find out the blocked
> reason, perhaps!)

I think we should look into this when working on the bug here.

Comment 2

[Robert Kaiser]

Attachment: implement "properties" in new dlmgr

Here's a patch to implement this feature again.

In addition to the obvious addition of the command, it does the following:
* Hand the parent to the progress window from the component
* Fix the component to throw and not call the progress window when no ID given
* Fetch the end time from the tree view if called from dlmgr
* Switch back the window ID of the dlmgr so it picks up the correct icon again

The try-catch removal and window ID change are not really the core scope of this bug, but they're very small and things either I stumbled over in this work or was asked about by users (those people actually notice when the window icon is wrong).

Comment 4

[[e-mail]]

Comment on attachment 393008 [details] [diff] [review]
implement "properties" in new dlmgr

>+          <menuitem id="dlMenu_properties"
>+                    label="&cmd.properties.label;"
>+                    accesskey="&cmd.properties.accesskey;"
>+                    command="cmd_properties"/>
[The old download manager's Enter action would open properties if it couldn't open the item. Not sure whether that's ideal or whether we should have a separate key or just use the menu.]

>+  if (opener && opener.gDownloadTreeView && opener.gDownloadTreeView.rowCount > 0) {
Not sure that we can rely on opener here; might want to use dmui.recentWindow either here or in dmui itself (instead of parent) when opening the progress dialog.

>     var download = null;
>-    try {
>-      let dm = Cc["@mozilla.org/download-manager;1"].
>-               getService(Ci.nsIDownloadManager);
>-      download = dm.getDownload(aID);
>-    } catch (ex) {}
>+    let dm = Cc["@mozilla.org/download-manager;1"].
>+             getService(Ci.nsIDownloadManager);
>+    download = dm.getDownload(aID);
Nit: don't need to pre-init download any more. Also, these files seem to prefer var to let ;-)

Comment 5

[[e-mail]]

Or another alternative is for download manager to open progress dialog directly (via openDialog) rather than going through dmui, thus allowing it to pass a third argument (the end time).

Comment 6

[Robert Kaiser]

Attachment: implement "properties" in new dlmgr, v2, with tests

OK, I decided I couldn't cheat my own rules and added testing for this function now ;-)

dmui.recentWindow is not exported to outside the component, so I used the window mediator directly to get the download manager window from the progress window to make this more robust.

I split out the call to the progress window into its own function in downloadmanager.js for two reasons: 1) it can be overridden by the selection test, 2) an extension can override it and create its own properties window.

The tests are on one hand an addition to the single/multiple selection test, verifying that it's called only on single selections right now, and a new, more in-depth test (based on test_clear_button_disabled.xul code) that actually calls up properties for both an inactive and an active download and even looks for correct setting of the end time in the progress window.

Comment 7

[Robert Kaiser]

Comment on attachment 394716 [details] [diff] [review]
implement "properties" in new dlmgr, v2, with tests

>--- /dev/null
>+++ b/suite/common/downloads/tests/chrome/test_open_properties.xul
>+          // force rebuilding of tree to generate a ui-done event on the DM
>+          dmview.initTree();

Actually, ignore those two lines, they're not needed, I had an earlier bug in the test. I removed those locally and the test still works fine.

Comment 8

[[e-mail]]

Comment on attachment 394716 [details] [diff] [review]
implement "properties" in new dlmgr, v2, with tests

>     var params = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
>-    var download = null;
>+    let parent = null;
>-      let dm = Cc["@mozilla.org/download-manager;1"].
>+    let dm = Cc["@mozilla.org/download-manager;1"].
>+    var download = dm.getDownload(aID);
>     let reason = Cc["@mozilla.org/supports-PRInt16;1"].
>     var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
Argh!

Comment 9

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/ee0aee8dbc9a with the var/let mess cleaned up in the lines I touched.

Comment 10

[[e-mail]]

Comment on attachment 394716 [details] [diff] [review]
implement "properties" in new dlmgr, v2, with tests

>+<!ENTITY cmd.properties.label            "Propertiesâ¦">
>+<!ENTITY cmd.properties.accesskey        "s">
Aargh, I just realised that, because this command's job is to open the Properties dialog, it shouldn't have an ellipsis...

Comment 11

[Robert Kaiser]

(In reply to comment #10)
> (From update of attachment 394716 [details] [diff] [review])
> >+<!ENTITY cmd.properties.label            "Propertiesâ¦">
> >+<!ENTITY cmd.properties.accesskey        "s">
> Aargh, I just realised that, because this command's job is to open the
> Properties dialog, it shouldn't have an ellipsis...

Erm, why not? Any command that opens a dialog should, right?