Closed Bug 236872 Opened 16 years ago Closed 8 years ago

Changing Character Coding Auto-Detection Scheme breaks the message display output

Categories

(MailNews Core :: Internationalization, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 6.0

People

(Reporter: cnst+bmo, Assigned: ZaneUJi)

References

(Blocks 1 open bug)

Details

(Keywords: intl)

Attachments

(2 files, 2 obsolete files)

If I change auto-detection scheme (View --- Character Coding ---
Auto-Detection), the whole message, including the header fields, is shown in
one-line using Serif font (i.e. it is not readable). If I then select any
character set, the message comes back to normal formatting.
Product: MailNews → Core
*** Bug 272053 has been marked as a duplicate of this bug. ***
Blocks: 262066
Product: Core → MailNews Core
QA Contact: marina → i18n
Attached patch Fixed how emails are reloaded (obsolete) — Splinter Review
Besides a little refactoring, I just replaced
window.content.location.reload()
with
MessengerSetForcedCharacterSet(charset)
in the case of reloading an email.

Just a reminder for newbies:
Please delete %USERPROFILE%\Local Settings\Application Data\Thunderbird\Profiles
or %USERPROFILE%\Local Settings\Application Data\Mozilla\SeaMonkey\Profiles
on WinXP. Similar action should be performed on other OSes. Or the original java script will be used.
Attachment #408246 - Flags: superreview?(bienvenu)
Attachment #408246 - Flags: review?(dmose)
That would delete all your mail and settings, so why should someone do that?
The real profiles are stored at
%USERPROFILE%\Application Data\Thunderbird\Profiles,

without "Local Settings\". The directories in Comment #3 are used by temporary files. There is just cache in. If you have concerns, rename it should do the trick.
Ah, ok, those tips were for people trying to apply your patch to a current installed build - I see now.
Yes, indeed. That saves me a lot of time. I believe that is what most people have in mind.
Comment on attachment 408246 [details] [diff] [review]
Fixed how emails are reloaded

I don't think we can review toolkit code.
Attachment #408246 - Flags: superreview?(bienvenu)
Attachment #408246 - Flags: superreview?
Attachment #408246 - Flags: review?(dmose)
Attachment #408246 - Flags: review?
Attached patch Patch (obsolete) — Splinter Review
Corrected code style.
Attachment #408246 - Attachment is obsolete: true
Attachment #448985 - Flags: review?(enndeakin)
Attachment #408246 - Flags: superreview?
Attachment #408246 - Flags: review?
Comment on attachment 448985 [details] [diff] [review]
Patch

>diff --git a/toolkit/content/charsetOverlay.js b/toolkit/content/charsetOverlay.js
>--- a/toolkit/content/charsetOverlay.js
>+++ b/toolkit/content/charsetOverlay.js
>@@ -1,60 +1,89 @@
> function MultiplexHandler(event)
> {
>-    var node = event.target;
>-    var name = node.getAttribute('name');
>-
>-    if (name == 'detectorGroup') {
>-        SetForcedDetector(true);
>-        SelectDetector(event, false);
>-    } else if (name == 'charsetGroup') {
>-        var charset = node.getAttribute('id');
>-        charset = charset.substring('charset.'.length, charset.length)
>-        SetForcedCharset(charset);
>-    } else if (name == 'charsetCustomize') {
>-        //do nothing - please remove this else statement, once the charset prefs moves to the pref window
>-    } else {
>-        SetForcedCharset(node.getAttribute('id'));
>-    }
>+    MultiplexHandlerEx(event, browserCharsetHandler);
> }
> 
> function MailMultiplexHandler(event)
> {
>-    var node = event.target;
>-    var name = node.getAttribute('name');
>-
>-    if (name == 'detectorGroup') {
>-        SelectDetector(event, true);
>-    } else if (name == 'charsetGroup') {
>-        var charset = node.getAttribute('id');
>-        charset = charset.substring('charset.'.length, charset.length)
>-        MessengerSetForcedCharacterSet(charset);
>-    } else if (name == 'charsetCustomize') {
>-        //do nothing - please remove this else statement, once the charset prefs moves to the pref window
>-    } else {
>-        MessengerSetForcedCharacterSet(node.getAttribute('id'));
>-    }
>+    MultiplexHandlerEx(event, mailCharsetHandler);
> }
> 
> function ComposerMultiplexHandler(event)
> {
>-    var node = event.target;
>-    var name = node.getAttribute('name');
>+    MultiplexHandlerEx(event, composerCharsetHandler);
>+}
> 
>-    if (name == 'detectorGroup') {
>-        ComposerSelectDetector(event, true);
>-    } else if (name == 'charsetGroup') {
>-        var charset = node.getAttribute('id');
>-        charset = charset.substring('charset.'.length, charset.length)
>+var browserCharsetHandler = {
>+    SetForcedDetector: function Browser_SetForcedDetector() {
>+        BrowserSetForcedDetector(true);
>+    },
>+    SelectDetector: function Browser_SelectDetector(event) {
>+        SelectDetector(event, null);
>+    },

Did you mean to pass this.Reload here?

Both SetForcedDetector and SelectDetector are always called in sequence. Perhaps just one function defined that does both together instead.

I think also I would prefer if the functions were just passed to MultiplexHandlerEx instead of using a separate object. (Or, at least, define them, for example, in MultiplexHandler/MailMultiplexHandler/ComposerMultiplexHandler.
(In reply to comment #10)
> >+var browserCharsetHandler = {
> >+    SetForcedDetector: function Browser_SetForcedDetector() {
> >+        BrowserSetForcedDetector(true);
> >+    },
> >+    SelectDetector: function Browser_SelectDetector(event) {
> >+        SelectDetector(event, null);
> >+    },
> 
> Did you mean to pass this.Reload here?
> 
No, because the origin code is
SelectDetector(event, false);
However, I'm not sure if It is correct.

> Both SetForcedDetector and SelectDetector are always called in sequence.
> Perhaps just one function defined that does both together instead.
> 
> I think also I would prefer if the functions were just passed to
> MultiplexHandlerEx instead of using a separate object. (Or, at least, define
> them, for example, in
> MultiplexHandler/MailMultiplexHandler/ComposerMultiplexHandler.

OK.
Attached patch PatchSplinter Review
Attachment #448985 - Attachment is obsolete: true
Attachment #451626 - Flags: review?(enndeakin)
Attachment #448985 - Flags: review?(enndeakin)
(In reply to comment #12)

There are two issues.
(a) When Auto Detect setting is touched, internal reload is invoked.
(b) When internal reload is invoked, garbled display is shown, because whole
    mail data starts "From -" line, unix mbox separator, is rendered as HTML.
To resolve this bug(and some other bugs in dependency tree, perhaps dup), I think fixing of (a) is mandatory and sufficient.
However, "internal reload" may happen by other reasons. AFAIK, internal reload was invoked upon charset change by auto-detect. I don't know it still occurs or not.
Zane U. Ji, do we need to open bug for issue of (b)?
Yes. However, there may have already been one.
(In reply to comment #14)
> Yes. However, there may have already been one.

If you are talking about my bug 506504, you are wrong. Even though I pointed issue of (b) in that bug, that bug is for issue of (a), i.e. dup of this bug.
And, although some other bugs report phenomenon of "internal reload" in some situations followed by phenomenon of (a), they are mainly for issue like (a) in some situations instead of for phenomenon of (b) itself.
Zane U. Ji, can you open bug for issue of (b)?
Neil (enn): ping for the re-review on this please?
Comment on attachment 451626 [details] [diff] [review]
Patch

OK, finally had a chance to look at this.
Attachment #451626 - Flags: review?(enndeakin) → review+
Zane U. Ji, do you need someone to check the patch in for you?
Assignee: smontagu → ZaneUJi
Whiteboard: [has patch][needs checkin]
Yes, please.
I'll push this later today.
Summary: Changing Auto-Detection Scheme breaks the message display output → Changing Character Coding Auto-Detection Scheme breaks the message display output
Checked in: http://hg.mozilla.org/mozilla-central/rev/83b30b8ad22c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs checkin]
Target Milestone: --- → Thunderbird 3.4
I couldn't see this bug's problem by Auto-Detect setting change with Tb 7.0.1.
Status: RESOLVED → VERIFIED
No longer blocks: 506504
Duplicate of this bug: 506504
You need to log in before you can comment on or make changes to this bug.