239136 - Thunderbird needs to integrate with the new download manager

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Scott MacGregor]

tracking bug for converting thunderbird over to use the new download manager
firefox has.

Comment 1

[Scott MacGregor]

I have a patch that converts us to use it but several additional things need to
happen:

1) Options button needs to be modified to bring up the correct thunderbird
options panel

2) Pause/Remove don't work for mail and need removed

3) Retry won't work for us. Might need some back end mail work to redownload
imap urls

4) Fork the DTD file for the download manager so we can change the wording to a
mail context.

Comment 2

[Scott MacGregor]

Some other issues that need addressed:

sometimes we don't report full progress on an attachment dialog but we still
finish downloading the attachment (the server reported an approximate download
size). This leaves the download manager saying the download is in progress even
though it is done and the attachment has been opened.

Comment 3

[Scott MacGregor]

we aren't going to do this for 1.0

Comment 4

[Scott MacGregor]

I'm reviving this bug in order to fix Bug 320744.

Comment 5

[Scott MacGregor]

Attachment: first part of the fix

For some reason, this was a heck of a lot easier to do the second time around.

Build foo to convert thunderbird to use toolkit's download manager. This + bsmedberg's xul runner checkin from a few minutes ago means the only thunderbird ifdef left in toolkit\components is for typeaheadfind!

Last time I modified xpfe\components\build2, I broke XUL Runner, I think I'm in the clear with this change as xul runner should already be getting the download manager from toolkit.

Still to do:
1) substitute Attachments for Downloads in the download manager UI
2) look into hiding resume/pause and retry until (if) those can be implemented for protocols like imap, but I think we can turn it on before that happens.

Comment 6

[Benjamin Smedberg]

Comment on attachment 209522 [details] [diff] [review]
first part of the fix

>Index: mail/installer/removed-files.in

>+components/nsProgressDialog.js
>+components/nsHelperAppDlg.js

Why are you removing these? These are not part of the download manager; they are shipped from http://lxr.mozilla.org/mozilla/source/embedding/components/ui/progressDlg/nsProgressDialog.js and http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in unless I'm vastly mistaken, and should be retained.

ditto for packages-static

r=me with that change, or please re-request review if I'm mistaken somehow

Comment 7

[Scott MacGregor]

I started to land this on the trunk. There are still some UI behaviors we're going to have to iron out. 

Comment 8

[Scott MacGregor]

Comment on attachment 209522 [details] [diff] [review]
first part of the fix

nominating now so I don't lose track of this bug. We'll look for branch approval after the dust has settled in a week or so.

Comment 9

[Scott MacGregor]

Attachment: package downloads.xpt

I forgot to package downloads.xpt

Comment 10

[Scott MacGregor]

For someone reason, scrollbars don't show up for the download manager when you have several attachments that you have opened during the session. The dialog just clips the richlistbox. Very strange. Same dialog in Firefox, scroll bars appear just fine.

I'm also noticing that when I click on an entry in the download dialog, I get the following JS error which I also don't see in Firefox:

Error: listbox has no properties
Source File: chrome://global/content/bindings/richlistbox.xml
Line: 563

Something isn't quite right yet. 

Comment 11

[Mike Cowperthwaite]

