accented characters not sent correctly with simple mapi blind send

RESOLVED FIXED

Status

MailNews Core
Simple MAPI
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

({fixed-aviary1.0, intl})

Trunk
x86
Windows XP
fixed-aviary1.0, intl

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

847 bytes, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
2.25 KB, patch
Jungshik Shin
: review+
Scott MacGregor
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
If you do a blind send of body text that contains accented characters, they're
not sent correctly. Fix upcoming.
(Assignee)

Comment 1

14 years ago
Created attachment 152630 [details] [diff] [review]
proposed fix

this fix is against the m4 branch, so I'll need to port it to the trunk, and
the aviary branch.

The fix is to try to mimic what happens when we do a send from the plain text
compose window, in terms of charset conversions, and to skip entity conversion
at send time, if we're doing mapi.  I'm not sure this is the right way to do
all this, but I'm pretty sure things were pretty broken before, so this is
better.
(Assignee)

Updated

14 years ago
Attachment #152630 - Flags: superreview?(mscott)
Attachment #152630 - Flags: review?(jshin)

Updated

14 years ago
Attachment #152630 - Flags: superreview?(mscott) → superreview+

Comment 2

14 years ago
Comment on attachment 152630 [details] [diff] [review]
proposed fix

>Index: mapi/mapihook/src/msgMapiHook.cpp


>+      unicodeBody.AssignWithConversion(aMessage->lpszNoteText);

This should be either
NS_CopyNativeToUnicode(nsDependentCString(aMessage->lpszNoteText), unicodeBody)

or equivalent (but a bit 'generous' implementation in nsMsgI18NUtil.cpp). As it
is now, lpszNoteText (I'm assuming here it's in the platform character
encoding) in non-ISO-8859-1 will be incorrectly inflated to UTF-16.  

>+      if (unicodeBody.Last() != nsCRT::LF)
>+        unicodeBody.AppendWithConversion(CRLF);

   AppendASCIItoUTF16(CRLF, unicodeBody); 

(xxxWithConversion's are all deprecated, I think)

Comment 3

14 years ago
Comment on attachment 152630 [details] [diff] [review]
proposed fix

I think we shouldn't change msgMapiHook.cpp to fix this problem.
|setBodyFromCString(in string)| is scary (at least it should be marked
[noscript]).

As it stands, it seems like the current code should just work.	
nsMapiHook::BlindSendMail seems to do the right thing (on the trunk), but may
not. Or, we may have to do some special-casing for the blind MAPI sending in
nsMsgCompose::SendMsg.


BTW, mapi/mapihook/src/msgMapiHook.cpp
has a number of incorrect inflation to UTF-16 with *WithConversion, but that
should be a separate bug.  I'll file a bug for that.
Attachment #152630 - Flags: review?(jshin) → review-

Updated

14 years ago
Keywords: intl
(Assignee)

Comment 4

14 years ago
I'm open to suggestions - if it works on the trunk, that's great. I'll try it. I
thought I did, but I may have just been debugging on a branch. 

nsMsgCompose::SendMsg doesn't know we're doing a blind send - and it's too late
at that point...

Comment 5

14 years ago
(In reply to comment #4)
> I'm open to suggestions - if it works on the trunk, that's great. I'll try it. 
> I thought I did, but I may have just been debugging on a branch.

Sorry for the confusion. I didn't try it myself, but I thought it'd work because
 I incorrectly assumed that SendMsg would convert |m_body| in UTF-16 (nsString)
(PRUnichar/AString) to a C string in the mail charset. It turns out that
|m_body| is nsCString which is sometimes interpreted as UTF-8 and other times
as, well, a C string in some character encoding. I don't like this 'overloading'
hack  at all,  but it has worked so far.  I think we have to overhaul
|nsIMsgCompField| to remove that strange 'overloading'.  (now I recall I planned
to work on this....)

For this bug, would you address what I raised in comment #2? It'd be also nice
to add a very conspicuous warning to |SetBodyFromCString| in nsIMsgCompField.idl
that this only works with a particular implementation of the interface in which
'body' is stored in |nsString|. 

Comment 6

14 years ago
> 'body' is stored in |nsString|

Sorry it's |nsCString| (or char*). (actually, PRUnichar* and nsString can be
used as an inflated storage for C strings, but that has to be avoided like plague)

Comment 7

14 years ago
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompose.cpp#883

883 rv = nsMsgI18NSaveAsCharset(attachment1 [details] [diff] [review]_type,m_compFields->GetCharacterSet(), 
884      NS_ConvertASCIItoUCS2(bodyString).get(),&outCString,
885      nsnull, &isAsciiOnly);

This call must be the cause of the problem.  Obviously, NS_ConvertASCIItoUCS2()
is wrong when bodyString is in UTF-8. 
 

Comment 8

14 years ago
David, can you try replacing NS_ConvertASCIItoUCS2() with
NS_ConvertUTF8toUTF16() without changing anything on the MAPI side? It's inside
|if (!entityConversionDone)| so that it should work. 

(Assignee)

Comment 9

14 years ago
that works, thx a lot - you rock!
(Assignee)

Comment 10

14 years ago
Created attachment 153528 [details] [diff] [review]
proposed fix
Attachment #152630 - Attachment is obsolete: true
(Assignee)

Comment 11

14 years ago
I'll check this in r/sr=me - thx again!
(Assignee)

Updated

14 years ago
Attachment #153528 - Flags: superreview+
Attachment #153528 - Flags: review?
(Assignee)

Updated

14 years ago
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED

Comment 12

14 years ago
Glad that the one-liner fixed it :-)
(Assignee)

