450861 - Add a "Tools > Downloads" menu item and Ctrl+J to open the Download Manager window (saved files are completely inaccessible via UI)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Brian S. Craigie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16
Build Identifier: 

Request for enhancement: To make it easy to access the files that have just been saved I would like to request to have a menu option to turn on the Download Manager window.  This would mirror the way Firefox works.

Reproducible: Always

Steps to Reproduce:
1. Locate an email with an attachment
2 [review]. Save the attachment
3 [review]. Download manager window opens and then disappears.
Actual Results:  
Now try to find the download manager window

Expected Results:  
Have a menu option to show the download manager window like in Firefox where it's on Tools -> Downloads.

This is related to 408647 and had been filed as a separate request as per the instructions in that bug (RFE).

Comment 2

[Thomas D. (:thomas8)]

There's currently no way to access the download manager, at all, without changing hidden prefs (cf. bug 408647). I. e. existing functionality is inaccessible via the UI. This isn't just enhancement --> bug.

Is it very hard to add "Downloads" menu option to the Tools menu? Say more than 5 lines of code?

Comment 3

[Thomas D. (:thomas8)]

Don't forget to add a keyboard shortcut key for this as well, preferrably ctrl+j as in FF, if that's still available.

Comment 4

[Phil Lacy]

Attachment: bring up thunderbird's own download window

works on windows XP.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 400435 [details] [diff] [review]
bring up thunderbird's own download window

>+  <key id="key_downloads"
>+       key="&downloads.key;"
>+       modifiers="accel"
>+       oncommand="Components.classes['@mozilla.org/download-manager-ui;1']
>+                            .getService(Components.interfaces.nsIDownloadManagerUI)
>+                            .show(window);"/>

Please put that in a function in appropriate js file (mailcore.js?)
And use that function in the other place too.


>+<!ENTITY downloads.tooltip "Display the progress of ongoing downloads">

Unused, no?

Should also get clarkbw to ui-r this. Seems to work fine though!

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 400435 [details] [diff] [review]
bring up thunderbird's own download window

Minusing per above

Comment 7

[David Ascher (:davida)]

Adding bryan to the list, for his ui input.  I sure want it =)

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Seems like a good addition except that we should call it something other than "downloads".  I worry that most users don't think of their attachments as downloading as much as saving...  Something as simple as "Attachments" or "Saved Attachments" might work.  Any other ideas?

Comment 9

[Phil Ringnalda (:philor)]

Is "Saved Attachments" actually accurate, or do we mean "Saved Or Opened Attachments," or do we mean "Saved Or Opened Attachments That Took More Than 100 Milliseconds And Thus None Of Yours At All Ever, Dirty POP User"?

Comment 10

[Phil Lacy]

I save mp3 or other files that are on an RSS feeds which aren't really attachments.

Also I'd like to someday utilize this window in drag and drop which currently is kind of whacky with large dnd's.

I say "Saved Files", dnd or save of complete messages should go into here.

Comment 11

[Phil Lacy]

Also the list of downloads doesn't survive the app like FF. So someone needs to make that particular config setting 2 by default and change the docs

https://developer.mozilla.org/en/Download_Manager_preferences

browser.download.manager.retention

Comment 12

[David Ascher (:davida)]

philor: how realistic is it to get POP downloaded attachments there too?

Comment 13

[Phil Ringnalda (:philor)]

