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

VERIFIED FIXED in mozilla1.4alpha

Status

MailNews Core
Attachments
P2
major
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: Trix Supremo, Assigned: Antonio.Xu)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.4alpha
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 rtm],custrtm-)

Attachments

(3 attachments, 13 obsolete attachments)

46.50 KB, patch
neil@parkwaycc.co.uk
: review-
Details | Diff | Splinter Review
941 bytes, patch
Details | Diff | Splinter Review
2.47 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

15 years ago
Summary: Opening a "*.eml" attachment displays the atttachment in raw text instead of opening an email message window. → Opening a "*.eml" attachment displays the atttachment in raw text instead of opening an email message window.

Comment 1

15 years ago
Nominating for RTM.
Keywords: nsbeta1

Comment 2

15 years ago
reassigning to ducarroz.
Assignee: mscott → ducarroz
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2 rtm]

Updated

15 years ago
Blocks: 143047

Updated

15 years ago
Whiteboard: [adt2 rtm] → [adt2 rtm],custrtm-

Comment 3

15 years ago
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.
Keywords: 4xp

Comment 4

15 years ago
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...
Status: NEW → ASSIGNED

Comment 6

15 years ago
I think this bug is a duplicate of bug 120453.
*** Bug 120453 has been marked as a duplicate of this bug. ***

Comment 8

15 years ago
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
Created attachment 89326 [details] [diff] [review]
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

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

Comment 13

15 years ago
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?

Updated

15 years ago
QA Contact: trix → yulian

Comment 14

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

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

15 years ago
Nominate this bug for Buffy.
*** Bug 83915 has been marked as a duplicate of this bug. ***
Created attachment 101104 [details] [diff] [review]
more complete patch, work in progress

Updated

15 years ago
Attachment #89326 - Attachment is obsolete: true
Created attachment 102193 [details] [diff] [review]
nsMessengerContentHandler.cpp & nsMessengerContentHandler.h

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

Comment 21

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

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

15 years ago
Raise the Priority and Severity tentatively. I agree with Olivier.
Wish this bug can be fixed soon.
Severity: normal → major
Priority: -- → P2

Comment 24

15 years ago
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.
QA Contact: yulian → stephend
(Assignee)

Comment 25

15 years ago
Reassign this bug to me.
Assignee: ducarroz → antonio.xu
Status: ASSIGNED → NEW

Comment 26

15 years ago
Reassigning.
Assignee: antonio.xu → cavin
(Assignee)

Comment 27

15 years ago
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.
Assignee: cavin → antonio.xu
(Assignee)

Comment 28

15 years ago
Sorry, I means Cavin.

Updated

15 years ago
Blocks: 26201

Comment 29

15 years ago
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
(Assignee)

Comment 30

15 years ago
Created attachment 114013 [details] [diff] [review]
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!
(Assignee)

Updated

15 years ago
Attachment #101104 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #114013 - Flags: review?(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?

Updated

15 years ago
Attachment #102193 - Flags: review?(antonio.xu)
(Assignee)

Comment 32

15 years ago
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
(Assignee)

Updated

15 years ago
Attachment #102193 - Flags: review?(antonio.xu) → review+
(Assignee)

Comment 33

15 years ago
Created attachment 114776 [details] [diff] [review]
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!
Attachment #102193 - Attachment is obsolete: true
Attachment #114013 - Attachment is obsolete: true

Comment 34

15 years ago
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.
(Assignee)

Comment 35

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

15 years ago
The tree I tried it with was from yesterday. However, I tried on Linux, maybe
that makes a difference.
(Assignee)

Comment 37

15 years ago
Created attachment 115113 [details] [diff] [review]
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 ?
Attachment #114776 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #115113 - Flags: review?(ducarroz)
Comment on attachment 115113 [details] [diff] [review]
patch version 1.2

good job, this is working well. You did it man! R=ducarroz
Attachment #115113 - Flags: review?(sspitzer)
Attachment #115113 - Flags: review?(ducarroz)
Attachment #115113 - Flags: review+
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?
Status: NEW → ASSIGNED

Comment 40

15 years ago
Seth, I just tried, you get a browser window.

Comment 41

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

Comment 42

15 years ago
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
Target Milestone: --- → mozilla1.4alpha

Comment 44

15 years ago
Created attachment 115841 [details] [diff] [review]
Additional patch for crypto status

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

Comment 45

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

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