Comment 13

14 years ago
this fix doesn't work for the plain text compose window. I'm trying to figure
out why...if you set your default compose window to be plain text, the
characters get changed to entities...

Comment 14

14 years ago
(In reply to comment #13)
> this fix doesn't work for the plain text compose window. I'm trying to figure
> out why...if you set your default compose window to be plain text, the
> characters get changed to entities...

 Is it the case even if those characters are covered by your default mail-send
character encoding?  Even if your answer is no, we still have a bug here. It
should prompt users to pick how to proceed (or should use question marks in
text/plain)
(Assignee)

Comment 15

14 years ago
Yes, I'm pretty sure the characters are covered by my default mail-send
encoding. They're just your basic accented french characters. This is blind mapi
send, so we can't put up a prompt. I don't know what will happen if the chars
aren't in my default encoding. Keep in mind that I'm only talking about the mapi
blind send case - there is no actual compose window, but there's still code that
looks at the default compose window type...

I believe I know what's going on here - we're doing entity conversion even in
plain text, which is just wrong, for the mapi blind send case. I think I can fix
it - but I'm generally wrong about issues like this :-(
(Assignee)

Comment 16

14 years ago
Created attachment 156447 [details] [diff] [review]
proposed fix
(Assignee)

Comment 17

14 years ago
Comment on attachment 156447 [details] [diff] [review]
proposed fix

this fix basically checks if there's no editor, we still convert the message
body to the default mail charset, and if there's no editor, and we're not doing
html compose, don't do html entity conversion.
Attachment #156447 - Flags: review?(jshin)
(Assignee)

Updated

14 years ago
Attachment #156447 - Flags: superreview?(mscott)
(Assignee)

Comment 18

14 years ago
re-opening for plain text compose window case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

14 years ago
Attachment #156447 - Flags: superreview?(mscott) → superreview+

Comment 19

14 years ago
Comment on attachment 156447 [details] [diff] [review]
proposed fix

r=jshin

Because I didn't realize that _SendMsg calls nsSaveAsCharset once more if
'EntityConversion' flag is not set here, I had a hard time why it didn't work
before and how this patch could fix it.. Now everything is clear :-)

>Index: nsMsgCompose.cpp

>     // The plain text compose window was used
>     const char contentType[] = "text/plain";
>     nsAutoString msgBody;
>     nsAutoString format;format.AssignWithConversion(contentType);

   While you're at it, can you make 'format' NS_NAMED_LITERAL_STRING' (i.e. |
NS_NAMED_LITERAL_STRING(format, "text/plain");|)? 
And, perhaps it'd be also a good idea to change msgBody to nsString because
it's likely to be longer than 64 units.

>+    if (m_editor)
>+    {
	.....
>+      m_compFields->SetBody((const char *)nsnull);
> 
>     const char *charset = m_compFields->GetCharacterSet();
>     if(UseFormatFlowed(charset))
>         flags |= nsIDocumentEncoder::OutputFormatFlowed;

The 3 lines above should be indented further now that they're in |if
(m_editor)| clause.
Attachment #156447 - Flags: review?(jshin) → review+

Comment 20

14 years ago
(In reply to comment #19)

>    While you're at it, can you make 'format' NS_NAMED_LITERAL_STRING' (i.e. |
> NS_NAMED_LITERAL_STRING(format, "text/plain");|)? 

It turned out that 'format' is used only once in the following line:

 rv = m_editor->OutputToString(format, flags, msgBody);

Then, we should just use NS_LITERAL_STRING

 rv = m_editor->OutputToString(NS_LITERAL_STRING("text/plain"),......);
(Assignee)

Comment 21

14 years ago
thx, I'll address your comments before checking in - except for the indentation,
which is just because it's a -w diff. 
(Assignee)

Comment 22

14 years ago
fixed
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 23

14 years ago
David, how about aviary-1.0 and 1.7branch? Both patches here seem simple enough
for branch check-ins.
(Assignee)

Comment 24

14 years ago
it is fixed on the aviary branch - I'll ask for 1.7 branch approval. I can't
remember if your other fix went into the 1.7 branch or not - I don't think so.
I'll ask approval for both.
(Assignee)

Updated

14 years ago
Attachment #153528 - Flags: approval1.7.3?
(Assignee)

Updated

14 years ago
Attachment #156447 - Flags: approval1.7.3?

Comment 25

14 years ago
Comment on attachment 156447 [details] [diff] [review]
proposed fix

I didn't approve the other patch because it still doesn't have a review flag
set.

a=mkaply
Attachment #156447 - Flags: approval1.7.3? → approval1.7.3+

Comment 26

14 years ago
Comment on attachment 153528 [details] [diff] [review]
proposed fix

a=mkaply
Attachment #153528 - Flags: approval1.7.3? → approval1.7.3+
Product: MailNews → Core

Updated

14 years ago
Attachment #153528 - Flags: review?
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.