483241 - Fully implement download progress dialogs

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Robert Kaiser]

The current patch for bug 381157 has a stub implementation for download progress dialogs, and it actually might be a good idea to initially land with them deactivated and only activate them when a fully implemented version is available, which we probably should have in place before beta.
This way of doing things also goes with smaller patches for separate chunks of the code, which helps ease review.

Comment 1

[Robert Kaiser]

Attachment: screen shot of progress dialog with both context menus

I basically have a patch for this ready, I'm just waiting for Callek to put up a new download manager patch as that will change around the locations of some files I'm patching.

That progress dialog implementation is completely done from scratch, with the objective to be cleaner than the old one, both in UI and code.

Tooltips on the file name and the download host show respectively the full local path and the full download URL, and on resumable downloads we show a resume icon instead of the pause/play and cancel ones, I think everything else of the UI is visible in this screen shot.

Comment 2

[Robert Kaiser]

Attachment: screen shot of progress dialog with finished/canceled downloads

Here is a version of a canceled and a finished download in progress dialogs, also showing the retry icons, by the way (and the code makes the dialog switch to the new download ID of the retried download, so that its info is shown in the same dialog, as a side note).

Comment 3

[Caio Tiago Oliveira (asrail)]

The new ui makes me wonder about accessibility.

Is there a option to use text instead or alongside icons for the play, pause, restart, cancel buttons?
They look too tiny for people with issues using the mouse.
Are they on taborder too, right?

Is the host on taborder and it will copy the whole text on control+c?
I meant, can I use the whole functionality without using the mouse?

Comment 4

[Philip Chee]

> The new ui makes me wonder about accessibility.
Yeah, I think we should ask Marco Zehe about this as well.

> Is there a option to use text instead or alongside icons for the play, pause,
> restart, cancel buttons?
> They look too tiny for people with issues using the mouse.
Yes, they don't look like proper buttons to me either. Perhaps we could put them in the dialog button row and make them look more like the (new) video controls.

> Are they on taborder too, right?
Ditto.

Comment 5

[Robert Kaiser]

