301291 - Forward-inline ignores outgoing-charset preference

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Elliotte Rusty Harold]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

Frequently when I'm replying to a message I am warned that the message contains
characters that cannot be sent in the current encoding and asking if I want to
send in UTF-8. I always click yes.

Thunderbird should not even ask me. It should just send in UTF-8. It should
probably never send in anything else. 

Reproducible: Couldn't Reproduce

Steps to Reproduce:
It happens frequently, but I can;t make it happen on command. It probably
depends on the message I'm replaying to.

Actual Results:  
Thunderbird asks me if I want to send in UTF-8. 

Expected Results:  
Just send in UTF-8 without bothering me.

In my opinion unless htere is some deep SMTP issue I'm unaware of, one of the
following should be done  (in decreasing order of preference)

1. Send everything in UTF-8 all the time without exception. Never ask the user.
Do not allow them to configure anything else. 
2. When necessary, send in UTF-8 without asking the user.
3. Put a checkbox in that dialog allowing the user to "Never show this alert
again" or some such. i.e. make the preference sticky.
4. Provide some hidden preference somewhere to make automatic UTF-8 the default.
If it exists already it's too hidden. I couldn't find it.

Comment 1

[Jo Hermans]

And what about email clients that don't support UTF-8 ?

Comment 2

[Elliotte Rusty Harold]

Which ones exactly? It's 2005. At this point anyone using a  client that doesn't
support UTF-8 needs to upgrade; but I think pretty much all the clients do
support UTF-8.  

In any case, do you really expect any user to know whether the recipient on the
other end is using a client that supports UTF-8 or not? 

Comment 3

[Jo Hermans]

(In reply to comment #2)
> Which ones exactly? It's 2005. At this point anyone using a  client that doesn't
> support UTF-8 needs to upgrade; but I think pretty much all the clients do
> support UTF-8.  
> 
> In any case, do you really expect any user to know whether the recipient on the
> other end is using a client that supports UTF-8 or not? 

see bug 194862 and bug 249530

Comment 4

[Justin Kerk]

Thunderbird already has a menu to select the character encoding, IMO this should
just default to UTF-8 and be made "sticky", so that if you regularly need to
communicate with people in some legacy encoding you can select that once and
have it be the default in the future.

See also bug 224391.

Comment 5

[Mike Cowperthwaite]

(In reply to comment #0)
> Thunderbird should not even ask me. It should just send in UTF-8. It should
> probably never send in anything else. 

There are two ways to make this happen:
1) Set your default outgoing preference to UTF-8 *and* check the box for
"Use the default encoding in replies."

-OR-

2) Set up the fallback preferences for UTF-8.  As an example, I have this:
  intl.fallbackCharsetList.ISO-8859-1     set to      "UTF-8"

You need to create a fallback list for every character set you might use for 
composing mail; and the string value for the preference can be a comma-separated 
list of different encodings.


