Setting the preference to Forward as attachment, if you double-click that attachment it displays the message in raw text instead of opening a new message window. STEPS: 1. Set preference to Forward messages as an attachment. 2. Forward a message to yourself. 3. Dbl-click attachment. RESULT: Opens a new browser window instead of a new message window.

Nominating for RTM.

reassigning to ducarroz.

Can we fix this please, pretty please? Adding 4xp to keywords. It's more serious when sendng forwarded signed messages and you would like to get at the certs in the attached mail messages. Currently you have to save to disk as an eml file, and then open in the browser to view it correctly. Opening it directly from the mail message display the raw text form.

Also, Opening the attached message from the desktop after it has been saved does display it correctly. However, in the case of signed/encrypted eml files, the s/mime functionality does not kick in, which is very bad. Attempting to view source of the message displays incorrect behavior in that on my win2k system I am prompted to open a helper app to view rfc822-data instead of displaying the source the same way the mail client displays it...

This is not a simple fix. The main problem is that the stand alone message display window use message key and URI while in order to be able to display any kind of messages (file, attachment), we need to expand it to support URL which imply to modify also message DB. cc'ing mscott to get his input on the matter...

I think this bug is a duplicate of bug 120453.

*** Bug 120453 has been marked as a duplicate of this bug. ***

They are duplicates. I would imagine opening the file from disk in the message window behaves the same way as double clicking a message in the summary pane - S/MIME functionality would kick in if needed.

*** Bug 149719 has been marked as a duplicate of this bug. ***

There is 2 main issues we need to resolve: 1. in order to be able to handle .eml file in mailnews, we need to register a content/handler for message/rfc822. By doing that, we get a major conflict with the mail3Pane window which cause messages to not be displayed anymore. 2. mime is not able to convert to html a subpart of a message as a stand alone message. I have hard time fixing that

This patch allows mime to nicely display an embedded message/rfc822 part in a browser window, no more raw data...

*** Bug 161306 has been marked as a duplicate of this bug. ***

If the attached message is displayed in a browser window, will the mail-specific options be available (i.e. reply, forward, etc.)? If not, is there an alternative way of fixing this which will make the message open in a real message window rather than a browser?

Is there any chance to have a fix (open the attached mail in a mail windows) for this bug included in the next release ? (Moz 1.2) ?

I'm quite surprised that so many seem to be so indifferent about this bug. As far as serious email handling, this one's a killer. For my business email traffic, I'm still using Outlook Express, but as soon as this bug is fixed, I'll quickly move everything over to Mozilla. The action that I can perform with Outlook that I can't with Mozilla is to "unbundle" multiple forwarded emails. For example, when I want to transfer a large volume of email from one account to another, I block all of the individual emails and forward them to the other address. At the other end (with Outlook Express) I unbundle them and store them like regular emails. I've never been able to do this with Netscape or Mozilla, and it's a deal breaker!

Nominate this bug for Buffy.

*** Bug 83915 has been marked as a duplicate of this bug. ***

I forget to include those 2 new file in the previous patch! they are needed in order to build it.

There still have several issues remaining. On top of my head: 1) The message headers are displayed in HTML as part of the message body instead beeing displayed in the chrome 2) Attachment list is missing (need to go in the chrome) 3) Moste of the toolbar function in the message display window must be disable 4) the print doesn't work 5) Opening an *.eml file does not open a mail display window

There is a nsbeta1+ in the Keywords, so will this be fixed in Mozilla 1.2(which is scheduled to be released on Nov, 08 according to the roadmap) ? Jean-Francois, do you have more progress on this bug?

I suggest to raise the severity level for this bug to "major" because this not a minor loss of function, or other problem where easy workaround is present. The priority could be also set to something higer. It seems that this bug is hard to resolve and it prevents the use of Mozilla for encrypted/signed mail and for massive mail handling. May be raising the severity and the priority level could make it more "visible" and then give some help (resources) to Jean-François.

Raise the Priority and Severity tentatively. I agree with Olivier. Wish this bug can be fixed soon.

This used to work but it broke in one of the nightlies. An email that was forwarded as an attachment used to open a mail window but now it opens a browser window. It seems like developers have a good handle on the problem so I'll just add myself to the CC: list.

Reassign this bug to me.

Reassigning.

Hi Calvin, I will add a propose patch ASAP, so I have to reassign this bug to me. I think I will complete my patch Tuesday.

Sorry, I means Cavin.

Is this supposed to be working? I'm still seeing the bug in fairly recent builds. Mozilla 1.3b Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030204

My patch was base on Jean-Francois Ducarroz's patch. It can solve issue 1-4,please see comment #20,but the issue 5 was hard to be solved due to current architecture of mailnews. So I will file a new bug for it. I think my patch still need complete, please give me your advice. Thanks!