(In reply to comment #3)
> The new ui makes me wonder about accessibility.

As Ratty said, I think we should ask Marco to test it once it has landed, I expect it's possible there are things we can improve, but I have tried as well as I know to address things in the code already.

> Is there a option to use text instead or alongside icons for the play, pause,
> restart, cancel buttons?
> They look too tiny for people with issues using the mouse.
> Are they on taborder too, right?

They are in the tab order and have a tooltip that describes what they are doing in text (I think visually impaired people can "read" tooltips).

> Is the host on taborder and it will copy the whole text on control+c?

both the download file name (target) and the source host name are on the tab order, Ctrl+C is not hooked up to anything yet but might be an interesting followup, IMHO.

> I meant, can I use the whole functionality without using the mouse?

I can use everything from my keyboard, but I haven't found out (yet) how to open a popup or context menu without a context-menu-key like I have on this keyboard and like many "Windows"-Keyboards have. This might be one thing to look into as a followup as well.

Comment 6

[Philip Chee]

On windows SeaMonkey uses SHIFT-F10 to pop up the context menu. I have no idea how this works on *nix and osx.

Phil

Comment 7

[[e-mail]]

I think the big problem here is that this doesn't look like a dialog ;-) This would be most easily dealt with by restoring the labels from the old dialog and using proper dialog buttons for the actions (possibly with icons). Note that since progress dialogs don't block access to the download manager, I think we shouldn't necessarily want all of the functionality available in the dialog.

I don't have an issue with not showing the full destination - Reveal and/or Copy can be used for that if necessary (Reveal could possibly be a dual menubutton option on the Launch button). I would like to see the source URL though not necessarily selectable (speaking of which the old dialog used the obsolete scrollfield class instead of the plain class that superseded it).

I noticed that the elapsed time seems to be missing from the "Finished" screenshot. If the dialog is open when the download finishes then we should leave the time there even if we can't repopulate it later. I also think that the title of a finished download should retain the "100% of" prefix.

I'm not convinced that hiding the progressmeter is useful - it should be pretty clear when the download is active and when it is not, unless again we don't store the percentage of a cancelled download (which I can believe).

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> I think the big problem here is that this doesn't look like a dialog ;-) This
> would be most easily dealt with by restoring the labels from the old dialog and
> using proper dialog buttons for the actions (possibly with icons). Note that
> since progress dialogs don't block access to the download manager, I think we
> shouldn't necessarily want all of the functionality available in the dialog.

Grr, and I was so happy I got rid of this ugly line of buttons - and to me, this always was and still is more of a download progress status window than a normal dialog. Real dialogs are not supposed to be dynamic status displays, but decision points, IMHO.

> I don't have an issue with not showing the full destination - Reveal and/or
> Copy can be used for that if necessary (Reveal could possibly be a dual
> menubutton option on the Launch button). I would like to see the source URL
> though not necessarily selectable (speaking of which the old dialog used the
> obsolete scrollfield class instead of the plain class that superseded it).

Is seeing the source URL as a tooltip on the host name not enough?

> I noticed that the elapsed time seems to be missing from the "Finished"
> screenshot. If the dialog is open when the download finishes then we should
> leave the time there even if we can't repopulate it later. I also think that
> the title of a finished download should retain the "100% of" prefix.

Hmm, this should be fixable, true. Actually, I think the download manager even stores start and end time of downloads, so it should even be something we can repopulate.

> I'm not convinced that hiding the progressmeter is useful - it should be pretty
> clear when the download is active and when it is not, unless again we don't
> store the percentage of a cancelled download (which I can believe).

Progressmeter doesn't support a disabled look, and therefore an almost finished download (99.9% or so) and finished download as well as potentially a an active and canceled download would all look active (IMHO, displaying a progressbar in any piece of UI implicates that some activity is going on). I also very much think that displaying any piece of UI that doesn't transport information or allow interaction is bad design, at least if it doesn't get a disabled look.

Comment 9

[Robert Kaiser]

Attachment: patch, v1: implement progress dialogs

Here's the first iteration of the patch, applying on top of Callek's latest download manager patch and my download manager UI patch (I'm reusing downloadmanager.properties here).

I should have fixed the nit on displaying "100%" for finished downloads, the elapsed time actually should work, only that the dialog I have in the screen shot was only shown first after it has been finished (file picker was up during the actual download phase, it's such a small file) and repopulation only works by digging into the database itself and not through the public interfaces (endTime is not exposed in nsIDownload).

Comment 10

[[e-mail]]

Comment on attachment 373104 [details] [diff] [review]
patch, v1: implement progress dialogs

>+  if (aDownloadData instanceof Components.interfaces.nsIDownload) {
>+    // progress dialog hands us a download object
>+    var dlid = aDownloadData.id;
>+    var file = aDownloadData.targetFile;
>+  }
>+  else {
>+    // download manager hands us a treeview data item
>+    var dlid = aDownloadData.dlid;
>+    var file = getLocalFileFromNativePathOrUrl(aDownloadData.file);
>+  }
Eww. Would it be easier to pass the id and file as separate parameters?

>diff --git a/suite/themes/classic/communicator/downloads/downloadmanager.css b/suite/themes/classic/communicator/downloads/downloadmanager.css
Ah, this explains why the dialog looked so bad in Modern ;-)

>+#fileName, #fileSource {
>+  border: 1px solid transparent;
>+  /* a 1px is used for border, make margins 1px smaller */
>+  margin-top: 0px;
>+  margin-bottom: 1px;
>+  -moz-margin-start: 5px;
>+  -moz-margin-end: 4px;
>+}
>+#fileName:focus,
>+#fileSource:focus {
>+  border: 1px dotted -moz-DialogText;
>+}
This should use outline (and -moz-outline-offset if necessary).

>+.mini-button {
>+  -moz-appearance: none;
>+  background-color: transparent;
>+  border: none;
>+  padding: 0;
>+  margin: 0;
>+  min-width: 0;
>+  min-height: 0;
>+}
>+
>+.mini-button > .button-box {
>+  -moz-appearance: none;
>+  padding: 0 !important;
>+}
You might be better off using <img> for these...

