User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a3pre) Gecko/20100303 Thunderbird/3.2a1pre In TB3.x, there are two code paths for saving an attachment to a message. On linux these are the following code paths 1st: Double clicking with left button on an attachment. A pop up appears to store the file, etc. This will be eventually handled in nsHelperAppDlg.js which is in bin/components (and comes from src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js ) 2nd: Right clicking on an attachment. A pop up menu appears and the user select save as. This seems to be handled in a different C++ file src/mailnews/base/src/nsMessenger.cpp A set of similar sounding functions can be found there. The first path mentioned above seems to be used by BROWSER (FireFox) as well. I found the existence of the two different paths while trying to fix Bug 429827. As I made my way through the bug fix, I only realized that I was fixing only the 1st path alone. The second path was not touched at all. The existence of two code paths that perform similar or essentially the same function pose the following problems. (a) maintenance headache. We need to keep both of them in good health. (b) similar but slightly different function confuses users. I just found out yesterday that the default directory doesn't seemed to be shared by the two code paths. Hmm... Also, I noticed (but not verified rigorously yet) that umask is handled differently in the two paths. This may lead to a security problem in a shared file environment! So I am posting this bugzilla entry to alert the developers of this problem and solicit a solution. My guess is that we should merge the two paths into one. However, the complication is that if we are talking only of TB, then choosing nsMessage.js is one way to go. But since the first path is actually used by FF also, this complicates the problem. Does anyone in the know, who knows why there are two paths, knows the early history of development of TB, etc. have a suggestion to fix this maintenance nightmare? I am close to the solution to the first path fix for Bug 429827 [for the last several days like 10 days, I could not compile the binary due to {mozilla,comm}-central reshuffle, but today I could compile it finally. But the second path fix is not tried yet. Any thought? Reproducible: Always I am setting the severity as normal, but someone may want to change this to enhancement. I am not sure exactly how the proposal for major code change like the merging of two paths to ease maintenance/fix existing bug(s) should be handled.

Moving to Mailnews Core since it involves shared code.

Phil, bienvenu, Standard8, do you guys have any thoughts on this? I think a lot depends on a) exactly how much work it would be to get rid of the nsMessenger code b) what sort of constraints depending on the helper app code would introduce. Last time I looked at that code, it was fragile and difficult to evolve without breaking. Is that still the case? Does that code have an owner these days, or is it still mostly drive-by fixes by people who need it?

