914517 - Attachments "Save as..." does not create an entry in "Saved Files" list aka Download Manager window

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[David O.]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130905180733

Steps to reproduce:

User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.0
Application Build ID 	20130828132738

1. received an EMail with one or several Attachments (eg. PDFs)
2. right-click on the attachment and "Save as..." from context menu
3. choose location
4. Press CTRL-J to open Saved Files Dialogue


Actual results:

The saved attachment file is not listed in the Saved Files dialogue


Expected results:

The above attachment should be listen in the Saved Files dialogue, as it would be when clicking / double-clicking on the attachment and then select either Open or Save File...

Comment 1

[David O.]

Might have similarities with bug 571858

Comment 2

[Thomas D. (:thomas8)]

David, thanks for your bug report, and sorry for the inconvenience...

There's a number of things going wrong with "Saved Files" aka Download manager in TB currently.
I can confirm your issue (tested on Pop3 acct, WinXP, TB24). Also, when opening files directly from attachment panel, they get listed in their own title-less Download manager window. The listing of files in "Saved Files" is much too volatile in general. And so on...

It's unlikely these will be fixed in the current setup.
We should really try to port the new download manager from FF (bug 907732) first, and then take it from there.

If you are or know of a volunteer coder willing/able to do this, most welcome :)

Comment 3

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #2)
> There's a number of things going wrong with "Saved Files" aka Download
> manager in TB currently.

Some more problems of current download manager are listed in Bug 571858 Comment 3.

Comment 4

[Magnus Melin [:mkmelin]]

(I don't think this ever worked.)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Call nsITransfer.init to create the attachment entry in the download manager

This patch fixes only save attachment(s) cases, not for DetachAttachmentsWOPrompts case because I have not investigated the detach case yet.  I do not know detached files should be also shown in the save files view though.

I am convinced that this patch also fixes bug 1067177 and moreover is also necessary for new download manager.

Comment 7

[Joshua Cranmer [:jcranmer]]

There is one thing I've noticed when I tested this patch. When you're downloading a small file, the downloads window pops up and then goes away when it's finished. Although I think this pre-exists already, it was especially annoying in a debug build because there's a noticeable gap of time where the window is basically waiting for a paint refresh.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8509326 [details] [diff] [review]
Call nsITransfer.init to create the attachment entry in the download manager

Josua, thanks for the checking. I can't reproduce the popup window on my local linux, so I thought the problem has been solved.

Anyway I will investigate it.

Comment 9

[Joshua Cranmer [:jcranmer]]

I will note that I was using a debug build without optimizations, which probably heavily impacts paint times, and I was also testing with 200-300KB-ish patches.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Oh, thanks for the info. I was also using debug build and testing 8.6kb attachment file! That was too small!

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: suppress_download_window.patch

This patch suppress the popup window. No popup download window shows up even if the attachment file size is not small. But I think it's OK because a progress bar shows at the bottom of main message window.

Pushed to try now to confirm there is no regression.
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=236bab5aa0b2

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8523355 [details] [diff] [review]
suppress_download_window.patch

Try server results seem fine.
There are a couple of new oranges but it is not caused by this patch.

For comparison, please see https://treeherder.mozilla.org/ui/#/jobs?repo=comm-central&revision=47be95d387af

Comment 13

[Thomas D. (:thomas8)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Created attachment 8523355 [details] [diff] [review]
> suppress_download_window.patch
> 
> This patch suppress the popup window. No popup download window shows up even
> if the attachment file size is not small. But I think it's OK because a
> progress bar shows at the bottom of main message window.

Hmmm. Main window status bar is very volatile and you never know what the progress bar is for, since several processes all use the same progress bar and hijack each other.

So I'm not sure if removing the popup download window with progress bar for large attachments is a good idea (but I'd have to recheck which exact window we're talking about...). Doesn't it make it much harder for user to understand what is going on while the big attachment is downloaded? It looks like an UI change which is not strictly required for this bug, so perhaps should be discussed and done in a separate bug?

Of course, if we had the new Download Manager as in current FF (Bug 907732), everything including progress indicators would be much better...

> Pushed to try now to confirm there is no regression.
> https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-
> central&revision=236bab5aa0b2

Comment 14

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8523355 [details] [diff] [review]
suppress_download_window.patch

Review of attachment 8523355 [details] [diff] [review]:
-----------------------------------------------------------------

Ultimately, this is a change that's not within my purview to review.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8509326 [details] [diff] [review]
Call nsITransfer.init to create the attachment entry in the download manager

Clearing review request.

Joshua, I am sorry for bothering you.
I should reconsider about UI/UX issue.

Comment 16

[Kent James (:rkent)]

Since we plan to land bug 1087233 for Thunderbird 38, we should also finish this to get the download manager working more reliably.

Comment 17

[Kent James (:rkent)]

Attachment: Unbitrotted

Comment 18

[Kent James (:rkent)]

Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

Review of attachment 8572901 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we should take this, otherwise Saved Files is for all practical purposes useless.

I could not reproduce the issue of a quick popup appearing, but the mail/ patch gets rid of that anyway IIUC.

Comment 19

[Kent James (:rkent)]

Comment on attachment 8523355 [details] [diff] [review]
suppress_download_window.patch

Magnus, should we do this pref change? It seems to be false in browser, only Thunderbird and Sea Monkey enable it. As Hiro says, we already have a progress indicator.

But I could not make this appear in any case, so maybe I don't really understand the issue.

Comment 20

[Kent James (:rkent)]

Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

I'd like to track this for landing in aurora. We'll decide later whether to do it or not.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking is different flags, adjusting.  If/when the patches are to be considered for uplift, they will need to be nominated again with the comment form filled out addressing questions of risk.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

unless you meant to flag these for approval-comm-aurora?

Comment 23

[Kent James (:rkent)]

Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

Sorry, I set the wrong flag.  I meant comm-aurora.

Comment 24

[Kent James (:rkent)]

Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

Pushed https://hg.mozilla.org/comm-central/rev/ece148c2ce41

Comment 25

[Kent James (:rkent)]

Sorry, this has nothing to do with Firefox.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 8523355 [details] [diff] [review]
suppress_download_window.patch

Review of attachment 8523355 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah let's take it. r=mkmelin

Comment 27

[Kent James (:rkent)]

Comment on attachment 8523355 [details] [diff] [review]
suppress_download_window.patch

Pushed https://hg.mozilla.org/comm-central/rev/05be474b042d

Comment 28

[Kent James (:rkent)]

https://hg.mozilla.org/releases/comm-aurora/rev/cbe3b7f342f5
https://hg.mozilla.org/releases/comm-aurora/rev/2c41ed5c7c94

Comment 29

[:aceman]

Comment on attachment 8509326 [details] [diff] [review]
Call nsITransfer.init to create the attachment entry in the download manager

This patch seems to be superseded. The commit message also seems weird as there is no nsITransfer.init in the patch. But maybe it is invoked somewhere behind the scene.

Comment 31

[WBT]

Possibly related: Bug 669615 
See also: http://forums.mozillazine.org/viewtopic.php?f=39&t=2226305 

Thanks for a fix!

Comment 32

[Andrew McGlashan]

I found that sometimes when I did a save, it didn't save anything; I had to check the save folder to see it wasn't there and then I did the save again.

I much preferred the separate window for <ctrl>-j ... I don't want that as a new tab, how can I restore the old display method for this?

Comment 33

[Magnus Melin [:mkmelin]]

There's no going back. That was using an old version of the download manager which is about to be removed.