Comment 11

[[e-mail]]

Comment on attachment 373104 [details] [diff] [review]
patch, v1: implement progress dialogs

>+  document.getElementById("fileSource").value = gDownload.source.host;
Technically the source might not have a host, for instance a data: URL. Also, you shouldn't use separate labels, you should use a l10n property to combine the From and whatever string you end up using.

>+  var isActive;
Unused?

>+          gProgressMeter.max = gDownload.size;
>+        gProgressMeter.value = gDownload.amountTransferred;
Sadly max is limited to 2 << 29 (nscoord_MAX / 2); between that and 2 << 31 you get nonlinear progress while above 2 << 31 you get silly things happening.

>+        gProgressText.hidden = true;
I don't think that's right ;-)

>+    var maxBytes = (gDownload.size == null) ? -1 : gDownload.size;
>+    var speed = (gDownload.speed == null) ? -1 : gDownload.speed;
These can't equal null, because they're numbers. They could be zero, but then I don't see the point of changing them to -1. (Actually the .idl says that maxBytes could be 0xFFFFFFFFFFFFFFFF which is a bit annoying as -1 would be much easier from the JS point o

>+    var maxBytes = (gDownload.size == null) ? -1 : gDownload.size;
>+    var speed = (gDownload.speed == null) ? -1 : gDownload.speed;
These can't equal null, because they're numbers. They could be zero, but then I don't see the point of changing them to -1, since you check for > 0 anyway.

>+  if (gDlActive)
>+    gEndTime = Date.now();
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!)