(In reply to comment #10)
> For someone reason, scrollbars don't show up for the download manager when you
> have several attachments that you have opened during the session. The dialog
> just clips the richlistbox. Very strange. Same dialog in Firefox, scroll bars
> appear just fine.

This isn't limited to "attachments opened this session."  I just encountered this dialog for the first time, and the list of "downloads" -- attachments -- stretches back in time for I don't  know how long: the list is so long that I can't get to the bottom to find (presumably) the "Clean Up" button.


> Something isn't quite right yet. 

Add to this list of "not quite right" things the fact that the DL manager is opened when you double-click on an attachment in an already-received POP message.  It also appears when double-clicking on an attachment in an IMAP message that's already been d/l'd (Viewed Inline).  I would really prefer not 
to have this window popping up for something that's already been downloaded.  

Was the intent to use this dialog to show progress of downloading an IMAP attachment?

Comment 12

[Mike Cowperthwaite]

(In reply to comment #11)
> This isn't limited to "attachments opened this session."  I just encountered
> this dialog for the first time, and the list of "downloads" -- attachments --
> stretches back in time for I don't  know how long: the list is so long that I
> can't get to the bottom to find (presumably) the "Clean Up" button.

Additional: some of the items apparently are listed internally as "completed" and show the "Open" and "Remove" links.  Most of the items are are apparently "incomplete" and show only the "Cancel" link.  When I click Cancel, the JS console shows:
   Error: gDownloadsView.clearSelection is not a function
   Source File: chrome://mozapps/content/downloads/downloads.js
   Line: 236
However, the item is then removed from the list, as I was hoping.

I also note that whether clicking Remove (which works) or Cancel, there is a fairly lengthy delay, almost 3 sec, before the list updates.

Comment 13

[Scott MacGregor]

Attachment: fix CSS for the qute theme

The CSS was out of date for qute, in particular downloads.xml. This was causing the bindings for the richlistbox to not load. That in turn led to the lack of scroll bars and the JS errors when you tried to interact with download items in the window.

Comment 14

[Scott MacGregor]

Attachment: migrate to the new download manager by deleting downloads.rdf

The first time the new download manager opens after opening an attachment, we don't want to show the user all the old data we've accumulated in downloads.rdf over the years. So do a one time migration where we remove downloads.rdf when the first mail 3 pane window comes up. Alternatively, I could have done this in C++ inside a method in the mail session when it first gets created, but it was so much easier in JS :).

From this point forward we'll use the browser retention pref (which we default to remove on exit) for deciding when to clean up downloads.rdf.

I made the pref a version pref instead of a simple boolean in case we have to do something like this again in the future.

Comment 15

[Scott MacGregor]

Comment on attachment 209522 [details] [diff] [review]
first part of the fix

This thunderbird change is ready for the branch now that I've fixed the issues that were bothering me.

Comment 16

[Scott MacGregor]

Attachment: tweak default pref settings

These pref changes make us behave a little bit more like the old progress dialog. Most notably:

1) Close the download manager after the attachment has been downloaded
2) Don't show the download manager at all if the download is already complete, or completes within the first 100ms of the download manager tracking the download.

Comment 17

[Scott MacGregor]

all the patches are in the branch and the trunk.

The tweaked pref settings should address mcow's points in comment 11.

Comment 18

[Mark Banner (:standard8)]

Attachment: Follow up - remove redundant ifdefs

As I see Thunderbird isn't using xpfe/components/download-manager any more, these ifdefs should no longer be needed

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 216960 [details] [diff] [review]
Follow up - remove redundant ifdefs

This patch now checked in as well.

Comment 20

[Jean-Michel Reghem]

(In reply to comment #16)
> 
> 1) Close the download manager after the attachment has been downloaded
> 2) Don't show the download manager at all if the download is already complete,
> or completes within the first 100ms of the download manager tracking the
> download.
> 

--> yes ... but it miss a optional confirmation windows ... In Firefox, you have the possibility to open the download dialog event when save to disk is set to default for a know type of file.
See the bug 342154 I've filled 2 days ago ...

Maybe it is wanted ... but for user which comes from 1.5.0.4 to 2.0 the first reaction is "i cannot open attachement" because all is going in silent mode 

Comment 21

[Oshyan Greene]

I would also like to note the problematic nature of this "improvement". I'm not sure I understand the value of changing to the FF download manager in itself, but assuming it's a good thing, we do at least need to be able to access it (as far as I know this is not currently possible in FF), and to set preferences for show/hide after "download", etc. as in FF.

More particularly this is an example of why the new functionality is bad. Naive (average) users are looking for simplicity. They will tend to simply Open a file and work on it (say it is a .doc) then save changes and close. In TB a direct file open saves to the Temp folders, which are often cleaned, overwritten, etc. so data saved there is not safe. For this reason I have impressed upon my staff the importance of saving *first*, then opening from the saved location. This was easy in TB 1.5x because you could save then open in 2 smooth, linked steps. In TB 2.x you have to save, then in a separate action outside of TB you navigate to where you saved the file (in our organization this is often many levels deep inside a documents folder), and then open it. While this is not an extreme amount of extra work, when a lot of document interchange occurs through email daily as it does here this can be a great frustration, especially for naive users who required training just to even do this correctly. Nevermind trying to teach them that email should not really be used for such document exchange. ;)

A revert to old behavior, or emulation of it in the new system, is strongly requested and advised given the above use case scenario. We are risking great frustration, alienation and data loss for less experienced users here.

Comment 22

[Jean-Michel Reghem]

(In reply to comment #21)

> 
> A revert to old behavior, or emulation of it in the new system, is strongly
> requested and advised given the above use case scenario. We are risking great
> frustration, alienation and data loss for less experienced users here.
> 


Oshyan ... I'm agree with you ...
as this bug is Resolved/fixed, see my open bug (blocking TB 2.0) here, discussing a similar problem of the new download manager in TB
Bug 342154