I wasn't able to apply the patch due to merge conflicts in messageWindow.js and msgHdrViewOverlay.js. I was able to resolve them except the last one in msgHdrViewOverlay.js. Also, your patch doesn't include the 2 news files nsMessengerContentHandler.cpp/h. which are in the previous attachment. Can you review them? I have found couple problems (despite I wasn't able to test the patch): 1) Looks like your are using TABS instead of spaces. Please replace TABs by 2 spaces. I have seen that in messageWindow.js 2) I found an occurance of messenger.OpenURL in addressbook.js which doesn't have the new boolean argument. Please verify all occurance in js and cpp files in Mozilla (use http://lxr.mozilla.org/seamonkey) 3) the function mime_url_cut_part scare me a little bit! What is the real meaning of this function, can you explain it? My first concern is about the fact the paremeter "part=" is not necessary the only query parameter and also not necessary the first one! you could potentially have <url>?type=something&part=2.1&.... Are you trying to remove the whole query section or only the part parameter? My second concern is why not use strstr instead of using strncasecmp in a loop?

sorry for my old code, I will add a new patch for trunk. so please don't worry about merge, I will give you a fresh patch.:-) I didn' change nsMessengerContentHandler.cpp/h, so I didn't add those files into my patch. but I will add those files into my new patch. For issue 1, Yes, I use a lot of "TAB",I found a lot of original code ues "TAB", so I just follow the original code stlye. but anyway I will follow your advice, change them to 2 spaces. For issue 2, I will change the code of addressbook.js. For issue 3, I must cut the "part" section due to it will make the wrong url for sub attachment. for example, when we display a attachment as a email. the base url may be imap://......?part=1.2, but the part url is base on the email including the attachment for display instead of the attachment for display. then if we didn't cut the base url's "part" section, it would make the wrong url for sub attachment of the attachment for display. so I cut the base url. but I think your advice was more useful for me, I think I should try to cut tha part of url from "?type". I will add a new patch according your advice. thanks

I have changed some codes according to ducarroz's advice. This patch was created by diff with trunk. So I think it can works fine on trunk. I have merged the attachment 102193 [details] [diff] [review] into my new patch. so please review my new patch. Thanks!

I have tried this patch out of curiousity. A technical problem with the patch is that files nsMessengerContentHandler.h/.cpp are created in mozilla/ instead of mozilla/base/src, after applying the patch and moving the files it compiles. However, the patch doesn't seem to produce the desired effect. When double clicking on a message attachment, I expected a new message window to open, but instead I see a file download dialog.

I can create a new patch for nsMessengerContentHandler.h/.cpp,but my patch could make mozilla launch a message windows when you click a message attachment. Which version of mozilla did you use? I have tested my patch on build(2003-02-18), I'm sure mozilla will launch a message windows when I click a message attachment.

The tree I tried it with was from yesterday. However, I tried on Linux, maybe that makes a difference.

I have changed some code to solve the problem,which is files nsMessengerContentHandler.h/.cpp are created in mozilla/ instead of mozilla/base/src. Furthermore, I have tested my patch with today's trunk on linux and windows, it is works fine. Could you try again ?

Comment on attachment 115113 [details] [diff] [review] patch version 1.2 good job, this is working well. You did it man! R=ducarroz

curious, what will this url do: news://news.mozilla.org:119/3E4BAB2A.8080906@netscape.com will you get that news message in a message window, or a browser window?

Seth, I just tried, you get a browser window.

Ok, I can confirm the patch works for me, too. In my earlier test, I had not deleted components.reg/xpti.dat

There is one issue left, but we can do that in a separate bug: When opening an attached message in a new window, where the attached message was signed, the UI does not display the signature icon, nor does the "view message security info" show the details of this message, but of the toplevel message. In order to solve this problem, we will have to extend mime/src/mimecms.cpp and mimemcms.cpp to be aware of the nesting level of the shown message.

this is a non-trivial patch, and will take some time to review. I hope to have this reviewed during 1.4 alpha

Actually, it was quite simple to implement what I requested in the previous comment. Additional patch attached.

Antonio, doing some testing, I found one case where the message in the new window is not shown correctly. I have sent you such a message by private mail. To describe in more detail: - create a new HTML message - use insert/image to add an inline image to your html message - send the message to yourself - receive the message - forward the message as attachment to yourself - receive the forwarded message When you view the second in the main mail view, the image is displayed correctly. Now click the attached message and open it in a new window. Problem: A broken image is shown.

Comment on attachment 115841 [details] [diff] [review] Additional patch for crypto status needs more work

Comment on attachment 115841 [details] [diff] [review] Additional patch for crypto status needs more work

I will do some testing tomorrow.

I will take a look at the issue about comment 43.

I have filed bug 195378 to track the S/Mime status feedback separately.

I will add some code for solve the problem of comment #45.

Add some code for solving the problem being mentioned in comment #45. Please test and review my patch. Thanks.

I only changed some code in mimemrel.cpp.

Comment on attachment 115858 [details] [diff] [review] patch version 1.3 I confirm that Antonio only added one more change in file mimemrel.cpp, so I'm giving r=kaie on that new change, since it looks reasonable to me, and I have tested that it works. Carrying forward r=ducarroz on the other unchanged portions of the patch.

Comment on attachment 115858 [details] [diff] [review] patch version 1.3 Just gave this a quick try out. Opening a message attachment opened a window, but there was a JS exception (mailWindowOverlay.js tried to convert the messageURI to a message header). Drag-n-drop didn't work, but then it failed before :-/ >Index: mailnews/base/public/nsIMsgWindow.idl >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/public/nsIMsgWindow.idl,v >retrieving revision 1.22 >diff -u -r1.22 nsIMsgWindow.idl >--- mailnews/base/public/nsIMsgWindow.idl 29 Nov 2001 04:56:23 -0000 1.22 >+++ mailnews/base/public/nsIMsgWindow.idl 9 Feb 2003 06:29:11 -0000 >@@ -45,6 +45,7 @@ > interface nsIMsgHeaderSink; > interface nsIPrompt; > interface nsIAuthPrompt; >+interface nsIURI; Does this do anything? >Index: mailnews/base/src/nsMessenger.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/src/nsMessenger.cpp,v >retrieving revision 1.257 >diff -u -r1.257 nsMessenger.cpp >--- mailnews/base/src/nsMessenger.cpp 8 Jan 2003 21:40:11 -0000 1.257 >+++ mailnews/base/src/nsMessenger.cpp 9 Feb 2003 06:31:01 -0000 >@@ -543,7 +545,7 @@ > } > > NS_IMETHODIMP >-nsMessenger::OpenURL(const char * url) >+nsMessenger::OpenURL(const char * url, PRBool useMessageService) > { > if (url) > { >@@ -561,18 +563,42 @@ > // for the web shell, but the message service doesn't need it unescaped. > nsUnescape(unescapedUrl); > >+ nsresult rv = NS_OK; > nsCOMPtr <nsIMsgMessageService> messageService; >- nsresult rv = GetMessageServiceFromURI(url, getter_AddRefs(messageService)); >+ if (useMessageService) >+ rv = GetMessageServiceFromURI(url, getter_AddRefs(messageService)); > >- if (NS_SUCCEEDED(rv) && messageService) >+ if (useMessageService && NS_SUCCEEDED(rv) && messageService) > { > nsCOMPtr<nsIWebShell> webShell(do_QueryInterface(mDocShell)); > messageService->DisplayMessage(url, webShell, mMsgWindow, nsnull, nsnull, nsnull); > mLastDisplayURI = url; // remember the last uri we displayed.... > } > //If it's not something we know about, then just load the url. >- else >+ else if (!useMessageService) > { >+ nsCOMPtr<nsIURI> uri; >+ nsAutoString uriString(NS_ConvertASCIItoUCS2(unescapedUrl).get()); >+ // Cleanup the empty spaces that might be on each end. >+ uriString.Trim(" "); >+ // Eliminate embedded newlines, which single-line text fields now allow: >+ uriString.StripChars("\r\n"); >+ NS_ENSURE_TRUE(!uriString.IsEmpty(), NS_ERROR_FAILURE); >+ >+ rv = NS_NewURI(getter_AddRefs(uri), uriString); >+ if (NS_FAILED(rv) || !uri) >+ return NS_ERROR_FAILURE; >+ nsCOMPtr<nsIMsgMailNewsUrl> msgurl = do_QueryInterface(uri); >+ if (msgurl) >+ msgurl->SetMsgWindow(mMsgWindow); >+ if (mDocShell) { >+ nsCOMPtr<nsIDocShellLoadInfo> loadInfo; >+ rv = mDocShell->CreateLoadInfo(getter_AddRefs(loadInfo)); >+ if (NS_FAILED(rv)) return rv; >+ loadInfo->SetLoadType(nsIDocShellLoadInfo::loadNormal); >+ mDocShell->LoadURI(uri, loadInfo, 0, PR_TRUE); >+ } >+ } else { I'd like to know what this is doing... you don't appear to save anything by trying to share the code. >Index: mailnews/base/resources/content/commandglue.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/commandglue.js,v >retrieving revision 1.218 >diff -u -r1.218 commandglue.js >--- mailnews/base/resources/content/commandglue.js 19 Dec 2002 05:27:43 -0000 1.218 >+++ mailnews/base/resources/content/commandglue.js 10 Feb 2003 11:27:04 -0000 >@@ -100,44 +100,44 @@ > > function setTitleFromFolder(msgfolder, subject) > { >- if (!msgfolder) return; >- > var title; >- var server = msgfolder.server; >- >+ > if (null != subject) >- title = subject+" - "; >+ title = subject; > else > title = ""; >- >- if (msgfolder.isServer) >- title += server.prettyName; >- else { >+ >+ if (msgfolder) >+ { >+ var server = msgfolder.server; >+ >+ title +=" - "; >+ This is nasty, and still wrong. I know you only want the " - " between the subject and the folder if there is a folder to display, but this always puts the " - " even if there is no subject. Try this: var title = subject || ""; if (msgfolder) { if (title) title += " - "; >Index: mailnews/base/resources/content/messageWindow.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v >retrieving revision 1.76 >diff -u -r1.76 messageWindow.js >--- mailnews/base/resources/content/messageWindow.js 17 Jan 2003 02:45:38 -0000 1.76 >+++ mailnews/base/resources/content/messageWindow.js 18 Feb 2003 09:25:21 -0000 >@@ -30,6 +30,7 @@ > > var gCompositeDataSource; > var gCurrentMessageUri; >+var gLoadCustomMessage = false; //set to true when either loading a message/rfc822 attachment or a .eml file I don't think you need this variable. You can tell a custom message is loaded because the dbView's current message key is nsMsgKey_None. This would also save you from writing the LoadMessageByMsgKey function. Also that function appears to reset the variable too late which is why it has to update the mail toolbar again. >@@ -252,24 +253,48 @@ > } > > var originalView = null; >+ var folder = null; >+ var mailUrl = null; >+ >+ if (window.arguments) >+ { >+ if (window.arguments[0]) >+ { >+ try >+ { >+ gCurrentMessageUri = window.arguments[0].QueryInterface(Components.interfaces.nsIURI); Prefer to use the if (x instanceof y) check. >+ if (gCurrentMessageUri) >+ { >+ gLoadCustomMessage = (gCurrentMessageUri.spec.indexOf("type=x-message-display") != -1); >+ if (!gLoadCustomMessage) >+ gCurrentMessageUri = gCurrentMessageUri.spec; >+ mailUrl = gCurrentMessageUri.QueryInterface(Components.interfaces.nsIMsgMailNewsUrl); >+ if (mailUrl) >+ folder = mailUrl.folder; >+ } >+ } catch(ex) {folder = null;dump("## ex=" + ex + "\n");} >+ >+ if (!gCurrentMessageUri) >+ gCurrentMessageUri = window.arguments[0]; >+ } >+ else >+ gCurrentMessageUri = null; > >- if (window.arguments) >- { >- if (window.arguments[0]) >- gCurrentMessageUri = window.arguments[0]; >- else >- gCurrentMessageUri = null; >- >- if (window.arguments[1]) >- gCurrentFolderUri = window.arguments[1]; >- else >- gCurrentFolderUri = null; >+ if (window.arguments[1]) >+ gCurrentFolderUri = window.arguments[1]; >+ else >+ { >+ if (folder) >+ gCurrentFolderUri = folder.folderURL; >+ else >+ gCurrentFolderUri = null; >+ } This looks very messy. Is there a reason that we must have a current folder? If there is, it would be nice if you could fix the JS strict warnings as well :-) Also, you don't need else { if ... } >@@ -492,6 +522,8 @@ > > function GetLoadedMessage() > { >+ if (gLoadCustomMessage) >+ return gCurrentMessageUri.spec; > return gCurrentMessageUri; Ideally keep gCurrentMessageUri as a string. >Index: mailnews/base/resources/content/msgHdrViewOverlay.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js,v >retrieving revision 1.101 >diff -u -r1.101 msgHdrViewOverlay.js >--- mailnews/base/resources/content/msgHdrViewOverlay.js 7 Feb 2003 02:36:18 -0000 1.101 >+++ mailnews/base/resources/content/msgHdrViewOverlay.js 18 Feb 2003 09:12:59 -0000 >@@ -565,6 +565,19 @@ > var headerField = currentHeaderData[headerName]; > var headerEntry; > >+ if (headerName == "subject") >+ { >+ try { >+ if (gLoadCustomMessage) >+ { >+ var folder = null; >+ if (gCurrentFolderUri) >+ folder = RDF.GetResource(gCurrentFolderUri).QueryInterface(Components.interfaces.nsIMsgFolder); >+ setTitleFromFolder(folder, headerField.headerValue); So, why is this needed?

I have change some code according to Neil's advice. 1.Solve warning of js when display a attachment as a email in message windows. 2.remove the changing of nsIMsgWindow.idl. 3.add a new function in neMessage.cpp for load a message with url 4.change the code of function setTitleFromFolder in commandglue.js according to Neil 's advice 5.add a attribute named "keyForFirstSelectedMessage" for using in js instead using gLoadCustomMessage. 6.change some codes according to Neil's advice in messageWindows.js 7.keep gCurrentMessageUri as a string in messageWindows.js

Sorry, last patch have something wrong. this is a correct patch.

Comment on attachment 118658 [details] [diff] [review] patch version 1.4 >Index: mailnews/base/public/nsIMsgWindow.idl >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/public/nsIMsgWindow.idl,v >retrieving revision 1.22 >diff -u -r1.22 nsIMsgWindow.idl >--- mailnews/base/public/nsIMsgWindow.idl 29 Nov 2001 04:56:23 -0000 1.22 >+++ mailnews/base/public/nsIMsgWindow.idl 9 Feb 2003 06:29:11 -0000 >@@ -45,6 +45,7 @@ > interface nsIMsgHeaderSink; > interface nsIPrompt; > interface nsIAuthPrompt; >+interface nsIURI; I don't think this belongs here, because you're not using it in this file. >Index: mailnews/base/src/nsMessenger.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/src/nsMessenger.cpp,v >retrieving revision 1.262 >diff -u -r1.262 nsMessenger.cpp >--- mailnews/base/src/nsMessenger.cpp 19 Mar 2003 22:27:51 -0000 1.262 >+++ mailnews/base/src/nsMessenger.cpp 27 Mar 2003 16:44:53 -0000 >@@ -83,11 +83,13 @@ > #include "nsIDOMWindowInternal.h" > #include "nsIScriptGlobalObject.h" > #include "nsIDocShell.h" >+#include "nsIDocShellLoadInfo.h" > #include "nsIDocShellTreeItem.h" > #include "nsIDocShellTreeNode.h" > #include "nsIWebNavigation.h" > > // mail >+#include "nsIMsgMailNewsUrl.h" > #include "nsMsgUtils.h" > #include "nsMsgBaseCID.h" > #include "nsIMsgAccountManager.h" >@@ -449,6 +451,37 @@ > return mDocShell->SetAllowPlugins(allowPlugins); > } > >+nsresult >+nsMessenger::LoadMessageByUrl(const char * url) LoadURL would have sufficed. What I was looking for here was a function that nsMsgDBView would call instead of OpenURL, rather than trying to load extra functions onto the existing OpenURL. I think that the only line of code from OpenURL that you actally use is SetDisplayCharset, so you could simply copy that line (with comments). >+{ >+ nsresult rv = NS_OK; >+ >+ if (!url) >+ return NS_ERROR_OUT_OF_MEMORY; Interestingly OpenURL returns NS_OK if url is null. >+ nsCOMPtr<nsIURI> uri; Declare this after the NS_ENSURE_TRUE. >+ nsAutoString uriString(NS_ConvertASCIItoUCS2(url).get()); >+ // Cleanup the empty spaces that might be on each end. >+ uriString.Trim(" "); >+ // Eliminate embedded newlines, which single-line text fields now allow: >+ uriString.StripChars("\r\n"); >+ NS_ENSURE_TRUE(!uriString.IsEmpty(), NS_ERROR_FAILURE); I'm not sure you need this cleanup. >+ rv = NS_NewURI(getter_AddRefs(uri), uriString); >+ if (NS_FAILED(rv) || !uri) >+ return NS_ERROR_FAILURE; I don't think you need to check uri, just return rv if it failed. >+ nsCOMPtr<nsIMsgMailNewsUrl> msgurl = do_QueryInterface(uri); >+ if (msgurl) >+ msgurl->SetMsgWindow(mMsgWindow); >+ if (mDocShell) { You might want to do this test earlier. >Index: mailnews/base/src/nsMsgDBView.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/src/nsMsgDBView.cpp,v >retrieving revision 1.138 >diff -u -r1.138 nsMsgDBView.cpp >--- mailnews/base/src/nsMsgDBView.cpp 23 Mar 2003 22:12:39 -0000 1.138 >+++ mailnews/base/src/nsMsgDBView.cpp 27 Mar 2003 14:16:51 -0000 >@@ -967,6 +967,23 @@ > } > > >+// Load a message. >+NS_IMETHODIMP nsMsgDBView::LoadMessageByUrl(const char* aUrl) >+{ >+ NS_ASSERTION(aUrl,"trying to load a null url"); >+ if (!aUrl) return NS_ERROR_UNEXPECTED; >+ >+ if (!mSuppressMsgDisplay) >+ { >+ nsCAutoString urlStr(aUrl); >+ mMessengerInstance->OpenURL(aUrl, PR_FALSE); >+ m_currentlyDisplayedMsgKey = nsMsgKey_None; >+ } >+ >+ return NS_OK; >+} You don't seem to use urlStr, which can save you the null pointer check (OpenURL has its own check). >@@ -5393,14 +5410,13 @@ > return PR_FALSE; > } > >-nsresult >-nsMsgDBView::GetKeyForFirstSelectedMessage(nsMsgKey *key) >+NS_IMETHODIMP nsMsgDBView::GetKeyForFirstSelectedMessage(nsMsgKey *aKeyForFirstSelectedMessage) Might as well keep NS_IMETHODIMP on its own line, and don't bother renaming key. >Index: mailnews/mime/src/mimei.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/mime/src/mimei.cpp,v >retrieving revision 1.61 >diff -u -r1.61 mimei.cpp >--- mailnews/mime/src/mimei.cpp 24 Jan 2003 05:17:42 -0000 1.61 >+++ mailnews/mime/src/mimei.cpp 18 Feb 2003 09:15:05 -0000 >@@ -1836,3 +1836,16 @@ > return 0; > } > >+char * >+mime_get_base_url(const char *url) >+{ >+ const char *s; >+ char *result; >+ >+ if (!url) return 0; Should probably be if (!url) return nsnull; >Index: mailnews/base/resources/content/commandglue.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/commandglue.js,v >retrieving revision 1.220 >diff -u -r1.220 commandglue.js >--- mailnews/base/resources/content/commandglue.js 12 Feb 2003 11:42:46 -0000 1.220 >+++ mailnews/base/resources/content/commandglue.js 27 Mar 2003 13:24:07 -0000 >@@ -101,44 +101,40 @@ > > function setTitleFromFolder(msgfolder, subject) > { >- if (!msgfolder) return; >+ var title = subject || ""; > >- var title; >- var server = msgfolder.server; >- >- if (null != subject) >- title = subject+" - "; >- else >- title = ""; >- >- if (msgfolder.isServer) >- title += server.prettyName; >- else { >+ if (msgfolder) >+ { >+ var server = msgfolder.server; >+ >+ if (title) >+ title += " - "; >+ >+ if (msgfolder.isServer) >+ title += server.prettyName; I've noticed that this is duplicated for the else case so you could probably use title += server.prettyName; if (!msgfolder.isServer) { >+ else { > var middle; > var end; > if (server.type == "nntp") { >- // <folder> on <hostname> >- middle = gMessengerBundle.getString("titleNewsPreHost"); >- end = server.hostName; >+ // <folder> on <hostname> >+ middle = gMessengerBundle.getString("titleNewsPreHost"); >+ end = server.hostName; > } else { >- var identity; >- try { >- var identities = accountManager.GetIdentitiesForServer(server); >- >- identity = identities.QueryElementAt(0, Components.interfaces.nsIMsgIdentity); >- // <folder> for <email> >- middle = gMessengerBundle.getString("titleMailPreHost"); >- end = identity.email; >- } catch (ex) { >- } >- >+ var identity; >+ try { >+ var identities = accountManager.GetIdentitiesForServer(server); >+ >+ identity = identities.QueryElementAt(0, Components.interfaces.nsIMsgIdentity); >+ // <folder> for <email> >+ middle = gMessengerBundle.getString("titleMailPreHost"); >+ end = identity.email; >+ } catch (ex) {} > } >- > title += msgfolder.prettyName; and then you can delete this line. > if (middle) title += " " + middle; > if (end) title += " " + end; >+ } > } >- > title += " - " + gBrandBundle.getString("brandShortName"); > window.title = title; > } > >Index: mailnews/base/resources/content/messageWindow.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v >retrieving revision 1.84 >diff -u -r1.84 messageWindow.js >--- mailnews/base/resources/content/messageWindow.js 23 Mar 2003 22:12:35 -0000 1.84 >+++ mailnews/base/resources/content/messageWindow.js 27 Mar 2003 16:10:09 -0000 >@@ -37,6 +37,7 @@ > var gCurrentFolderToRerootForStandAlone; > var gRerootOnFolderLoadForStandAlone = false; > var gNextMessageAfterLoad = null; >+var gLoadCustomMessage = false; //set to true when either loading a message/rfc822 attachment or a .eml file I hope you're not using this any more! >+ try >+ { >+ messageUri = window.arguments[0].QueryInterface(Components.interfaces.nsIURI); >+ if (messageUri) >+ { >+ gLoadCustomMessage = (messageUri.spec.indexOf("type=x-message-display") != -1); >+ gCurrentMessageUri = messageUri.spec; >+ mailUrl = messageUri.QueryInterface(Components.interfaces.nsIMsgMailNewsUrl); >+ if (mailUrl) >+ folder = mailUrl.folder; This is not how QueryInterface works; it will never return null when it fails so you don't need to use if. Another alternative is to use if (messageUri instanceof Components.interfaces.nsIMsgMailNewsUrl); folder = messageUri.folder; although I'm not convinced that you need to set the current folder. > function OnLoadMessageWindowDelayed() > { >- var msgKey = extractMsgKeyFromURI(gCurrentMessageUri); >- gDBView.loadMessageByMsgKey(msgKey); >+ if (gLoadCustomMessage) >+ gDBView.loadMessageByUrl(gCurrentMessageUri); Can't you do the test here instead of in OnLoadMessageWindow? >@@ -1051,4 +1071,13 @@ > return gDBView; > } > >- >+function LoadMessageByMsgKey(messageKey) >+{ >+ if (nsMsgKey_None == gDBView.keyForFirstSelectedMessage) >+ { >+ gDBView.loadMessageByMsgKey(messageKey); >+ UpdateMailToolbar("update toolbar for message Window"); Won't this update happen anyway? In which case, you can get rid of this and revert your other changes. >Index: mailnews/base/resources/content/mailWindowOverlay.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v >retrieving revision 1.156 >diff -u -r1.156 mailWindowOverlay.js >--- mailnews/base/resources/content/mailWindowOverlay.js 23 Mar 2003 22:12:35 -0000 1.156 >+++ mailnews/base/resources/content/mailWindowOverlay.js 27 Mar 2003 12:54:08 -0000 >@@ -1950,8 +1950,11 @@ > > function OnMsgLoaded(folder, aMessageURI) > { >- var msgHdr = messenger.messageServiceFromURI(aMessageURI).messageURIToMsgHdr(aMessageURI); >- SetUpJunkBar(msgHdr); >+ if (!(aMessageURI.indexOf("type=x-message-display") != -1)) You've got alot of !s here, is that right? Aside: I like using .test which works like this: if (!/type=x-message-display/.test(aMessageURI)) >+ { >+ var msgHdr = messenger.messageServiceFromURI(aMessageURI).messageURIToMsgHdr(aMessageURI); >+ SetUpJunkBar(msgHdr); >+ } You'll probably want an else SetUpJunkBar(null); here in case the previously loaded message was junk. >Index: mailnews/base/resources/content/msgHdrViewOverlay.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js,v >retrieving revision 1.102 >diff -u -r1.102 msgHdrViewOverlay.js >--- mailnews/base/resources/content/msgHdrViewOverlay.js 21 Feb 2003 04:24:21 -0000 1.102 >+++ mailnews/base/resources/content/msgHdrViewOverlay.js 27 Mar 2003 14:06:45 -0000 >@@ -565,6 +565,19 @@ > var headerField = currentHeaderData[headerName]; > var headerEntry; > >+ if (headerName == "subject") >+ { >+ try { >+ if (nsMsgKey_None == gDBView.keyForFirstSelectedMessage) >+ { >+ var folder = null; >+ if (gCurrentFolderUri) >+ folder = RDF.GetResource(gCurrentFolderUri).QueryInterface(Components.interfaces.nsIMsgFolder); Not indented correctly.

Thanks for Neil's advices, I have solved every issue beside 3 issues,which issue has been pointed out by Neil in comment #58.I didn't add the new method in interface of nsIMessage, I still choice add a new parameter for method of function named "OpenURL", I think that would make interface more clear and simple. So I choice still using OpenURL(...,...) for opening attachment as email. I didn't change my fix in function named "setTitleFromFolder" due to "title += server.prettyName" and "title += msgfolder.prettyName" was difference thing. I didn't remove the code of invoke "updateMailToolbar" in function named "LoadMessageByMsgKey", In fact, If we only load message in messageWindows without updateMailToolbar, messageWindows's toolbar and something else wouldn't be updated. So after loading a attachment as email in messageWindows, loading a normal email in same message windows,we need update the toolbar due to a lot of items and buttons has been disabled when loading the attachment as email.

Comment on attachment 118930 [details] [diff] [review] patch version 1.5 I still think a separate method is neater because you're not sharing any useful code, you're actually making the existing function harder to use. If you insist on doing it with the extra parameter then put the code back in the function, the whole idea of the separate function was to get rid of the parameter. I also think there should be a better place to update the mail toolbar, but I can't see one for now :-/ I was wondering why you need to pass a URI as the first argument, would it be possible to pass a string (that way the currentMessageUri is always window.arguments[0]) and test loadCustomMessage = /type=x-message-display/.test(messageUri); it and then if necessary turn it into a URI? As for msgFolder.prettyName this is always equal to server.prettyName when the folder is a server (line 917 of nsMsgFolder.cpp). Finally, setTimeout("OnLoadMessageWindowDelayed(" + loadCustomMessage + ")", 0); is wrong, please change it to setTimeout(OnLoadMessageWindowDelayed, 0, loadCustomMessage); Getting there :-)

Doh, I told you to delete the msgFolder.prettyName instead of server.prettyName Also, I didn't realize that the message window toolbar doesn't normally update.

Change some codes according to the discussing with Neil in IRC. I have added a new method in interface of nsIMessenger. Thus, we could invoke this method to load a custom message instead of invoke the function named "OpenURL" which has been added a parameter for point out loading a custom message in my provious patch.

Comment on attachment 119059 [details] [diff] [review] patch version 1.6 The patch didn't compile, did you forgot to diff nsMsgDBView.h ? Gave me a chance for some last nitpicking before you get r+ :-) >Index: mailnews/base/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/src/Makefile.in,v >retrieving revision 1.101 >diff -u -r1.101 Makefile.in >--- mailnews/base/src/Makefile.in 4 Oct 2002 22:32:11 -0000 1.101 >+++ mailnews/base/src/Makefile.in 9 Feb 2003 06:30:10 -0000 >@@ -103,6 +103,7 @@ > nsMsgSearchDBView.cpp \ > nsMsgOfflineManager.cpp \ > nsMsgProgress.cpp \ >+ nsMessengerContentHandler.cpp \ > nsSpamSettings.cpp \ > $(NULL) > >@@ -139,6 +140,7 @@ > nsMsgPrintEngine.h \ > nsStatusBarBiffManager.h \ > nsMsgProgress.h \ >+ nsMessengerContentHandler.h \ Oops, you've mixed tabs and spaces :-( > $(NULL) > > # we don't want the shared lib, but we want to force the creation of a static lib. > >Index: mailnews/base/src/nsMessenger.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/src/nsMessenger.cpp,v >retrieving revision 1.262 >diff -u -r1.262 nsMessenger.cpp >--- mailnews/base/src/nsMessenger.cpp 19 Mar 2003 22:27:51 -0000 1.262 >+++ mailnews/base/src/nsMessenger.cpp 1 Apr 2003 12:59:22 -0000 >@@ -587,7 +590,42 @@ > return NS_ERROR_OUT_OF_MEMORY; > } > } >- return NS_OK; >+ return rv; >+} >+ >+NS_IMETHODIMP >+nsMessenger::LoadURL(const char * url) >+{ >+ nsresult rv = NS_ERROR_FAILURE; >+ >+ if (url) Perhaps use if (url && mDocShell) here? >+ { >+ SetDisplayCharset(NS_LITERAL_STRING("UTF-8").get()); >+ >+ if (!mDocShell) return NS_ERROR_FAILURE; >+ >+ nsAutoString uriString(NS_ConvertASCIItoUCS2(url).get()); >+ // Cleanup the empty spaces that might be on each end. >+ uriString.Trim(" "); >+ // Eliminate embedded newlines, which single-line text fields now allow: >+ uriString.StripChars("\r\n"); >+ NS_ENSURE_TRUE(!uriString.IsEmpty(), NS_ERROR_FAILURE); >+ >+ nsCOMPtr<nsIURI> uri; >+ rv = NS_NewURI(getter_AddRefs(uri), uriString); >+ NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE); NS_ENSURE_SUCCESS(rv, rv); ? >+ >+ nsCOMPtr<nsIMsgMailNewsUrl> msgurl = do_QueryInterface(uri); >+ if (msgurl) >+ msgurl->SetMsgWindow(mMsgWindow); >+ >+ nsCOMPtr<nsIDocShellLoadInfo> loadInfo; >+ rv = mDocShell->CreateLoadInfo(getter_AddRefs(loadInfo)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ loadInfo->SetLoadType(nsIDocShellLoadInfo::loadNormal); >+ rv = mDocShell->LoadURI(uri, loadInfo, 0, PR_TRUE); >+ } >+ return rv; >Index: mailnews/mime/src/mimei.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/mime/src/mimei.cpp,v >retrieving revision 1.61 >diff -u -r1.61 mimei.cpp >--- mailnews/mime/src/mimei.cpp 24 Jan 2003 05:17:42 -0000 1.61 >+++ mailnews/mime/src/mimei.cpp 31 Mar 2003 08:15:02 -0000 >@@ -1836,3 +1836,16 @@ > return 0; > } > >+char * >+mime_get_base_url(const char *url) >+{ >+ const char *s; >+ char *result; >+ >+ if (!url) return nsnull; I think that general style is to put the return on its own line >Index: mailnews/base/resources/content/commandglue.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/commandglue.js,v >retrieving revision 1.220 >diff -u -r1.220 commandglue.js >--- mailnews/base/resources/content/commandglue.js 12 Feb 2003 11:42:46 -0000 1.220 >+++ mailnews/base/resources/content/commandglue.js 1 Apr 2003 12:46:18 -0000 >@@ -101,44 +101,40 @@ > > function setTitleFromFolder(msgfolder, subject) > { >- if (!msgfolder) return; >+ var title = subject || ""; > >- var title; >- var server = msgfolder.server; >+ if (msgfolder) >+ { >+ var server = msgfolder.server; Could be moved further down (see below) > >- if (null != subject) >- title = subject+" - "; >- else >- title = ""; >+ if (title) >+ title += " - "; > >- if (msgfolder.isServer) >- title += server.prettyName; >- else { >+ title += msgfolder.prettyName; >+ >+ if (!msgfolder.isServer) >+ { You could move the var server here. > var middle; > var end; > if (server.type == "nntp") { >- // <folder> on <hostname> >- middle = gMessengerBundle.getString("titleNewsPreHost"); >- end = server.hostName; >+ // <folder> on <hostname> >+ middle = gMessengerBundle.getString("titleNewsPreHost"); >+ end = server.hostName; > } else { >- var identity; >- try { >- var identities = accountManager.GetIdentitiesForServer(server); >- >- identity = identities.QueryElementAt(0, Components.interfaces.nsIMsgIdentity); >- // <folder> for <email> >- middle = gMessengerBundle.getString("titleMailPreHost"); >- end = identity.email; >- } catch (ex) { >- } >- >+ var identity; >+ try { >+ var identities = accountManager.GetIdentitiesForServer(server); >+ >+ identity = identities.QueryElementAt(0, Components.interfaces.nsIMsgIdentity); >+ // <folder> for <email> >+ middle = gMessengerBundle.getString("titleMailPreHost"); >+ end = identity.email; >+ } catch (ex) {} > } >- >- title += msgfolder.prettyName; > if (middle) title += " " + middle; > if (end) title += " " + end; >+ } > } >- > title += " - " + gBrandBundle.getString("brandShortName"); > window.title = title; > } > >Index: mailnews/base/resources/content/messageWindow.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v >retrieving revision 1.84 >diff -u -r1.84 messageWindow.js >--- mailnews/base/resources/content/messageWindow.js 23 Mar 2003 22:12:35 -0000 1.84 >+++ mailnews/base/resources/content/messageWindow.js 1 Apr 2003 12:49:10 -0000 >@@ -263,34 +263,57 @@ > } > > var originalView = null; >+ var folder = null; >+ var messageUri; >+ var loadCustomMessage = false; //set to true when either loading a message/rfc822 attachment or a .eml file >+ if (window.arguments) >+ { >+ if (window.arguments[0]) >+ { >+ try >+ { >+ messageUri = window.arguments[0].QueryInterface(Components.interfaces.nsIURI); >+ if (messageUri) messageUri = window.arguments[0]; if (messageUri instanceof Components.interfaces.nsIURI) >+ { >+ loadCustomMessage = (messageUri.spec.indexOf("type=x-message-display") != -1); Please use /type=x-message-display/.test(messageUri.spec) >Index: mailnews/base/resources/content/mailWindowOverlay.js >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v >retrieving revision 1.156 >diff -u -r1.156 mailWindowOverlay.js >--- mailnews/base/resources/content/mailWindowOverlay.js 23 Mar 2003 22:12:35 -0000 1.156 >+++ mailnews/base/resources/content/mailWindowOverlay.js 31 Mar 2003 08:54:44 -0000 >@@ -1162,7 +1162,7 @@ > windowID.gCurrentFolderUri = msgHdr.folder.URI; > windowID.UpdateMailToolbar('MsgOpenExistingWindowForMessage'); > windowID.CreateView(gDBView); >- windowID.gDBView.loadMessageByMsgKey(msgHdr.messageKey); >+ windowID.LoadMessageByMsgKey(msgHdr.messageKey); Is this an example of updating the toolbar too often? >@@ -1950,8 +1950,13 @@ > > function OnMsgLoaded(folder, aMessageURI) > { >- var msgHdr = messenger.messageServiceFromURI(aMessageURI).messageURIToMsgHdr(aMessageURI); >- SetUpJunkBar(msgHdr); >+ if (!/type=x-message-display/.test(aMessageURI)) >+ { >+ var msgHdr = messenger.messageServiceFromURI(aMessageURI).messageURIToMsgHdr(aMessageURI); >+ SetUpJunkBar(msgHdr); >+ } >+ else >+ SetUpJunkBar(null); Hmmm... might it be better to do if (/type=x-message-display/.test(aMessageURI)) SetUpJunkBar(null); // this is a standalone message else {

By the way, I was impressed that I can open an attachment which is of a message which itself has an attached message and then open that message attachment.

Sorry for forgetting nsMsgDBView.h, I have added the fixing of this file into the patch and change some code according to Neil's advice. About the issue of updating toolbar, I think it won't be invoke too often. it will be invoked after switch to display a normal email.

Comment on attachment 119151 [details] [diff] [review] patch version 1.7 Phew!

Comment on attachment 119151 [details] [diff] [review] patch version 1.7 Thanks Neil.

reviewing (and testing) this out now.

applied (dealt with some cvs conflicts), and I'm running with it. I fixed one problem, which is that you should be able to do "Create filter from message", which works. (fix in my local tree) more todo items: 1) should we disable message security info? 2) we should disable Message | Move and Message | Copy menu items 3) right click on the message body, there's a bunch we should disable there 4) how hard to make it so if I load message.eml from disk, it loads this way? that's a long standing bug request, and I think you are close. I'll continue to test and review tomorrow. if you can get a supplimental patch for 1,2,3,4 let me know, or we can wait until after this lands.

> 1) should we disable message security info? I think we should not, but make it work correctly! It is very helpful to see, whether an attached message was signed and by whom etc. Bug 195378 contains a working patch. Disabling the message security info is not a correct workaround. By fixing bug 195378 we also prevent incorrect security icons showing up in the UI.

Hello Seth,Thanks for your review. For issue 1, I agree with Kai's opinion, So I think his patch can solve the problem which is how to display the security info correctly. For issue2 & 3, I think I will add a supplimental patch for those two issues. for issue4, In fact, I have tried to complete this function, but I was failed, there have lots of code should be changed in mime module and streamconvert. so I will filed another bug for this issue. Furthermore, I think if we want to complete the issue 4, there may have another relational issues need to be solved.

I've got kaie's fix for #195378 in my tree, and I'm testing and review it, in parallel. I hope to finish testing and reviewing today.

the fix from me was for junk mail. see my change HandleJunkStatusChanged(folder). when the stand alone msg window is open (for an attachment) we need to bail out early.

fixed. landed kaie's fix for bug #195378 with this. I'll log a spin off bug on these issues: "2) we should disable Message | Move and Message | Copy menu items 3) right click on the message body, there's a bunch we should disable there 4) how hard to make it so if I load message.eml from disk, it loads this way?"

bug #201881 and bug #201882 logged on the spin off issues.

whoops, there was a problem. 1) if you have "reuse msg windows" set to true 2) open a rfc/822 attachment in a stand alone msg window 3) now try to open a message, re-using that window it would load, but the toolbar items would be disabled. we could fix this in function LoadMessageByMsgKey(), but I want to see if there's a way that results in less toolbar updating.

