25.83 KB, patch
Karsten Düsterloh: review+
|Details | Diff | Splinter Review|
5.28 KB, patch
|Details | Diff | Splinter Review|
48.70 KB, image/png
66.60 KB, image/png
Created attachment 294962 [details] [diff] [review] proposed fix Per Karsten ~ bug 224391 comment 18. This patch gets rid of the "Send in UTF-8" dialog, and silently switches to utf-8 when necessary. It's protecting users from making unnecessary choices most of them not understand. I didn't really see why we should let anyone choose to send broken mails anyway.
Comment on attachment 294962 [details] [diff] [review] proposed fix Many thanks for taking this, Magnus!
> This patch gets rid of the "Send in UTF-8" dialog, and silently switches to > utf-8 when necessary. It's protecting users from making unnecessary choices > most of them not understand. Agreed, that dialog is more confusing than helpful. My only concern is that the patch apparently does not honor the mailnews.reply_in_default_charset setting. Thus, if replying to a message that contains characters which cannot be encoded in the default character set, the presence of those characters would prompt the message to be sent in UTF-8 despite the user preference of always applying the selected default character encoding. Maybe treat as "sendInUTF8" for mailnews.reply_in_default_charset=false but as "sendAnyway" if mailnews.reply_in_default_charset=true?
I don't see how that's related, this would only kick in if your reply contains characters the selected charset can't handle, the selected charset being set by the pref or not.
Unless I'm missing something, it should kick in if you are replying to a message which is in a different character encoding than your own default. Quoting that message, when it contains characters not representable in your default encoding, would prompt the upgrade to UTF-8, correct? In that case, the patch's behavior would override the user's preference to always reply in his or her own encoding. While for the reasons you mention it is questionable to send out a message with broken characters, that preference should be honored if set by the user.
(In reply to comment #5) > Unless I'm missing something, it should kick in if you are replying to a > message which is in a different character encoding than your own default. Only if they can't coexist, so only for some cases. "Always reply in my default encoding" would instead become "always reply in my default encoding when possible", which I assume is what people expect. Why would anyone contentiously send broken messages (in 2008, when it's perfectly acceptable to assume utf-8 support)? There is no point supporting a pref just for the sake of it.
Ok, I see your point. My understanding was that this preference means to send the user's encoding "no matter what"; given your interpretation, it would simply mean to start off with the user's preferred encoding rather than the actual encoding of the message replied to, then move to UTF-8 if a character doesn't fit. Concern resolved.
This bug is actually trying to avoid broken messages. If someone sets default charset to iso-8859-1, it would be unwise to enforce it ultimately and completely disallow e.g. latin-2 characters completely. So yes, it should work as you describe - if reply_in_default_charset is set, Thunderbird should start off with your default charset (and not with the charset from original message). But if the text doesn't fit it should silently switch to UTF-8 like in all other cases without asking the users questions they don't understand and without giving them opportunity to break emails by clicking "Send anyway".
Thanks, makes perfectly sense now. I was reading that preference too literally.
I agree with comment #6, but in that case the UI text for that pref should be changed to "always reply in my default encoding when possible" (preferably in all localizations), rather than being left as it is.
David: any concerns with this, or is it just waiting in the sr queue?
Comment on attachment 294962 [details] [diff] [review] proposed fix no, I agree that killing this dialog would be a good thing, and is worth trying out.
Checking in mail/components/compose/content/MsgComposeCommands.js; /cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v <-- MsgComposeCommands.js new revision: 1.127; previous revision: 1.126 done Checking in mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties; /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties,v <-- composeMsgs.properties new revision: 1.17; previous revision: 1.16 done Checking in mailnews/compose/resources/content/MsgComposeCommands.js; /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v <-- MsgComposeCommands.js new revision: 1.413; previous revision: 1.412 done Checking in mailnews/compose/src/nsComposeStrings.h; /cvsroot/mozilla/mailnews/compose/src/nsComposeStrings.h,v <-- nsComposeStrings.h new revision: 1.7; previous revision: 1.6 done Checking in mailnews/compose/src/nsMsgCompose.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v <-- nsMsgCompose.cpp new revision: 1.558; previous revision: 1.557 done Checking in mailnews/compose/src/nsMsgPrompts.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgPrompts.cpp,v <-- nsMsgPrompts.cpp new revision: 1.32; previous revision: 1.31 done Checking in mailnews/compose/src/nsMsgPrompts.h; /cvsroot/mozilla/mailnews/compose/src/nsMsgPrompts.h,v <-- nsMsgPrompts.h new revision: 1.9; previous revision: 1.8 done Checking in mailnews/compose/src/nsMsgSend.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgSend.cpp,v <-- nsMsgSend.cpp new revision: 1.419; previous revision: 1.418 done Checking in mailnews/compose/src/nsMsgSendReport.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgSendReport.cpp,v <-- nsMsgSendReport.cpp new revision: 1.17; previous revision: 1.16 done
Missed one file earlier. Checking in suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties; /cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties,v <-- composeMsgs.properties new revision: 1.90; previous revision: 1.89 done
Created attachment 311842 [details] [diff] [review] proposed fix (wording change for replyInDefaultCharset) Change the wording for the mailnews.reply_in_default_charset. The behaviour of *the pref* didn't really change, but now the charset is set to utf-8 as send time if need be. Screen shots coming
Comment on attachment 311842 [details] [diff] [review] proposed fix (wording change for replyInDefaultCharset) >-<!ENTITY replyInDefaultCharset.label "Always use this default character encoding in replies. (When unchecked, only new messages use this default.)"> >-<!ENTITY replyInDefaultCharset.accesskey "w"> >+<!ENTITY replyInDefaultCharset2.label "When possible, use this default character encoding in replies. (When unchecked, only new messages use this default.)"> >+<!ENTITY replyInDefaultCharset2.accesskey "w"> Please change the access key to upper case to match the new text.
Checking in mailnews/base/prefs/resources/content/pref-character_encoding.xul; /cvsroot/mozilla/mailnews/base/prefs/resources/content/pref-character_encoding.xul,v <-- pref-character_encoding.xul new revision: 1.3; previous revision: 1.2 done Checking in suite/locales/en-US/chrome/mailnews/pref/pref-character_encoding.dtd; /cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/pref/pref-character_encoding.dtd,v <-- pref-character_encoding.dtd new revision: 1.3; previous revision: 1.2 done Checking in mail/components/preferences/fonts.xul; /cvsroot/mozilla/mail/components/preferences/fonts.xul,v <-- fonts.xul new revision: 1.15; previous revision: 1.14 done Checking in mail/locales/en-US/chrome/messenger/preferences/fonts.dtd; /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/preferences/fonts.dtd,v <-- fonts.dtd new revision: 1.10; previous revision: 1.9 done ->FIXED
I generally agree with the approach taken here. But I want to discuss the following problem and see if we can accomodate it. I am a German national that frequently writes mails to Japanese users. For some very strange reasons, the adoption of UTF-8 in Japan is still lacking to say the least. This patch will lead to a lot of mails that are unreadable on the other end. Not because the mail is incorrect, but because the recipient's software does not understand UTF-8. A lot of times this will be out of the recipient's control. Many Japanese receive mails on their mobile phone. Can we maybe have a configurable list of encodings (try iso-8859-15 first, then iso-2022-jp, then default to utf-8)? any possible other ways to solve this?
(In reply to comment #21) > This patch will lead to a lot of mails that are unreadable on > the other end. Not because the mail is incorrect, but because the recipient's > software does not understand UTF-8. This patch only changes the last resort. > Can we maybe have a configurable list of encodings (try iso-8859-15 first, > then iso-2022-jp, then default to utf-8)? any possible other ways to solve > this? You already can, that's what the intl.fallbackCharsetList.* pref "family" is for. You can configure global encoding defaults in Mail&News -> Character Encoding, but we don't have a pref panel for configuring custom fallbacks. In either case, please file new bugs for any problems/regressions with this one.
(In reply to comment #22) > that's what the intl.fallbackCharsetList.* pref "family" is for. I am using Thunderbird. And the above setting does not seem to change anything. I opened bug 448842 about the regression issues wrt Japanese users that this fix introduces.