Been so long since the last go-round, I forgot which way the problem works: we already do put "downloaded" POP attachments there - when you double-click "don'tletpatfindoutaboutthis.foo" and open it in the Foo application, we already do put it in the download manager and keep it there through the session, without ever showing the window at all (and I'd forgotten that whether or not to show was no longer under our control - there used to be a pref for minimum dl time before it's shown, but that's gone).

In the current scheme that's a problem only during the current session (since we clear on quit) and only if later in the session Pat saves a remote attachment or link that's slow enough to show the window; in the proposed scheme it's a problem because you might never know that attachments wind up there, until Pat goes to Tools - Foopied Thingies and finds all the attachments you've ever opened since the dawn of time, without having once saved one.

We can solve the "you have no idea opening an attachment lists it in the 'download' manager" thing by setting browser.download.manager.closeWhenDone to false, which then opens it (or maybe more accurately, doesn't close it in the same millisecond it opens), but that's massively annoying since it will getAttention to tell you that your millisecond-long download has finished.

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #8)
> Seems like a good addition except that we should call it something other than
> "downloads".  I worry that most users don't think of their attachments as
> downloading as much as saving...  Something as simple as "Attachments" or
> "Saved Attachments" might work.  Any other ideas?

We've also the possibility that extensions will use it with content tabs to save files...

Comment 15

[Phil Lacy]

ok, beside adding the function, I'll change the title of the window to "Saved Files" or is there still discussion going on.

Comment 16

[Thomas D. (:thomas8)]

Attachments that you just open for viewing are only temporarily saved in some temp folder. Maybe they should even be deleted by TB after use. Therefore, "Saved Files" might create the wrong impression that the files in download manager have all been safely saved for later use. But it might be ok to call them "saved files" anyway. I think menu caption and window title should be the same. Maybe "saved files" is actually quite good.

Comment 17

[Phil Lacy]

(In reply to comment #16)
> temp folder. Maybe they should even be deleted by TB after use. Therefore,

lets not complicate it in this bug. So far this is pretty much FF code and has the advantage of UI consistentcy and familiarity.
If the file is non existent the rt click context menu is grayed out appropriately.

> same. Maybe "saved files" is actually quite good.

Waiting for Bryan to decide then. I'd still say the familiarity of FF's 'Downloads' title is a UI plus.

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Phil, I'm comfortable with "Saved Files"

Comment 19

[Magnus Melin [:mkmelin]]

With string freeze coming up tuesday time is running short on getting this in for tb3. Got an updated patch?

Comment 20

[Phil Lacy]

Attachment: per review comments

the files have lots of untrimmed lines and openActivityMgr() needs reformatting.

Let me know if you want me to fix those nits.

Comment 21

[Phil Lacy]

Attachment: 'd' access key was taken using 'l' see further comments on previous submitted patch

Comment 22

[Thomas D. (:thomas8)]

+<!ENTITY savedFiles.label "Saved Files">
+<!ENTITY savedFiles.accesskey "l">

Why not take "s" (initial letter of *S*aved files) as access key? It's still free, and much more intuitive (for English, that is).

Did you add a keyboard shortcut command for showing Download / saved files manager? Ctrl+J is still available, and would match existing Firefox shortcut for download manager.
http://www.mozilla.org/support/thunderbird/keyboard#

Comment 23

[Phil Lacy]

(In reply to comment #22)
> Why not take "s" (initial letter of *S*aved files) as access key? It's still
> free, and much more intuitive (for English, that is).

taken by account settings menu

> 
> Did you add a keyboard shortcut command for showing Download / saved files
> manager? Ctrl+J is still available, and would match existing Firefox shortcut
> for download manager.
> http://www.mozilla.org/support/thunderbird/keyboard#

yes, shortcut is the next line down noted as '.key'

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 403186 [details] [diff] [review]
'd' access key was taken using 'l'

see further comments on previous submitted patch

>diff --git a/mail/base/content/mailCore.js b/mail/base/content/mailCore.js
>--- a/mail/base/content/mailCore.js
>+++ b/mail/base/content/mailCore.js
>@@ -290,16 +290,34 @@ function openAddonsMgr()
> }
> 
> function openActivityMgr()
> {
>   Components.classes['@mozilla.org/activity-manager-ui;1'].
>     getService(Components.interfaces.nsIActivityManagerUI).show(window);
> }
> 
>+function openSavedFilesWnd()
>+{
>+  Components.classes['@mozilla.org/download-manager-ui;1']
>+            .getService(Components.interfaces.nsIDownloadManagerUI)
>+            .show(window);
>+  setTimeout(changeTitle, 5);

Why the timeout? And if 5ms is ok isn't 0 ok too?

Anyway, this isn't really /the/ way to change the title. You should overlay the download dialog with a different title. Something like https://bug509044.bugzilla.mozilla.org/attachment.cgi?id=403113 but overlaying the actual window.

>+# savedFiles UI window. Same as FireFox downloads window

Nit: Firefox, single capital f

Comment 25

[Phil Lacy]

(In reply to comment #24)
> Why the timeout? And if 5ms is ok isn't 0 ok too?

show() returns before window gets created. 1ms worked but I didn't know if other systems would work the same, so it's just padding.

> Anyway, this isn't really /the/ way to change the title. You should overlay the

I figured there must be a trick with overlay but I couldn't find anyone this weekend on IRC who knew what I wanted and the docs on overlay indicated manifest files were necessary. I'll look into your example.

Comment 26

[Phil Lacy]

Attachment: changed to overlay

changed to overlay. thanks for review.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 404171 [details] [diff] [review]
changed to overlay

> toolkit.jar:
> % overlay chrome://global/content/customizeToolbar.xul chrome://messenger/content/customizeToolbarOverlay.xul
>+% overlay chrome://mozapps/content/downloads/downloads.xul chrome://messenger/content/downloadsOverlay.xul

It's probably best to make the scheme match what you override, i.e. chrome://messenger/content/downloads/downloadsOverlay.xul

While it's just one string, i think also savedFiles.title should be in a downloadsOverlay.dtd

Comment 28

[David Ascher (:davida)]

As such it's an l10n bug.  As we're past string freeze, that can't land until branch w/o a strong claim.  If there was a version of this patch that didn't require a new string, we could take it.

Comment 29

[Phil Lacy]

the browser.dtd has the menu labels and access key but I don't see where we use that and I'm not sure how to incorporate it since it has other strings that may be named in other dtd files. Any suggestion on using the browser.dtd?

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #29)
> the browser.dtd has the menu labels and access key but I don't see where we use
> that and I'm not sure how to incorporate it since it has other strings that may
> be named in other dtd files. Any suggestion on using the browser.dtd?

We can't. browser.dtd is part of Firefox specific code (it is in mozilla/browser) and as such we don't touch or package those files.

Comment 31

[Phil Lacy]

Attachment: patch with no strings

this is same as Firefox version and uses the browser.dtd strings. We can customize TB version after branch.

But please review consequences of bringing in browser.dtd. I don't know what problems might happen with strings that are the same in our other dtd files used.

Comment 32

[Phil Lacy]

Comment on attachment 404436 [details] [diff] [review]
patch with no strings

sorry didn't have the comment when I uploaded the patch. I'll remove flag

Comment 33

[Phil Lacy]

Comment on attachment 404436 [details] [diff] [review]
patch with no strings

can't use browser strings

Comment 34

[Phil Lacy]

Attachment: with with overlay files

changed files to more relevant location in jar and added separate dtd file. Please check the files are in the right tree. I'm not sure if they need to be moved or if review comment was only for chrome:// location.

Comment 35

[Magnus Melin [:mkmelin]]

Attachment: reviewed fix

I guess the downloads/ mapping idea wasn't all that great. I originally meant the whole dir structure should be mapped over... but maybe this is better after all. Also made you the initial developer of the code.

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 406931 [details] [diff] [review]
reviewed fix

If this works for you, also get ui-r

Comment 37

[Phil Lacy]

Comment on attachment 406931 [details] [diff] [review]
reviewed fix

thanks for review. Let me know how to flag it for checkin with the string freeze and all.

Comment 38

[Mark Banner (:standard8)]

(In reply to comment #37)
> (From update of attachment 406931 [details] [diff] [review])
> thanks for review. Let me know how to flag it for checkin with the string
> freeze and all.

I've added [needs branch] as a flag as it won't be until Thursday that we officially branch. After we've fully branched, add checkin-needed and a note in the whiteboard that it is for trunk only.

Comment 39

[Magnus Melin [:mkmelin]]

Came to mind, perhaps the menu item should be above Add-ons, to match ff? Possibly with a separator above.

Comment 40

[Phil Lacy]

Attachment: changed layout look

changed tools location, also have a screen shot.
above separator will not be in mac.

Comment 41

[Phil Lacy]

Attachment: layout picture

Comment 42

[Phil Lacy]

Comment on attachment 407405 [details] [diff] [review]
changed layout look

forgot to make Magnus changes on chrome location and author note. I'll wait till this is ok'd.

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 407405 [details] [diff] [review]
changed layout look

Sure, but don't forget to base it on the other patch

Comment 44

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 407405 [details] [diff] [review]
changed layout look

looks ok to me.  thanks for the screenshot.

Comment 45

[Phil Lacy]

Attachment: ready for checkin

Comment 46

[Phil Lacy]

Comment on attachment 407866 [details] [diff] [review]
ready for checkin

forward mkmelin+mozilla=r+
clarkbw=r+

Comment 47

[Phil Lacy]

Attachment: trunk-ready

forward +=mkmelin ui+=clarkbw

Comment 48

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/c44932faa22f

Comment 49

[Brian S. Craigie]

Thank you.

I still don't see the menu option in Thunderbird 3.  Where is it, or how do I add it?

Thanks

Brian

Comment 50

[Mark Banner (:standard8)]

(In reply to comment #49)
> I still don't see the menu option in Thunderbird 3.  Where is it, or how do I
> add it?

The patch was too late to be included in Thunderbird 3 (as it affected strings and the freeze date is earlier in that case) and therefore the menu option will be available in Thunderbird 3.1.