(In reply to comment #4)
> [TB] already has a menu to select the character encoding, IMO this should
> just default to UTF-8 and be made "sticky"

The preference for outgoing encoding *is* sticky, and if you enforce that for 
replies as well (see (1) above), you get exactly what you ask for.

Comment 6

[Elliotte Rusty Harold]

First, I do have Thunderbird configured as you suggest, wiuth UTF-8 for outgoing
messages and "Use the default encoding in replies." and yet twice just yesterday
I still saw the dialog asking me if I wanted to send the reply in UTF-8. This
happens a few times a week. So there seems to be an out and out bug somewhere,
and at least on Mac OS X and Thunderbird 1.1 alpha 2. The preference is not
being respected.

Second, I still think this shouldn't even be a preference. It should just send
in UTF-8 and not bother the user.

For some more UTF-8 everywhere propaganda see
http://www.google.com/url?sa=t&ct=res&cd=1&url=http%3A//www.ibm.com/developerworks/xml/library/x-utf8/

Comment 7

[Mike Cowperthwaite]

(In reply to comment #6)
> First, I do have Thunderbird configured as you suggest, wiuth UTF-8 for
> outgoing messages and "Use the default encoding in replies." and yet twice
> just yesterday I still saw the dialog asking me if I wanted to send the reply
> in UTF-8. This happens a few times a week. So there seems to be an out and out 
> bug somewhere, and at least on Mac OS X and Thunderbird 1.1 alpha 2. The
> preference is not being respected.

I agree, that is a bug.  The next time you get this, hit Cancel and go back to 
the compose window; check under Options to see which encoding was in fact set.  
And, check the encoding on the message being replied to.  Report back here.

Also check if it's reproducible -- does hitting Reply again from the same 
message result in the same encoding being set on the Options dialog?  Or, 
possibly, is the encoding set as expected to UTF-8 *until* you click Send?

If you find a message where this is reproducible, it would be helpful if you 
could provide it; save it as a .EML file and attach to this bug (using the 
Create New Attachment link above) -- or, if it's sensitive, you could forward it 
(as an attachment) directly to me.

Comment 8

[Elliotte Rusty Harold]

OK. Just happenend again. This is now in 1.5 beta 1, still on Mac OS X.

Under options I see that character encoding is to Western/.ISO-8859-1. Note that
I did check and "Always use UTF-8" and "Use default encoding in replies" is
checked in my preferences. 

However, this time it did not happen when replying. It happened when forwarding
a message inline. Is there some reason forwarding doesn't pick up the default
reply encoding? I suspect it should.  

Comment 9

[Mike Cowperthwaite]

(In reply to comment #8)
> However, this time it did not happen when replying. It happened when
> forwarding a message inline.

I'd bet folding money you've actually seen this only with forward-inline.


> Is there some reason forwarding doesn't pick up the default
> reply encoding? I suspect it should.  

Forward Inline behaves much as Edit As New, which means the message is 
initialized according to the original message, mostly.  (The most visible 
exception is that Forward Inline of an HTML message is still set up as a plain 
message, if that's how the sending identity (From:) is configured, and vice 
versa.)

If instead the character set for forward-inline was always the one you 
specified, but in the opposite situation -- you were configured to always send 
ISO-8859-1 but you inline-forwarded a message containing Unicode characters -- 
you'd still be getting this prompt.

Still, I guess it would make sense for Forward-Inline to adhere to the same pref 
as Reply: if "use default encoding in replies" is checked, that encoding should 
also be used for forwarding.

I can't find a dupe, so confirming.
Reproduced with TB 1.6a1-1003, Seamonkey 1.0a-0910, Win2K: moving to Core, 
trunk, updating platform/OS.

See also bug 287517.

Comment 11

[Petr Hroudný]

Looked at mailnews/compose/src/nsMsgCompose.cpp

The code snippet providing the default charset for replies is quite
strightforward:

      // use send_default_charset if reply_in_default_charset is on.
      nsCOMPtr<nsIPrefBranch> prefs (do_GetService(NS_PREFSERVICE_CONTRACTID));
      if (prefs)
      {
        PRBool replyInDefault = PR_FALSE;
        prefs->GetBoolPref("mailnews.reply_in_default_charset",
                           &replyInDefault);
        if (replyInDefault) {
          nsXPIDLString str;
          NS_GetLocalizedUnicharPreferenceWithDefault(prefs, "mailnews.send_defa
ult_charset",
                                                      EmptyString(), str);
          if (!str.IsEmpty())
            LossyCopyUTF16toASCII(str, charset);
        }
      }

I think this bug can be resolved by putting similar code into function which handles inline-forwarding.

Comment 12

[Petr Hroudný]

Looked further at source - for Inline forwards, the function
exits 70 lines sooner:

  // If we are forwarding inline, mime did already setup the compose fields 
     therefore we should stop now

  if (type == nsIMsgCompType::ForwardInline )
    return rv;

Could someone please advice if this is the correct place for the check if
"mailnews.reply_in_default_charset" is set?

Comment 13

[Magnus Melin [:mkmelin]]

Petr: looks like it could be the correct place, please attach a patch for it if it works.

Comment 14

[Petr Hroudný]

Attachment: Patch against TB 2.0.0.6

Please check this patch.
Forward inline now works as expected (tested with TB 2.0.0.6)

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 282376 [details] [diff] [review]
Patch against TB 2.0.0.6

Thanks for putting together a patch!

However, it needs to be updated to trunk as the code is undergoing significant string cleanup.

http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompose.cpp#1749

After that, to get the code reviewed (and super reviewed) set the r? and sr? flags to the email of a suitable reviewer, like bienvenu and/or mscott (look at cvs history). 

The other flags aren't needed, at least at this point.

Not strictly necessary, but it's easier to apply patches if you diff from the source root (stand in the 'mozilla' dir when doing cvs diff).

Comment 16

[Petr Hroudný]

Attachment: Patch against CVT trunk

Updated patch against CVT trunk.

Comment 17

[David :Bienvenu]

Petr, thx for the patch. A couple comments - please use consistent braces style. This:

+      if (replyInDefault) {
+        nsString str;

should be
+      if (replyInDefault) 
+      {
+        nsString str;


+        nsCString charset;
+        NS_GetLocalizedUnicharPreferenceWithDefault(prefs, "mailnews.send_default_charset",
+                                                    EmptyString(), str);

I suspect you left out a { here: Either that or the indentation is wrong.
+        if (!str.IsEmpty())
+          LossyCopyUTF16toASCII(str, charset);
+          m_compFields->SetCharacterSet(charset.get());
+      }
+    }


could you submit a new patch with those two things addressed, and re-request review from me? Otherwise, the patch looks OK to me...

Comment 18

[Petr Hroudný]

Attachment: Patch v2

Thanks for the comments - applied.

Comment 19

[David :Bienvenu]

Comment on attachment 282732 [details] [diff] [review]
Patch v2

thx, Petr.

Comment 20

[Scott MacGregor]

Comment on attachment 282732 [details] [diff] [review]
Patch v2

thanks for the patch!

Comment 21

[Petr Hroudný]

Thanks for reviewing! Could someone with CVS write access please check it in?

I'd also like to have this included in TB 2.0.0.x, but when I look into
TB 2.0.0.6 sources, there are some differences regarding strings.

For 2.0.0.x the declarations in my patch should be modified to:

        nsXPIDLString str;
        nsXPIDLCString charset;

and also the final charset assignment should read:

          m_compFields->SetCharacterSet(charset);

Thanks.

Comment 22

[Magnus Melin [:mkmelin]]

I'm afraid only security and stability fixes go into tb2.0, so you'll have to wait for tb3. 

Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.531; previous revision: 1.530
done

->FIXED

Comment 23

[Petr Hroudný]

Well, TB3 is several months ahead AFAIK...

In our setup we need to ensure that all emails leave encoded in UTF-8 and this bug means inline-forwards leak in different charsets.

Any chance to make an exception and include it into TB2 ?

Thanks.

Comment 24

[Frederic Bezies]

Erh... I've seen a strange crash in thunderbird since this patch was added.

If I want to reply to a news article with a "à é è" or other non-english letters, composition window is opened, and a second letter, thunderbird is crashing !

Could it be related ?

I am using thunderbird under linux.

Comment 25

[Petr Hroudný]

Code from my patch is only used when you *forward* message to someone else, and
only when your preferences are set to forward inline. It does not change anything
for replies:

if (type == nsIMsgCompType::ForwardInline )
....

Comment 26

[Frederic Bezies]

Ok. Sorry for the spam. I will look at other nsMsgCompose.cpp patch. Sorry if I upset you :(

Comment 27

[Magnus Melin [:mkmelin]]

When a bug is fixed we set the target milestone to the nearest official release the fix will be in, so moz1.9 M9 in this case.

> Any chance to make an exception and include it into TB2 ?

You are free to set the approval1.8.1.8/9? flag of the patch, but generally these kind of changes aren't approved for inclusion.

Comment 28

[Petr Hroudný]

Comment on attachment 282732 [details] [diff] [review]
Patch v2

I'd like to kindly ask for inclusion into next Thunderbird 2.0 rebubild.
Please see comment #23 for motivation and #21 for 2.0 specifics. Thanks

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 282732 [details] [diff] [review]
Patch v2

Will have to wait for next release.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 282732 [details] [diff] [review]
Patch v2

next TB is 1.8.1.11

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 282732 [details] [diff] [review]
Patch v2

minusing this non-branch patch. We'd need a patch that applies if you're going to get someone else to checkin. But not sure this is something we'd take  even then.

Comment 32

[Petr Hroudný]

Attachment: Patch for 1.8 branch

This is the patch for 1.8 branch. Taking review+ from the original patch.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 297175 [details] [diff] [review]
Patch for 1.8 branch

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 34

[Petr Hroudný]

(In reply to comment #33)
> (From update of attachment 297175 [details] [diff] [review])
> approved for 1.8.1.12, a=dveditz for release-drivers

Thanks for the approval, could someone please check it into CVS? 

Comment 35

[Samuel Sidler (old account; do not CC)]

Phil said he could land this on Sunday at the earliest, or Reed can sooner.

Comment 36

[Reed Loden [:reed]]

MOZILLA_1_8_BRANCH

Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.460.2.35; previous revision: 1.460.2.34
done

Comment 37

[Petr Hroudný]

works as expected [Thunderbird 2.0.0.12pre (20080120) nightly], thanks to all involved in fixing this.