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

RESOLVED FIXED in Thunderbird 39.0

Status

MailNews Core
Attachments
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: David O., Assigned: hiro)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
Thunderbird 39.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird38+ fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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...
(Reporter)

Comment 1

5 years ago
Might have similarities with bug 571858
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 :)
Blocks: 571858
Status: UNCONFIRMED → NEW
Depends on: 907732
Ever confirmed: true
See Also: → bug 536737
Summary: Attachment Save as... does not create a Saved Files entry → Attachments "Save as..." does not create an entry in "Saved Files" list aka Download Manager window
(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.
Component: Untriaged → Mail Window Front End
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

5 years ago
Keywords: regression
No longer blocks: 1067177
Keywords: regressionwindow-wanted

Comment 4

4 years ago
(I don't think this ever worked.)
Keywords: regression, regressionwindow-wanted

Updated

4 years ago
Duplicate of this bug: 1071641
(Assignee)

Comment 6

4 years ago
Created attachment 8509326 [details] [diff] [review]
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.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #8509326 - Flags: review?(irving)
(Assignee)

Updated

4 years ago
Component: Mail Window Front End → Attachments
Product: Thunderbird → MailNews Core
Version: 24 → Trunk
Attachment #8509326 - Flags: review?(irving) → review?(Pidgeot18)
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.
(Assignee)

Comment 8

4 years ago
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.
Attachment #8509326 - Flags: review?(Pidgeot18)
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.
(Assignee)

Comment 10

4 years ago
Oh, thanks for the info. I was also using debug build and testing 8.6kb attachment file! That was too small!
(Assignee)

Comment 11

4 years ago
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.

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

Updated

4 years ago
Attachment #8509326 - Flags: review?(Pidgeot18)
(Assignee)

Comment 12

4 years ago
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
Attachment #8523355 - Flags: review?(Pidgeot18)
(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 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.
Attachment #8523355 - Flags: review?(Pidgeot18)
(Assignee)

Comment 15

4 years ago
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.
Attachment #8509326 - Flags: review?(Pidgeot18)

Comment 16

3 years ago
Since we plan to land bug 1087233 for Thunderbird 38, we should also finish this to get the download manager working more reliably.
tracking-thunderbird38: --- → +

Comment 18

3 years ago
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.
Attachment #8572901 - Flags: review+

Comment 19

3 years ago
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.
Attachment #8523355 - Flags: review?(mkmelin+mozilla)
Attachment #8523355 - Flags: approval-mozilla-aurora?

Comment 20

3 years ago
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.
Attachment #8572901 - Flags: approval-mozilla-aurora?
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.
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox38: --- → +
Attachment #8523355 - Flags: approval-mozilla-aurora?
Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

unless you meant to flag these for approval-comm-aurora?
Attachment #8572901 - Flags: approval-mozilla-aurora?

Comment 23

3 years ago
Comment on attachment 8572901 [details] [diff] [review]
Unbitrotted

Sorry, I set the wrong flag.  I meant comm-aurora.
Attachment #8572901 - Flags: approval-comm-aurora?

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0

Comment 25

3 years ago
Sorry, this has nothing to do with Firefox.
status-firefox38: affected → ---
status-firefox39: affected → ---
tracking-firefox38: + → ---

Comment 26

3 years ago
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
Attachment #8523355 - Flags: review?(mkmelin+mozilla) → review+

Comment 27

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

Pushed https://hg.mozilla.org/comm-central/rev/05be474b042d
Attachment #8523355 - Flags: approval-comm-aurora?

Updated

3 years ago
Attachment #8523355 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

3 years ago
Attachment #8572901 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

3 years ago
status-thunderbird38: --- → fixed

Comment 29

3 years ago
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.
Attachment #8509326 - Attachment is obsolete: true

Updated

3 years ago
Duplicate of this bug: 1067177

Comment 31

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

Thanks for a fix!

Comment 32

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

3 years ago
There's no going back. That was using an old version of the download manager which is about to be removed.
You need to log in before you can comment on or make changes to this bug.