>+  onDownloadStateChange: function(aState, aDownload) {
>+    // first, check if we are retrying and this is the new download starting
Remind me to find out from sdwilsh why you're not getting the same download.

>+    if (aDownload.id == gDownload.id) {
If the ids are the same, then the objects should be the same too... otherwise you would have to copy aDownload to gDownload all the time ;-)

>+    if (gCloseWhenDone.checked &&
>+        (aDownload.state == nsIDownloadManager.DOWNLOAD_FINISHED)) {
Doesn't this need to be inside the above block? Also, hardly worth updating stuff if we're just going to close the window. Also, what if the download finishes before the window opens?

>+        buttons="cancel"
>+        buttonlabelcancel="&closeDialog.label;"
>+        buttonaccesskeycancel="&closeDialog.accesskey;"
The close button looks lonely. If you don't want to put all the features in dialog buttons (this is a dialog, right?) then we should make it a window.

>+    <popup id="fileContext">
These menus need to be disabled (or not show) as appropriate.

>+  <label id="fileName" crop="center"
>+         context="fileContext" popup="fileContext"
>+         style="-moz-user-focus: normal;"/>
Style nit: put the focusable elements inside boxes so that the focus ring shrinks to fit.

>+    <label id="fromLabel" value="&from.label;"/>
>+    <label id="fileSource" crop="center" flex="1"
>+           context="sourceContext" popup="sourceContext"
>+           style="-moz-user-focus: normal;"/>
[Should be one label, assuming you're not using headers.]

>+  <hbox>
>+    <label id="dlStatus" flex="1"/>
>+    <button id="pauseButton" class="mini-button"
>+            command="cmd_pause" tooltiptext="&cmd.pause.label;"/>
>+    <button id="resumeButton" class="mini-button"
>+            command="cmd_resume" tooltiptext="&cmd.resume.label;"/>
>+    <button id="retryButton" class="mini-button"
>+            command="cmd_retry" tooltiptext="&cmd.retry.label;"/>
>+    <button id="cancelButton" class="mini-button"
>+            command="cmd_cancel" tooltiptext="&cmd.cancel.label;"/>
>+  </hbox>
The problem here (at least with my fonts) is that the buttons are 2px taller than the label, thus leaving an ugly gap between this line and the next.
<hbox>
  <vbox flex="1">
    <label/>
    <label/>
    <label/>
  </vbox>
  <button/>
  ...
</hbox>
is the best I could come up with.

>+    <progressmeter id="progressMeter" mode="determined" value="0" flex="1"/>
[Probably unnecessary to set the value here.]

>+    <label id="progressText" value="&#160;"/>
For what I think you need it for, value="" should work, and you should probably set value="" on the other "empty" labels too.

> # LOCALIZATION NOTE (timeSingle): %1$S time number; %2$S time unit
> # example: 1 minute; 11 hours
> timeSingle=%1$S %2$S
> # LOCALIZATION NOTE (timeDouble):
> # %1$S time number; %2$S time unit; %3$S time sub number; %4$S time sub unit
> # example: 11 hours, 2 minutes; 1 day, 22 hours
> timeDouble=%1$S %2$S, %3$S %4$S
> 
>+# LOCALIZATION NOTE (timeElapsedSingle): %1$S time number; %2$S time unit
>+# example: 1 minute elapsed; 11 hours elapsed
>+timeElapsedSingle=%1$S %2$S elapsed
>+# LOCALIZATION NOTE (timeElapsedDouble):
>+# %1$S time number; %2$S time unit; %3$S time sub number; %4$S time sub unit
>+# example: 11 hours, 2 minutes elapsed; 1 day, 22 hours elapsed
>+timeElapsedDouble=%1$S %2$S, %3$S %4$S elapsed
You see, by not having headers you have to duplicate all these strings ;-)

>+<!ENTITY cmd.pause.label                 "Pause">
>+<!ENTITY cmd.resume.label                "Resume">
>+<!ENTITY cmd.retry.label                 "Retry">
>+<!ENTITY cmd.cancel.label                "Cancel">
Except these are tooltips...

Comment 12

[[e-mail]]

Comment on attachment 372246 [details]
screen shot of progress dialog with finished/canceled downloads

OK, this looks a bit bare; firstly we should usually have an end time even for an instantaneous download which I hope will display as "A few seconds elapsed"; secondly I think that the download status should replace the time remaining i.e.
<filename>
From <host>
<size>
<duration> elapsed
<status> [- <duration> remaining (if available)]
<progressmeter> (always!)

Comment 13

[[e-mail]]

I don't suppose anyone knows what the behaviour of Firefox is when opening a large file with a helper application? XPFE behaviour was to display a progress dialog but automatically close it, ignoring the preference.

Comment 14

[[e-mail]]

Oh, and I'd very much like to be able to open a progress dialog from the download manager, even if it's only for an active download.

Comment 15

[Robert Kaiser]

(In reply to comment #10)
> (From update of attachment 373104 [details] [diff] [review])
> >+  if (aDownloadData instanceof Components.interfaces.nsIDownload) {
> >+    // progress dialog hands us a download object
> >+    var dlid = aDownloadData.id;
> >+    var file = aDownloadData.targetFile;
> >+  }
> >+  else {
> >+    // download manager hands us a treeview data item
> >+    var dlid = aDownloadData.dlid;
> >+    var file = getLocalFileFromNativePathOrUrl(aDownloadData.file);
> >+  }
> Eww. Would it be easier to pass the id and file as separate parameters?

Actually, it would probably be nice from an interface POV to just always make the param the nsIDownload instance, but then we'd need to re-fetch data that we already pulled out into the array powering the tree. I see why you dislike it the way it is, I also don't really like passing both an ID and a file separately, I'm not sure what's the really nice and good way to do it. What I did here feels like the best compromise for me right now.

> >diff --git a/suite/themes/classic/communicator/downloads/downloadmanager.css b/suite/themes/classic/communicator/downloads/downloadmanager.css
> Ah, this explains why the dialog looked so bad in Modern ;-)

Uh, right, forgot to do this in Modern as well.

> >+#fileName, #fileSource {
> >+  border: 1px solid transparent;
> >+  /* a 1px is used for border, make margins 1px smaller */
> >+  margin-top: 0px;
> >+  margin-bottom: 1px;
> >+  -moz-margin-start: 5px;
> >+  -moz-margin-end: 4px;
> >+}
> >+#fileName:focus,
> >+#fileSource:focus {
> >+  border: 1px dotted -moz-DialogText;
> >+}
> This should use outline (and -moz-outline-offset if necessary).

