143565 - Opening a "*.eml" attachment displays the atttachment in raw text instead of opening an email message window.

Status: VERIFIED FIXED

(MailNews Core :: Attachments, defect, P2)

Description

[Trix Supremo]

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.

Comment 1

[Gayatri Rimola]

Nominating for RTM.

Comment 2

[scottputterman]

reassigning to ducarroz.

Comment 3

[Charles Rosendahl]

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.

Comment 4

[Charles Rosendahl]

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...

Comment 5

[Jean-Francois Ducarroz]

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...

Comment 6

[Kai Engert (:KaiE:)]

I think this bug is a duplicate of bug 120453.

Comment 7

[Jean-Francois Ducarroz]

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

Comment 8

[Charles Rosendahl]

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.

Comment 9

[Jean-Francois Ducarroz]

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

Comment 10

[Jean-Francois Ducarroz]

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

Comment 11

[Jean-Francois Ducarroz]

Attachment: Proposed fix for problem #1 (mime)

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

Comment 12

[Ian Neal]

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

Comment 13

[Malcolm Scott]

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?

Comment 14

[Olivier Prompt]

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) ? 

Comment 15

[John Kiff]

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!

Comment 16

[yulian chang]

Nominate this bug for Buffy.

Comment 17

[Jean-Francois Ducarroz]

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

Comment 18

[Jean-Francois Ducarroz]

Attachment: more complete patch, work in progress

Comment 19

[Jean-Francois Ducarroz]

Attachment: nsMessengerContentHandler.cpp & nsMessengerContentHandler.h

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

Comment 20

[Jean-Francois Ducarroz]

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

Comment 21

[Harry Lu]

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? 

Comment 22

[Olivier Prompt]

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.

Comment 23

[Harry Lu]

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

Comment 24

[Bob Lockie]

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.

Comment 25

[Antonio.Xu]

Reassign this bug to me.

Comment 26

[Cavin Song]

Reassigning.

Comment 27

[Antonio.Xu]

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.

Comment 28

[Antonio.Xu]

Sorry, I means Cavin.

Comment 29

[Bob Lockie]

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

Comment 30

[Antonio.Xu]

Attachment: patch version 1.0

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!

Comment 31

[Jean-Francois Ducarroz]

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?

Comment 32

[Antonio.Xu]

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

Comment 33

[Antonio.Xu]

Attachment: patch version 1.1

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!

Comment 34

[Kai Engert (:KaiE:)]

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.

Comment 35

[Antonio.Xu]

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.

Comment 36

[Kai Engert (:KaiE:)]

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

Comment 37

[Antonio.Xu]

Attachment: patch version 1.2

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 38

[Jean-Francois Ducarroz]

Comment on attachment 115113 [details] [diff] [review]
patch version 1.2

good job, this is working well. You did it man! R=ducarroz

Comment 39

[(not reading, please use [e-mail] instead)]

curious, what will this url do:

news://news.mozilla.org:119/[e-mail]

will you get that news message in a message window, or a browser window?

Comment 40

[Kai Engert (:KaiE:)]

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

Comment 41

[Kai Engert (:KaiE:)]

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

Comment 42

[Kai Engert (:KaiE:)]

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.

Comment 43

[(not reading, please use [e-mail] instead)]

this is a non-trivial patch, and will take some time to review.

I hope to have this reviewed during 1.4 alpha

Comment 44

[Kai Engert (:KaiE:)]

Attachment: Additional patch for crypto status

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

Comment 45

[Kai Engert (:KaiE:)]

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 46

[Kai Engert (:KaiE:)]

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

needs more work

Comment 47

[Kai Engert (:KaiE:)]

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

needs more work

Comment 48

[Antonio.Xu]

I will do some testing tomorrow.

Comment 49

[Antonio.Xu]

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

Comment 50

[Kai Engert (:KaiE:)]

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

Comment 51

[Antonio.Xu]

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

Comment 52

[Antonio.Xu]

Attachment: patch version 1.3

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

Comment 53

[Antonio.Xu]

I only changed some code in mimemrel.cpp.

Comment 54

[Kai Engert (:KaiE:)]

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 55

[[e-mail]]

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?

Comment 56

[Antonio.Xu]

Attachment: patch version 1.4

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

Comment 57

[Antonio.Xu]

Attachment: patch version 1.4

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

Comment 58

[[e-mail]]

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.

Comment 59

[Antonio.Xu]

Attachment: patch version 1.5

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 60

[[e-mail]]

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 :-)

Comment 61

[[e-mail]]

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.

Comment 62

[Antonio.Xu]

Attachment: patch version 1.6

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 63

[[e-mail]]

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
{

Comment 64

[[e-mail]]

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.

Comment 65

[Antonio.Xu]

Attachment: patch version 1.7

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 66

[[e-mail]]

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

Phew!

Comment 67

[Antonio.Xu]

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

Thanks Neil.

Comment 68

[(not reading, please use [e-mail] instead)]

reviewing (and testing) this out now.

Comment 69

[(not reading, please use [e-mail] instead)]

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.

Comment 70

[Kai Engert (:KaiE:)]

> 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.

Comment 71

[Antonio.Xu]

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.

Comment 72

[(not reading, please use [e-mail] instead)]

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.

Comment 73

[(not reading, please use [e-mail] instead)]

Attachment: antonio's patch + kaie's patch + a fix + some code cleanup

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.

Comment 74

[(not reading, please use [e-mail] instead)]

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?"

Comment 75

[(not reading, please use [e-mail] instead)]

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

Comment 76

[(not reading, please use [e-mail] instead)]

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.

Comment 77

[(not reading, please use [e-mail] instead)]

Attachment: supplimental fix

Comment 78

[(not reading, please use [e-mail] instead)]

nother spin off bug:

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

Comment 79

[[e-mail]]

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.

Comment 80

[(not reading, please use [e-mail] instead)]

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.

Comment 81

[(not reading, please use [e-mail] instead)]

Attachment: patch

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 82

[(not reading, please use [e-mail] instead)]

Comment on attachment 115858 [details] [diff] [review]
patch version 1.3

clearing old review requests.

Comment 83

[(not reading, please use [e-mail] instead)]

Comment on attachment 114013 [details] [diff] [review]
patch version 1.0

clearing old review requests.

Comment 84

[(not reading, please use [e-mail] instead)]

Comment on attachment 115113 [details] [diff] [review]
patch version 1.2

clearing old review requests.

Comment 85

[(not reading, please use [e-mail] instead)]

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

clearing old review requests.

Comment 86

[(not reading, please use [e-mail] instead)]

Comment on attachment 118656 [details] [diff] [review]
patch version 1.4

clearing old review requests.

Comment 87

[(not reading, please use [e-mail] instead)]

I've checked in my patches.

Comment 88

[(not reading, please use [e-mail] instead)]

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

Comment 89

[(not reading, please use [e-mail] instead)]

another issue (bug #201881) is also related.

Comment 90

[esther]

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 

Comment 91

[esther]

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. 

Comment 92

[Mike Cowperthwaite]

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

Comment 93

[Vaclav Dvorak]

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

Comment 94

[Vaclav Dvorak]

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