nother spin off bug: bug #201895, heed the mailnews.reuse_message_window pref, when opening rfc/822 attachments

Comment on attachment 120389 [details] [diff] [review] antonio's patch + kaie's patch + a fix + some code cleanup >+function LoadMessageByMsgKey(messageKey) >+{ >+ gDBView.loadMessageByMsgKey(messageKey); > >+ if (gDBView.keyForFirstSelectedMessage == nsMsgKey_None) >+ UpdateMailToolbar("update toolbar for message Window"); >+} This cleanup is wrong, we only want to update the toolbar if there was no previous selected message.

ah, I see. here was the original code from antonio / neil: +function LoadMessageByMsgKey(messageKey) +{ + if (nsMsgKey_None == gDBView.keyForFirstSelectedMessage) + { + gDBView.loadMessageByMsgKey(messageKey); + UpdateMailToolbar("update toolbar for message Window"); + } + else + gDBView.loadMessageByMsgKey(messageKey); +} I'll revert, and try it out.

reverting my change, per neil, and adding neil's comment. also, fixing a problem in the code I added to HandleJunkStatusChanged(). if there is no selection in the 3 pane, that code will have js errors when the junk status changes.

Comment on attachment 115858 [details] [diff] [review] patch version 1.3 clearing old review requests.