I tired that at first, but at least here on Linux, it doesn't clean up the focus rings nicely and not set them all around the element for the source label. There must be a reason why it's done with borders in other such cases I found.

> >+.mini-button {
> >+  -moz-appearance: none;
> >+  background-color: transparent;
> >+  border: none;
> >+  padding: 0;
> >+  margin: 0;
> >+  min-width: 0;
> >+  min-height: 0;
> >+}
> >+
> >+.mini-button > .button-box {
> >+  -moz-appearance: none;
> >+  padding: 0 !important;
> >+}
> You might be better off using <img> for these...

Why? so I can implement all the characteristics myself to make it accessible and acting like a button? I basically do the same thing as that toolkit is doing in their dlmgr richlistbox cells, and I at least know that this works well with a11y and all that stuff...

(In reply to comment #11)
> (From update of attachment 373104 [details] [diff] [review])
> >+  document.getElementById("fileSource").value = gDownload.source.host;
> Technically the source might not have a host, for instance a data: URL. Also,
> you shouldn't use separate labels, you should use a l10n property to combine
> the From and whatever string you end up using.

Hmm, what should we use instead of the host when we have none?
I really like the separate labels because it clearly showed that the source (host name) had the focus when tabbed to or right-clicked on, and not the "From", and also the middle-crop potentially works better on the host alone than on "From %S", I guess - but as you wish, this isn't that hard to change ;-)

> >+          gProgressMeter.max = gDownload.size;
> >+        gProgressMeter.value = gDownload.amountTransferred;
> Sadly max is limited to 2 << 29 (nscoord_MAX / 2); between that and 2 << 31 you
> get nonlinear progress while above 2 << 31 you get silly things happening.

Bummer. OK, back to a standard max of 100 and using percent progress. Would have been a nice idea...

> >+        gProgressText.hidden = true;
> I don't think that's right ;-)

I pretty much dislike showing UI elements that contain no information at all, but as you wish. I still will hide progressText when shown an undetermined progressmeter, OK? (IMHO, there it's even uglier to not hide the progressmeter itself as it's animating when the download is actually inactive. Good that I don't use those dialogs usually.)

> >+    var maxBytes = (gDownload.size == null) ? -1 : gDownload.size;
> >+    var speed = (gDownload.speed == null) ? -1 : gDownload.speed;
> These can't equal null, because they're numbers. They could be zero, but then I
> don't see the point of changing them to -1. (Actually the .idl says that
> maxBytes could be 0xFFFFFFFFFFFFFFFF which is a bit annoying as -1 would be
> much easier from the JS point o

I copied all this from the download manager tree view where you reviewed it already, and to there from the toolkit files that determine the remaining time. I don't claim that I understand that piece of code, I'm happy if it works. So what was the change that all three places, including toolkit, need? Just use them directly, without ?:, and treat them as numbers as we are are checking for >0 below anyway?

> >+  if (gDlActive)
> >+    gEndTime = Date.now();
> 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!)

Hmm, that could work. It's a bit lame that we need to do that at all, but unfortunately nsIDownload doesn't expose the end time stored in the DM DB.

> >+  onDownloadStateChange: function(aState, aDownload) {
> >+    // first, check if we are retrying and this is the new download starting
> Remind me to find out from sdwilsh why you're not getting the same download.

Sure, will do ;-)

> >+    if (aDownload.id == gDownload.id) {
> If the ids are the same, then the objects should be the same too... otherwise
> you would have to copy aDownload to gDownload all the time ;-)

True, but I though checking just .id would be cheaper than the whole object - isn't it?

