Last Comment Bug 474620 - Re-Implement "Properties" (opening Progress Dialog) in new download manager UI
: Re-Implement "Properties" (opening Progress Dialog) in new download manager UI
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0b2
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
: 139606 (view as bug list)
Depends on: 472001 483241
Blocks: 515053 508899
  Show dependency treegraph
 
Reported: 2009-01-21 09:38 PST by Robert Kaiser (not working on stability any more)
Modified: 2009-11-02 10:27 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
implement "properties" in new dlmgr (10.45 KB, patch)
2009-08-06 13:28 PDT, Robert Kaiser (not working on stability any more)
neil: review+
neil: superreview+
Details | Diff | Review
implement "properties" in new dlmgr, v2, with tests (23.64 KB, patch)
2009-08-16 06:47 PDT, Robert Kaiser (not working on stability any more)
neil: review+
kairo: superreview+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2009-01-21 09:38:27 PST
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 (not working on stability any more) 2009-05-25 05:17:21 PDT
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 (not working on stability any more) 2009-08-06 13:28:51 PDT
Created attachment 393008 [details] [diff] [review]
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 3 Robert Kaiser (not working on stability any more) 2009-08-06 13:31:46 PDT
*** Bug 139606 has been marked as a duplicate of this bug. ***
Comment 4 neil@parkwaycc.co.uk 2009-08-06 16:47:51 PDT
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 neil@parkwaycc.co.uk 2009-08-06 16:50:32 PDT
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 (not working on stability any more) 2009-08-16 06:47:04 PDT
Created attachment 394716 [details] [diff] [review]
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 (not working on stability any more) 2009-08-16 07:23:25 PDT
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 neil@parkwaycc.co.uk 2009-08-16 14:03:29 PDT
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 (not working on stability any more) 2009-08-16 15:12:12 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/ee0aee8dbc9a with the var/let mess cleaned up in the lines I touched.
Comment 10 neil@parkwaycc.co.uk 2009-09-07 08:44:34 PDT
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 (not working on stability any more) 2009-09-07 11:11:42 PDT
(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?

Note You need to log in before you can comment on or make changes to this bug.