Comment on attachment 114013 [details] [diff] [review] patch version 1.0 clearing old review requests.

Comment on attachment 115113 [details] [diff] [review] patch version 1.2 clearing old review requests.

Comment on attachment 119151 [details] [diff] [review] patch version 1.7 clearing old review requests.

Comment on attachment 118656 [details] [diff] [review] patch version 1.4 clearing old review requests.

I've checked in my patches.

another regression, see bug #203261 (fix in hand)

another issue (bug #201881) is also related.

Using trunk build 20030513 on winxp, macosx and linux this is verified fixed. IMAP account testing: I sent myself a message with an attached .eml, when double-clicking on the eml a standalone mail message opened with the appropriate toolbar buttons disabled like they should be. I forwarded a message w/a jpg attachment, to myself as an attachment. When I double clicked on the attached message, it opened up in a stand alone mail window with the appropriate toolbar buttons disabled like they should be. I sent myself a signed message, then forwarded that signed message to myself. When I double-clicked on the attached message it opened oup in a stand alone mail window and displayed the signed icon. Note: I did see spinoff bug 201881. POP account testing: Same results as above but I did see spin off bug 203570. I still need to see what happens to a

My comment above is completed, unfortunately the last sentence was one that I deleted, not sure why I didn't see that it had not been removed from the comment area before I commmited the comment, but this bug is verified per testing I mentioned in the comment. If any of the reporters or dev have other suggested tests for this please add them to this bug and I will try them too as time permits.

*** Bug 152792 has been marked as a duplicate of this bug. ***

*** Bug 69359 has been marked as a duplicate of this bug. ***

*** Bug 55344 has been marked as a duplicate of this bug. ***