needs more work
Attachment #115841 - Attachment is obsolete: true
Attachment #115841 - Flags: review-

Comment 47

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

needs more work
(Assignee)

Comment 48

15 years ago
I will do some testing tomorrow.
(Assignee)

Comment 49

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

Updated

15 years ago
Blocks: 195378

Comment 50

15 years ago
I have filed bug 195378 to track the S/Mime status feedback separately.
(Assignee)

Comment 51

15 years ago
I will add some code for solve the problem of comment #45.
(Assignee)

Comment 52

15 years ago
Created attachment 115858 [details] [diff] [review]
patch version 1.3

Add some code for solving the problem being mentioned in comment #45. Please
test and review my patch. Thanks.
Attachment #115113 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #115858 - Flags: superreview?(sspitzer)
Attachment #115858 - Flags: review?(ducarroz)
(Assignee)

Comment 53

15 years ago
I only changed some code in mimemrel.cpp.

Updated

15 years ago
Attachment #115858 - Flags: review?(ducarroz) → review?(kaie)

Comment 54

15 years ago
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.
Attachment #115858 - Flags: review?(kaie) → review+

Comment 55

15 years ago
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?
(Assignee)

Comment 56

15 years ago
Created attachment 118656 [details] [diff] [review]
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
(Assignee)

Updated

15 years ago
Attachment #115858 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #118656 - Flags: review?(neil)
(Assignee)

Comment 57

15 years ago
Created attachment 118658 [details] [diff] [review]
patch version 1.4

Sorry, last patch have something wrong. this is a correct patch.
Attachment #118656 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #118658 - Flags: review?(neil)

Comment 58

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

Updated

14 years ago
Attachment #118658 - Flags: review?(neil) → review-
(Assignee)

Comment 59

14 years ago
Created attachment 118930 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #118658 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #118930 - Flags: review?(neil)

Comment 60

14 years ago
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 :-)
Attachment #118930 - Flags: review?(neil) → review-

Comment 61

14 years ago
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.
(Assignee)

Comment 62

14 years ago
Created attachment 119059 [details] [diff] [review]
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.
Attachment #118930 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #119059 - Flags: review?(neil)

Comment 63

14 years ago
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
{
Attachment #119059 - Flags: review?(neil) → review-

Comment 64

14 years ago
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.
(Assignee)

Comment 65

14 years ago
Created attachment 119151 [details] [diff] [review]
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.
Attachment #119059 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #119151 - Flags: review?(neil)

Comment 66

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

Phew!
Attachment #119151 - Flags: review?(neil) → review+
(Assignee)

Comment 67

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

Thanks Neil.
Attachment #119151 - Flags: superreview?(sspitzer)
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.

Comment 70

14 years ago
> 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.
(Assignee)

Comment 71

14 years ago
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.
Created attachment 120389 [details] [diff] [review]
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.
Attachment #119151 - Attachment is obsolete: true
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?"
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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.
Created attachment 120401 [details] [diff] [review]
supplimental fix
nother spin off bug:

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

Comment 79

14 years ago
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.
Attachment #120389 - Flags: review-
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.
Created attachment 120453 [details] [diff] [review]
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 on attachment 115858 [details] [diff] [review]
patch version 1.3

clearing old review requests.
Attachment #115858 - Flags: superreview?(sspitzer)
Comment on attachment 114013 [details] [diff] [review]
patch version 1.0

clearing old review requests.
Attachment #114013 - Flags: review?(ducarroz)
Comment on attachment 115113 [details] [diff] [review]
patch version 1.2

clearing old review requests.
Attachment #115113 - Flags: review?(sspitzer)
Comment on attachment 119151 [details] [diff] [review]
patch version 1.7

clearing old review requests.
Attachment #119151 - Flags: superreview?(sspitzer)
Comment on attachment 118656 [details] [diff] [review]
patch version 1.4

clearing old review requests.
Attachment #118656 - Flags: review?(neil)
I've checked in my patches.
Blocks: 202071
another regression, see bug #203261 (fix in hand)
Blocks: 203261
No longer blocks: 203261
Blocks: 203261
Blocks: 203524
another issue (bug #201881) is also related.
Blocks: 201881

Comment 90

14 years ago
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 
Status: RESOLVED → VERIFIED

Comment 91

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

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

Comment 93

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

Comment 94

13 years ago
*** Bug 55344 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.