> >+    if (gCloseWhenDone.checked &&
> >+        (aDownload.state == nsIDownloadManager.DOWNLOAD_FINISHED)) {
> Doesn't this need to be inside the above block? Also, hardly worth updating
> stuff if we're just going to close the window. Also, what if the download
> finishes before the window opens?

Right, inside the block and before the updates it is. Not sure about the fast downloads that finish before the dialog opens, maybe we should care to check for that at startup and close again right away?

> >+        buttons="cancel"
> >+        buttonlabelcancel="&closeDialog.label;"
> >+        buttonaccesskeycancel="&closeDialog.accesskey;"
> The close button looks lonely. If you don't want to put all the features in
> dialog buttons (this is a dialog, right?) then we should make it a window.

It's no classical dialog, IMHO, but it needs the close button in some form. All the features as classical dialog buttons looks too heavy for me (actually, making this a classic dialog probably makes an unexperienced user think he needs to decide something and click the "whatever" button, and that isn't the case here - perhaps that's why I don't feel well with the usual dialog buttons etc. here, I see this as per-file download status displays rather than the classic "question-answer" style we refer to as a "dialog").
If we convert it to a <window>, I still don't see how the close button wouldn't feel lonely though, as we need it in some form.

> >+    <popup id="fileContext">
> These menus need to be disabled (or not show) as appropriate.

I thought that would happen automatically with the menuitem command attributes but I see it isn't. Did I overlook something here?

> You see, by not having headers you have to duplicate all these strings ;-)

True, but if it happens to look nicer, I'm very much illing to do that.

> >+<!ENTITY cmd.pause.label                 "Pause">
> >+<!ENTITY cmd.resume.label                "Resume">
> >+<!ENTITY cmd.retry.label                 "Retry">
> >+<!ENTITY cmd.cancel.label                "Cancel">
> Except these are tooltips...

Oh, right ;-)

