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

RESOLVED FIXED in seamonkey2.0b2

Status

SeaMonkey
Download & File Handling
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.0b2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

23.64 KB, patch
neil@parkwaycc.co.uk
: review+
Robert Kaiser
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
Depends on: 483241
(Assignee)

Comment 2

8 years ago
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).
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #393008 - Flags: superreview?(neil)
Attachment #393008 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 139606
(Assignee)

Updated

8 years ago
Blocks: 508899

Updated

8 years ago
Attachment #393008 - Flags: superreview?(neil)
Attachment #393008 - Flags: superreview+
Attachment #393008 - Flags: review?(neil)
Attachment #393008 - Flags: review+

Comment 4

8 years ago
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

8 years ago
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).
(Assignee)

Comment 6

8 years ago
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.
Attachment #393008 - Attachment is obsolete: true
Attachment #394716 - Flags: superreview+
Attachment #394716 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Target Milestone: --- → seamonkey2.0b2
(Assignee)

Comment 7

8 years ago
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

8 years ago
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!
Attachment #394716 - Flags: review?(neil) → review+
(Assignee)

Comment 9

8 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/ee0aee8dbc9a with the var/let mess cleaned up in the lines I touched.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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...

Updated

8 years ago
Blocks: 515053
(Assignee)

Comment 11

8 years ago
(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?
You need to log in before you can comment on or make changes to this bug.