It's possible there could be more code sharing, but I don't think the code paths could be merged completely (or at least, I have a vague recollection that there's a reason the code starts at different points). I don't see how you can get rid of the neMessenger code. It's used for things like detach/deleting attachments, and save as.

we need to go with the first option if this bug is patched. We now have a download manager and the second saveas method does not use it. I haven't tried the second method on large files but the download manager gives a nice non modal progress bar for long downloads. I have a problem with not seeing progress on drag and drop progress, bug 242204.

oops forget that. We seem to save to our download manager in all cases here including using the rt click context, double click and the menu save attachment. and.. the correct reference is Bug 501054 for dnd

(In reply to comment #5) > oops > > forget that. > > We seem to save to our download manager in all cases here including using the > rt click context, double click and the menu save attachment. > > and.. the correct reference is Bug 501054 for dnd Dear Phil, Are you sure of our "download manager in all cases here"? I think you mean the saving via URLs both in TB and Firefox here (?). My original post was about ThunderBird (Mailer)'s behavior when we try to "save attachment", i.e., saving the [MIME] attached data (doc/spreadsheet/picture or whatever) within an e-mail message. Re progress bar: It is nice to see that for browser download (of data from a remote server). However, for TB (mailer) attachment saving, it is a little strange to see the progress bar when the message that includes the attachment is ALREADY in the local folder. (Yes, for a large attachment converting MIME data to original takes a little time, and in that sense, progress bar may make sense.) But often times, the saving the attachment is almost instantaneous, and the download progress bar that shows up for a fraction of second confuse people. Well, at least I am. See some related bugs in TB 2 (two) series concerning the use of download manager for "Saving the Attachment": Bug 472932 - Thunderbird's download window is blank during download (At least, in TB3 series, blank download manager window doesn't stay open for TB3.) Bug 442068 - Downloads dialog is not evoked when saving attachments if message size <64k (This seems to be the case of right-clicking. Quote from that first comment :3. Right-click the attachment in the preview pane and choose "Save as". I chimed in with my observation, but I was not sure if there was this subtle difference of code paths between left-clicking and right-clicking. ) Yes, it is very confusing that the browser and mailer shares the same code nsHelperAppDlg.js and in the current TB, it is ALSO used for "Saving As" attachment that is invoked with left-clicking while a different code, nsMessanger.cpp, is used for saving the attachment with right clicking. I am afraid that many bug entries related to similar bugs contain many posts from people who have not realized that "saving the attachment" uses code from nsHelperAppDlg.js in one case, and uses nsMessanger.cpp in another case. I agree that eliminating nsMessager.cpp completely may not be possible due to the reasons mentioned Comment 3. But I think at least functions for saving attachments in nsMessanger.cpp may be eliminated and instead the nsHelperAppDlg.js may be used for saving the attachment just for the simple reason of reducing maintenance work? OTOH, there may be an unfamiliar data flow path which I have been unaware of since Bug 501054 mentions "since it may take awhile like attachments to rss feeds.". So maybe the saving of attachment from a message goes to the outside (??) instead of to a local file. If so, the simple nsHelperAppDlg.js may not be able to handle such data flow and `saving attachmetn' code in nsMessanger.cpp may still be necessary. Hmm.

I am throwing an observation here why I don't think actually sharing the code for "downloading from URL" and "Saving an Attachment" is not such a great idea, or that the code should have a flag to change behavior. I am not sure if this is relevant to the discussion of code merging, but since GUI related topics ought to be considered, I am throwing this in. I don't know about others, but when I save an attachment from an e-mail, I usually have a very clear idea of where the attachment should go (i.e., the directory) based on the content of the e-mail and from whom it was sent to me, and to a lesser extent under what name (I can often live with the original file name given by the sender, but I often want to add the sender's initial or something to the front of the name. Also, for sharing the file with people in many countries later, I often need to rename the local Japanese character name to US ASCII only names, but I digress). On the other hand, I usually don't care much about such naming during downloading from URL using a browser. I am usually happy with a default download location, and dump everything there. (This seem to be also a major operation mode for many users considering that there is this default download location setting in both nsHelperAppDlg.js and nsMessanger.cpp although in the latter case, I have a feeling it is meant to ease the saving of subsequent attachments from the save e-mail message to a newly chosen directory.) Sometimes, I even forgot that I have downloaded the same file before, and only realize this when TB attached "(2)" or something like that to SILENTLY modify the download filename without warning me at all in advance. This silent renaming may seem a good idea to some, but I have a doubt about it. BTW, this gratuitous renaming of the file name is one thing that makes nsHelperAppDlg.js unnecessary complex and hard to read, and if the users are persuaded, probably should be eliminated from nsHelperAppDlg.js IMHO. (To be honest, I am not sure what the code does exactly in there regarding this renaming. That there seems to be system-dependency #if,#endif is beyond me :-( I often move the downloaded files (very often drivers, public spec specification and such) to desirable storage folders after the fact. I know some people DO create a separate folder in advance and download the files there. The above is a simple observation of a user behavior. The code duplication in two files should be reduced in any case, but we may want to add a switch if necessary to accommodate the above and similar user interaction requests. IMHO, the gratuitous renaming of download files can be handled OUTSIDE the nsHelperAppDlg.js. Sorry for the interruption.

seems it saves through the download manager when I was in local folders. Now, I'm not sure what state my build is in so I can't say there is a problem with IMAP but I can't seem to save an attachment my rt click on attachment and saveas nor can I use the menu. Only left double click and saveas seems to work with imap.

again we need QA to try these, I discovered I'm using empty attachments to test, because most of my build folders are test email and aren't dependable. But at my latest check the menu saveas doesn't work open does, rt click saveas doesn't work rt click open does, double click saveas works and open works. By works I mean showing the files in the saved files window Ctrl-J.

It's possible that IMAP may cause a slightly different behavior, but empty attachment (?) might be the cause of your symptomn of the right-clicking not working. Anyway, thank you for the feedback. The more people know this problem, the better. The solution will come once a consensus emerges, I hope.

(In reply to comment #9) > again we need QA to try these, I discovered I'm using empty attachments to > test, because most of my build folders are test email and aren't dependable. > > But at my latest check the menu saveas doesn't work open does, > rt click saveas doesn't work rt click open does, > double click saveas works and open works. Do we need try builds ? What exactly needs to be tested ?

(In reply to comment #11) > Do we need try builds ? What exactly needs to be tested ? If we could a confirmation of following, then if we plan on merging the codes (patching this bug) then at least we know which code is the keeper. with trunk build confirm the files are placed in our 'Saved Files' list (ctrl-J) with following actions on an email with attachments: 1. menu-File > Attachments > [name] > open 2. menu-File > Attachments > [name] > save 3. message pane double click attachment > save file 3. message pane rt click > open > save file 4. message pane rt click > save as

I am not sure if this question should be asked here, but here it goes. As I tried to make nsMessanger.cpp handle user interaction better by differentiating explicit user-cancellation and I/O error (such as non-existent directory or write-protected directory) when user attempts to save an attachment (which seems to be invoked by right-clicking), my approach was to propagate the user-cancellation information explicitly by returning a flag as [out] parameter to low-level functions. Of course, the information needs to seep through function call hierarchy to upper level functions where an intelligent use of such information can be made. But, first the information needs to passed. Thus, I began modifying function prototypes to accommodate this piece of information. Whenever, a prototype mismatch occurred with nsMessenger.h, I modified the prototype declarations to match the change after checking the usage using http://mxr.mozilla.org/ . But then finally, I hit upon a prototype that seems to come from an IDL file. That is the prototype change requires a change to IDL(!). I checked the usage of the function using http://mxr.mozilla.org/ and it doesn't seem to be used outside the nsMessanger.cpp (!). So it is possible to change this and not break TB3, I think. The function prototype in question is for SaveAttachment. I want to add a pointer to PRBool so that the user-cancellation information can be passed back as PR_TRUE (user cancelled) or PR_FALSE (user didn't cancel. The error code is due to some genuine error.) To wit: /home/ishikawa/TB-3HG/src/mailnews/base/src/nsMessenger.cpp:893: error: prototype for 'nsresult nsMessenger::SaveAttachment(const nsACString_internal&, const nsACString_internal&, const nsACString_internal&, const nsACString_internal&, PRBool, PRBool*)' does not match any in class 'nsMessenger' /home/ishikawa/TB-3HG/src/mailnews/base/src/nsMessenger.cpp:738: error: candidates are: nsresult nsMessenger::SaveAttachment(nsIFile*, const nsACString_internal&, const nsACString_internal&, const nsACString_internal&, void*, nsIUrlListener*, PRBool*) /home/ishikawa/TB-3HG/src/mailnews/base/src/nsMessenger.h:62: error: virtual nsresult nsMessenger::SaveAttachment(const nsACString_internal&, const nsACString_internal&, const nsACString_internal&, const nsACString_internal&, PRBool) Line numbers are from my locally modified files. I am not that familiar with XPCOM, but the virtual function prototype mentioned at nsMessenger.h:62 must be from IDL definition from XPCOM magic (preprocessing and such). What is the proper procedure to propose an IDL definition and get it accepted? Not that the propagated user-cancellation information is used for better UI handling yet. I found that the code in msMessenger.cpp requires more massaging to offer better UI handling as offered by nsHelperAppDlg.js now. (For example, nsMessenger.cpp doesn't offer the detailed I/O error information from my cursor reading. So an error due to missing directory, and an error due to write-protected directory seem to be handled as generic I/O error.) So the suggested change to IDL is one small step. The fix as a whole is a long way to come to nsMessenger.cpp :-( It may be that I misunderstood the XPCOM interaction pretty much, but I will be glad if someone can shed some light on what is going on here.

(In reply to comment #0) > Also, I noticed (but not verified rigorously yet) that umask is > handled differently in the two paths. Related to bug 533976?

(In reply to comment #14) > (In reply to comment #0) > > Also, I noticed (but not verified rigorously yet) that umask is > > handled differently in the two paths. > > Related to bug 533976? More than likely. On superficial survey, both nsHelperAppDlg.js and nsMessenger.cpp uses 0600 mode mask to lower-level file open (for creation) routines. I have not yet followed the lower-level routines to figure out where umask kicks in (or not in the worst case scenario.) I even vaguely recall seeing 0666 or 0660 mask which may be a little loose, but can't figure out where. All right, searching for (.*066.*) using mxr.mozilla.org turned up quite a few places where such masks are used. http://mxr.mozilla.org/comm-central/search?string=\%28.*066.*\%29&regexp=1&find=\.c&findi=\.c&filter=^[^\0]*%24&hitlimit=&tree=comm-central Auditing these may take a while. I found only ONE place where ~mask was mentioned. /mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp (View Hg log or Hg annotations) * line 643 -- destFile->SetPermissions(0666 & ~(mask)); Maybe, nowadays majority of developers are not aware of "umask" existence (too much exposure to Windows?) or that the lower-level routines invoked by these upper calls take care of umask under the table (and may fail in some code paths obviously according some reports). Just my two cents. I will try to finish the clean up of nsHelperAppDlg.js for now.

While I am trying to clean up and fix the problem mentioned in bug 429827 recently with the code for TB 3.2a1, I noticed that the 2nco code path mentioned above, namely the path invoked by RIGHT-clicking on an attachment to an e-mail message when one tries to save it to a write-protected file produced a warning dialog box TWICE in succession. That is, TB warns that the saving fails, but if I close it, there is another same dialog box which I need to close before I can do anything. Just an observation. I will dig into where the dialog box is shown (TWICE). TIA

According Comment 4 of Bug 56758, https://bugzilla.mozilla.org/show_bug.cgi?id=567585#c4 the default save directory for the case of browser and messenger (mailer) are as follows. --- begin quote Save location choice is affected by next settings. Tools/Options/Attachments [ ] Save files to xxx browser.download.useDownloadDir=true [ ] Always ask me where to save file browser.download.useDownloadDir=false And, directory choice is affected by next settings. > browser.download.folderList > browser.download.dir > browser.download.downloadDir > messenger.save.dir <= last used directory by Tb for save mail or attachmet > mail.compose.attach.dir <= last used directory for attachment choice Used prefs.js entries by Tb is slightly different from browser. See next documents, for related settings. > http://mdn.beonex.com/en/Download_Manager_preferences > http://kb.mozillazine.org/Unable_to_save_or_download_files --- end quote Oh, wait, I think there is a bug due to the split code paths. The saving of attachment by left clicking seems to use the "BROWSER" default even from TB3 (!) while saving attachment from by right-clicking (in different code path) seems to use the "Messanger" default directory. Oh well. Left-clicking: nsHelperAppDlg.js // Retrieve the user's default download directory let dnldMgr = Components.classes["@mozilla.org/download-manager;1"] .getService(Components.interfaces.nsIDownloadManager); let defaultFolder = dnldMgr.userDownloadsDirectory; ===== Right-clicking path mailnews/base/src/nsMessenger.cpp nsresult nsMessenger::GetLastSaveDirectory(nsILocalFile **aLastSaveDir) { nsresult rv; nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv); NS_ENSURE_SUCCESS(rv,rv); // this can fail, and it will, on the first time we call it, as there is no default for this pref. nsCOMPtr <nsILocalFile> localFile; rv = prefBranch->GetComplexValue(MESSENGER_SAVE_DIR_PREF_NAME, NS_GET_IID(nsILocalFile), getter_AddRefs(localFile)); if (NS_SUCCEEDED(rv)) { NS_IF_ADDREF(*aLastSaveDir = localFile); } return rv; } Note that MESSENGER_SAVE_DIR_PREF_NAME is #define MESSENGER_SAVE_DIR_PREF_NAME "messenger.save.dir" So as far as I can tell, the use of download manager invalidates a somewhat older behavior of default directory choice. The existence of two paths poses a few problems and this is only one such problem. The different default directory positions are very easy to notice. Just try saving an attachment with left-clicking and right-clicking after the startup of TB3. You will find that different directories are shown (assuming that you have different directories for both the browser default and messenger default.)

Most of this discussion is coming from developers concerned about maintaining code. This comment is coming from a user: I'm voting for this bug because it drives me crazy to have two completely different dialog boxes appear, depending on whether I choose Save As or Save All. Save As brings up a normal (contemporary) Windows dialog box, with the option to View as List, Details, Icons, etc., and with shortcuts to the Desktop, My Computer, etc. in the lefthand pane. Save All brings up an ancient tree-list structure in which I have to find and click a million of those damned tiny plus-signs, while losing sight of which parent folder I'm in, and often having to scroll horizontally while I'm at it. This type of dialog violates just about every established principle of UI design there is, and it's annoying as hell. Sure would be nice if you could consolidate the UI to a single "modern" dialog style at the same time you resolve your maintenance headache. TB 3.0.4 on WinXP. (And thanks for all your work on the product!)

I think the 2 code paths are inherited from Firefox as it also distinguishes between click and rightclick+save target. As you said, the first one may open a dialog for choosing what to do with the attachment the other one may save directly. I think it would be hard to merge them (and maybe even unwanted, as some users may want the dialog to directly open attachments without saving). I think Chiaki knows better about this then 2 years ago as he is fixing bug 567585. If in need, we can call in Gavin Sharp form that bug or maybe someone from the Download manager (hey, mconley :)).

Firefox's plan is to consolidate our various download code paths into bug 825588.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20) > Firefox's plan is to consolidate our various download code paths into bug > 825588. Maybe we can use the same unifying code for thunderbird, too. I will wait and see. The bug 567585 needs to be fixed in the meantime anyway.

FF has a new download manager and the UX is much superiour over the old one which TB is still using. The new UI solves a whole range of UX problems around the current download manager in TB, which is mostly not unavailable or even empty when it's needed. Furthermore, the old nsIDownloadManager is on its way out of toolkit (Bug 851471), which adds another strong argument for TB to make the switch (Bug 907732), which will also affect this bug. For the original intention of this bug, I can generally confirm that the UX around saving/opening attachments, due to the historical legacy from FF involving the two code paths, is currently very complex to the point of being entirely useless and broken/incomplete, especially for users with browser.download.useDownloadDir=true (try using a default directory for saving all your TB attachments...). I'm not in the mood for details, but it may suffice to say by way of example that: * depending on your setup ("ask me"), you an get "Open or Save?" question just after clicking "open" from context menu * even after setting a one-for-all download folder (useDownloadDir=true) there's no way you could use "Save all" button or any other "save" menu option to actually save all directly to that folder, it will stubbornly insist on asking you for a folder. I'm not saying it's easy to get the UX and UI right, but currently it's really weird.

(In reply to Thomas D. from comment #22) > FF has a new download manager and the UX is much superiour over the old one > which TB is still using. The new UI solves a whole range of UX problems > around the current download manager in TB, which is mostly not unavailable > or even empty when it's needed. a negation too much is a negation too much, this should read: ...current download manager is *mostly unavailable* or even empty when it's needed...

On the few occasions I have used TB save dialogs and download manager, the majority of times have been very DISAPPOINTING. So I am much in favor of Thomas' suggested direction. And I'll bet doing this will solve several other bug reports.

(In reply to Wayne Mery (:wsmwk) from comment #24) > On the few occasions I have used TB save dialogs and download manager, the > majority of times have been very DISAPPOINTING. So I am much in favor of > Thomas' suggested direction. And I'll bet doing this will solve several > other bug reports. So please get me straight. I think nsHelperAppDlg.js (used by TB in comm-central) today uses *asynchronous* API (it uses task, yield, and now the name of the function has been changed to include "Async" in it). Is the discussion in there is an upper-layer that uses "the new js downloads manager with new UI" in FF? (The one that shows the Moving Green Arrow until the download finishes(?)) Well, I thought nsHelperAppDlg.js (used by TB in comm-central) refers to the (new) *.jsm file already, but maybe I was wrong. http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js#100 101 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); 102 Components.utils.import("resource://gre/modules/DownloadLastDir.jsm", downloadModule); 103 Components.utils.import("resource://gre/modules/DownloadPaths.jsm"); 104 Components.utils.import("resource://gre/modules/DownloadUtils.jsm"); 105 Components.utils.import("resource://gre/modules/Downloads.jsm"); <=== This? 106 Components.utils.import("resource://gre/modules/FileUtils.jsm"); 107 Components.utils.import("resource://gre/modules/Task.jsm"); In any case, if FF's upper-level download manager can handle TB's saving of attachment (already in a local mail article) in a user-understandable manner without using browser's terminology, I would be a happy user. Oh, I see. Probably the discussion is about the changing nsMessenger.cpp to use the above JSM files and the download manager? (That would be a work, indeed.) TIA

(In reply to ISHIKAWA, Chiaki from comment #25) > (In reply to Wayne Mery (:wsmwk) from comment #24) > The　next is incoherent sentence.: > Is the discussion in there is an upper-layer that uses > "the new js downloads manager with new UI" in FF? (The one that shows the > Moving Green Arrow until the download finishes(?)) Is the discussion here progressing with the assumption that there is an upper-layer (on top of this ASYNC JS API) "the new js downloads mangaer with new UI" in FF. And I presume that is responsible for showing the moving green arrow when we download something from the web URL in FF. > Oh, I see. Probably the discussion is about the changing nsMessenger.cpp to > use the above JSM files and the > download manager? (That would be a work, indeed.) The last part in "()" ought to have read "That would be a work since the code needs to support additionally the detach/attach of the e-mail attachments aside from the saving of attachment (and also the generic saving from web URL in TB? If so, we seem to be going back to the old suite where browser/mailer/etc. was in one package.) TIA

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

I am reminded that this issue of two different save paths (remembered as default) still exists today. I usually save attachments by double-clicking and so I hit only one of them, but today I somehow used "Save" button that is shown at the lower-right corner and I was surprised to see an unexpected default folder which was different from the directory chosen for the downloading by "double-clicking". I vote for the unification because of the code duplication and maintenance burden However, if not enough man-power exists (as is now) until the planned major rewrite, probably this can be ignored since it looks I am the only one so far who has made issues out of this misfeature. TIA