(In reply to comment #12)
> secondly I think that the download status should replace the time remaining

Interesting idea, I'll look at that!

(In reply to comment #13)
> I don't suppose anyone knows what the behaviour of Firefox is when opening a
> large file with a helper application? XPFE behaviour was to display a progress
> dialog but automatically close it, ignoring the preference.

Hmm, not sure, would need to investigate that. I think our current (and probably also xpfe) behavior is at least to show either download manager or progress dialog depending on pref, dlmgr seems to stay open when finished, I never used progress windows much (I only took over this work because nobody was doing it, it was said to be needed, I already knew the backend integration from dlmgr UI work, and I found the old dialogs to be too ugly for my taste).

(In reply to comment #14)
> Oh, and I'd very much like to be able to open a progress dialog from the
> download manager, even if it's only for an active download.

That's bug 474620, already filed when working on dlmgr UI and didn't know much on progress "dialogs" yet. :)

Comment 16

[Robert Kaiser]

(In reply to comment #12)
> (From update of attachment 372246 [details])
> OK, this looks a bit bare; firstly we should usually have an end time even for
> an instantaneous download which I hope will display as "A few seconds elapsed";
> secondly I think that the download status should replace the time remaining
> i.e.
> <filename>
> From <host>
> <size>
> <duration> elapsed
> <status> [- <duration> remaining (if available)]
> <progressmeter> (always!)

So, do I get this right, this is just to fill whitespace and basically means moving the status away from the size?

Comment 17

[[e-mail]]

(In reply to comment #15)
> Actually, it would probably be nice from an interface POV to just always make
> the param the nsIDownload instance, but then we'd need to re-fetch data that we
> already pulled out into the array powering the tree.
And you don't get nsIDownload instances in the tree normally, right?
How about you pass a "fake" nsIDownload instance with just the id and file?

> I tired that at first, but at least here on Linux, it doesn't clean up the
> focus rings nicely and not set them all around the element for the source
> label. There must be a reason why it's done with borders in other such cases I
> found.
Hmm... I guess I could try some alternative styles for myself.

> Why? so I can implement all the characteristics myself to make it accessible
> and acting like a button? I basically do the same thing as that toolkit is
> doing in their dlmgr richlistbox cells, and I at least know that this works
> well with a11y and all that stuff...
Good point, we don't want to get the a11y stuff wrong.

> Hmm, what should we use instead of the host when we have none?
alert() apparently uses the prePath when it can't use the host.

> > >+        gProgressText.hidden = true;
> > I don't think that's right ;-)
Hmm, I think maybe I misread the code here. Looking at it again, I can't remember why I commented on that line of code, but I though that it seems odd that the determined state of the progressmeter depends on the hidden state...

> I copied all this from the download manager tree view where you reviewed it
> already, and to there from the toolkit files that determine the remaining time.
Bah, I must have overlooked that :-( Sorry.

> I don't claim that I understand that piece of code, I'm happy if it works. So
> what was the change that all three places, including toolkit, need? Just use
> them directly, without ?:, and treat them as numbers as we are are checking for
> >0 below anyway?
Right. [What's the toolkit change you're referring to?]

> > >+    if (aDownload.id == gDownload.id) {
> > If the ids are the same, then the objects should be the same too... otherwise
> > you would have to copy aDownload to gDownload all the time ;-)
> True, but I though checking just .id would be cheaper than the whole object -
> isn't it?
Not sure... it's two property accesses and a string compare, versus some xpconnect magic - maybe mrbkap would know?

> > >+    if (gCloseWhenDone.checked &&
> > >+        (aDownload.state == nsIDownloadManager.DOWNLOAD_FINISHED)) {
> > Doesn't this need to be inside the above block? Also, hardly worth updating
> > stuff if we're just going to close the window. Also, what if the download
> > finishes before the window opens?
> Right, inside the block and before the updates it is. Not sure about the fast
> downloads that finish before the dialog opens, maybe we should care to check
> for that at startup and close again right away?
I guess that's the best we can do. (Does Firefox have this problem?)

> It's no classical dialog, IMHO, but it needs the close button in some form.
Actually we have precedents e.g. Page Info.

> If we convert it to a <window>, I still don't see how the close button wouldn't
> feel lonely though, as we need it in some form.
We'd have an OS close widget...

> > >+    <popup id="fileContext">
> > These menus need to be disabled (or not show) as appropriate.
> I thought that would happen automatically with the menuitem command attributes
> but I see it isn't. Did I overlook something here?
Now that I look at it I think you meant to call onCommandUpdate somewhere.

Comment 18

[Robert Kaiser]

(In reply to comment #17)
> > I don't claim that I understand that piece of code, I'm happy if it works. So
> > what was the change that all three places, including toolkit, need? Just use
> > them directly, without ?:, and treat them as numbers as we are are checking for
> > >0 below anyway?
> Right. [What's the toolkit change you're referring to?]

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/src/DownloadUtils.jsm#166

> > > >+    if (aDownload.id == gDownload.id) {
> > > If the ids are the same, then the objects should be the same too... otherwise
> > > you would have to copy aDownload to gDownload all the time ;-)
> > True, but I though checking just .id would be cheaper than the whole object -
> > isn't it?
> Not sure... it's two property accesses and a string compare, versus some
> xpconnect magic - maybe mrbkap would know?

I don't find docs about this, I'm just concerned about perf in this path because the download listener is probably being called a lot.
I changed to object comparison in the upcoming new patch.

Comment 19

[Robert Kaiser]

Attachment: patch, v2: address comments

Here's an updated patch that hopefully adresses all comments (except the integration with download manager that should go into the already filed followup).

Comment 20

[[e-mail]]

(In reply to comment #15)
> (In reply to comment #10)
> > >+#fileName:focus,
> > >+#fileSource:focus {
> > >+  border: 1px dotted -moz-DialogText;
> > >+}
> > This should use outline (and -moz-outline-offset if necessary).
> I tired that at first, but at least here on Linux, it doesn't clean up the
> focus rings nicely and not set them all around the element for the source label.
Strange, because it worked perfectly for me when I tried it on Windows XP...

Comment 21

[[e-mail]]

Comment on attachment 379574 [details] [diff] [review]
patch, v2: address comments

>-function showDownload(aDownloadData)
>+function showDownload(aDownload)
> {
>-  var file = getLocalFileFromNativePathOrUrl(aDownloadData.file);
>+  var file = aDownload.targetFile;
Does this just use the file? Could just pass the file object directly instead.

>+  var fromString = gDownload.source.host
It sucks, but it turns out this could throw, so you'll have to wrap this in try/catch and then test it afterwards. r+sr=me with this fixed.

>+                oncommandupdate="ProgressDlgController.onCommandUpdate()"/>
Nit: missing ;

Comment 22

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/fc43764b4eea - Please file followups for any issues arising.

Comment 23

[Robert Kaiser]

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query