Closed Bug 934492 Opened 11 years ago Closed 10 years ago

Adjust the languages preference pane to take into account Bug 910192 which removes intl.charset.default and deduces the fallback from the locale

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(seamonkey2.26 affected, seamonkey2.27 affected, seamonkey2.28 affected, seamonkey2.29 fixed, seamonkey2.30 fixed)

RESOLVED FIXED
seamonkey2.30
Tracking Status
seamonkey2.26 --- affected
seamonkey2.27 --- affected
seamonkey2.28 --- affected
seamonkey2.29 --- fixed
seamonkey2.30 --- fixed

People

(Reporter: philip.chee, Assigned: neil)

References

Details

Attachments

(3 files, 3 obsolete files)

Bug 910192 Comment 0
> Leaving the management of the HTML and plain text fallback encoding to
> localizations isn't quite working: bug 910163, bug 910165, bug 910169, bug
> 910179, bug 910181, bug 910187, bug 844087, bug 844082, bug 522218, bug
> 844114.
> 
> Instead of expecting localizations to set intl.charset.default correctly, We
> should make the code that currently reads the pref read the language code of
> the localization instead and use a lookup table of known non-windows-1252
> locales to choose a fallback encoding.
> 
> The problem with only doing what's proposed in the previous paragraph is
> that it would remove the possibility of using a localization out of locale.
> For example, it would remove the possibility of configuring an en-US build
> to use Shift_JIS as the fallback encoding so that the build is suitable for
> use in Japan. To that end, the current UI for changing intl.charset.default
> should be replaced with UI for managing a new non-localizable preference for
> overriding the hard-coded lookup table. It should be possible to set this
> override only to encodings that also exist in the lookup table (plus
> windows-1252). The UI labels for these encodings could then be less
> technical: e.g. Western, Central European (Windows), Central European (ISO),
> Greek, Turkish, Cyrillic, Traditional Chinese, Simplified Chinese, Korean,
> Japanese, Thai, etc., since  with the exception of Central European, each of
> these would mean a single  legacy encoding.

Bug 910192 Comment 1
> The override pref menu (in Preferences: Content: Font & Colors: Advanced:
> Character Encoding for Legacy Content) should probably have these items:
>  * Default for Current Locale
>  * Arabic
>  * Baltic
>  * Central European (ISO)
>  * Central European (Microsoft)
>  * Chinese, Simplified
>  * Chinese, Traditional
>  * Cyrillic
>  * Hebrew
>  * Japanese
>  * Korean
>  * Thai
>  * Turkish
>  * Other (incl. Western European)
> 
> Where "Default for Current Locale" means: Choose which other item on the
> list to actually use based on the currently active language pack.
> 
> The label for "Other" is a bit awkward, but it was just "Other", I bet some
> people who should be choosing "Other" would choose either of the Central
> European options. In any case, this short list would be much less of a
> footgun than our current list. And of course, users who don't mismatch their
> UI locale and the content they browse never need to touch this setting.
We should also port Bug 846221 at the same time (Labeling for the intl.charset.default pref should better communicate its purpose).
See Also: → 846221
Oops, Neil points out the ewong ported Bug 846221 in 895751.
See Also: 846221
Attached patch proposted patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8337519 - Flags: review?(iann_bugzilla)
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8337519 - Attachment is obsolete: true
Attachment #8337519 - Flags: review?(iann_bugzilla)
Attachment #8337531 - Flags: review?(iann_bugzilla)
Attachment #8337531 - Attachment is obsolete: true
Attachment #8337531 - Flags: review?(iann_bugzilla)
Attached patch Proposed Patch. (v3) (obsolete) — Splinter Review
Previous patches had errors that I missed.
Attachment #8337533 - Flags: review?(iann_bugzilla)
Comment on attachment 8337533 [details] [diff] [review]
Proposed Patch. (v3)

I'm not a fan of "incl." but I'm not sure what would fit on a Mac - does "Other (including Western European)" fit or should we go with "Other/Western European"?
Attachment #8337533 - Flags: feedback?(stefanh)
Comment on attachment 8337533 [details] [diff] [review]
Proposed Patch. (v3)

> I'm not a fan of "incl." but I'm not sure what would fit on a Mac - does
> "Other (including Western European)" fit or should we go with "Other/Western
> European"?

"Other (including Western European)" fits fine. That said, what do we mean by saying "Other (including Western European)"?

Ewong, mind filing a bug for updating help?
Attachment #8337533 - Flags: feedback?(stefanh) → feedback+
(In reply to Stefan [:stefanh] from comment #7)
> Comment on attachment 8337533 [details] [diff] [review]
> Proposed Patch. (v3)
> 
> > I'm not a fan of "incl." but I'm not sure what would fit on a Mac - does
> > "Other (including Western European)" fit or should we go with "Other/Western
> > European"?
> 
> "Other (including Western European)" fits fine. That said, what do we mean
> by saying "Other (including Western European)"?
> 
> Ewong, mind filing a bug for updating help?

Sure.  Bug 945078.
Attachment #8337533 - Attachment is obsolete: true
Attachment #8337533 - Flags: review?(iann_bugzilla)
Attachment #8340852 - Flags: review?(iann_bugzilla)
Attachment #8340852 - Flags: feedback+
(In reply to Stefan [:stefanh] from comment #7)
> Comment on attachment 8337533 [details] [diff] [review]
> Proposed Patch. (v3)
> 
> > I'm not a fan of "incl." but I'm not sure what would fit on a Mac - does
> > "Other (including Western European)" fit or should we go with "Other/Western
> > European"?
> 
> "Other (including Western European)" fits fine. That said, what do we mean
> by saying "Other (including Western European)"?
> 
> Ewong, mind filing a bug for updating help?

After filing that help bug and looking at the help,  I'm not exactly sure what needs to be updated.  Can  you point out what it is?
Flags: needinfo?(stefanh)
(In reply to Edmund Wong (:ewong) from comment #10)
> (In reply to Stefan [:stefanh] from comment #7)
> > Comment on attachment 8337533 [details] [diff] [review]
> > Proposed Patch. (v3)
> > 
> > > I'm not a fan of "incl." but I'm not sure what would fit on a Mac - does
> > > "Other (including Western European)" fit or should we go with "Other/Western
> > > European"?
> > 
> > "Other (including Western European)" fits fine. That said, what do we mean
> > by saying "Other (including Western European)"?
> > 
> > Ewong, mind filing a bug for updating help?
> 
> After filing that help bug and looking at the help,  I'm not exactly sure
> what needs to be updated.  Can  you point out what it is?

Hmm... no, sorry - I now see that you're not changing anything that requires an help update :-/
Flags: needinfo?(stefanh)
Attachment #8340852 - Flags: review?(iann_bugzilla) → review+
Hmm, looks like this isn't the only place affected; pref-character_encoding.xul is affected, as is folderProps.xul and am-server.xul (but they're in mailnews...) and EditorSaveAsCharset.xul too.
Comment on attachment 8340852 [details] [diff] [review]
proposed patch (v4)

>   Services.obs.notifyObservers(null, "charsetmenu-selected", "other");
Nit: don't need this any more

> <!ENTITY languages.customize.Fallback.accesskey         "C">
> <!ENTITY languages.customize.Fallback.desc              "Used for legacy content that fails to declare its encoding.">
> <!-- LOCALIZATION NOTE  Character Encoding Preferences Dialog: Do NOT localize the terms "en-bz, ar-jo"  -->
> <!ENTITY languages.customize.others.examples            "e.g.: en-bz, ar-jo">
> <!ENTITY languages.customize.moveUp.label               "Move Up">
> <!ENTITY languages.customize.moveUp.accesskey           "U">
> <!ENTITY languages.customize.moveDown.label             "Move Down">
> <!ENTITY languages.customize.moveDown.accesskey         "D">
>+
>+<!ENTITY languages.customize.Fallback.auto        "Default for Current Locale">
Nit: all the new entities belong above the dialog entities (i.e. languages.customize.Fallback.auto after languages.customize.Fallback.desc and languages.customize.Fallback.other before the l10n note)
Nit: Please line up the strings with the rest of the strings in this file i.e.

<!ENTITY languages.customize.Fallback.desc              "Used for legacy content that fails to declare its encoding.">
<!ENTITY languages.customize.Fallback.auto              "Default for Current Locale">
...
<!ENTITY languages.customize.Fallback.other             "Other (including Western European)">
<!-- LOCALIZATION NOTE  Character Encoding Preferences Dialog: Do NOT localize the terms "en-bz, ar-jo"  -->
Attached patch Proposed patchSplinter Review
We need this RSN because there are now three affected menulists.

To reduce duplication I moved the new labels into a new DTD file.

The weird convolutions in the character encoding preference pane are to work around the fact that the preference is a localised string, and there's no easy way for onsynctopreference to indicate that the pref should be reset to default. If you don't like it like that, I can remove the "Default" option and just let the default value select the appropriate menuitem from the list.
Assignee: ewong → neil
Attachment #8428419 - Flags: review?(iann_bugzilla)
(In reply to neil@parkwaycc.co.uk from comment #12)
> Hmm, looks like this isn't the only place affected;
> pref-character_encoding.xul is affected, as is folderProps.xul and
> am-server.xul (but they're in mailnews...) and EditorSaveAsCharset.xul too.

So which bug will the changes to EditorSaveAsCharset.xul be in? Would it be better for the dtd not to be under pref so it can be used in editor too?
Needs to be some co-ordination between this and bug 1003716
Flags: needinfo?(neil)
(In reply to Ian Neal from comment #15)
> (In reply to comment #12)
> > Hmm, looks like this isn't the only place affected;
> > pref-character_encoding.xul is affected, as is folderProps.xul and
> > am-server.xul (but they're in mailnews...) and EditorSaveAsCharset.xul too.
> 
> So which bug will the changes to EditorSaveAsCharset.xul be in? Would it be
> better for the dtd not to be under pref so it can be used in editor too?

These strings wouldn't make sense for Editor. The CharsetMenu strings make slightly less worse sense so I was going to use those. (But I forgot to actually write a patch. D'oh!)

> Needs to be some co-ordination between this and bug 1003716

Depending on where bug 1003716 puts its strings, I guess we could share them for the mail pref pane, although it would be a little untidy for the browser pane, in which case I would prefer to go back to (an adjusted) attachment 8340852 [details] [diff] [review].
Flags: needinfo?(neil)
Depends on: 1021696
No longer depends on: 1021696
Sorry because of the bugmail about bug 1021696. I have just created it but it seems that it duplicates this one.
Attachment #8428419 - Flags: review?(iann_bugzilla) → review+
See Also: → 1021900
Bug 910192 which this depends on landed on Mozilla28 (eq SeaMonkey 2.25) should we get this landed on all branches?
Flags: needinfo?(neil)
Flags: needinfo?(iann_bugzilla)
Pushed comm-central changeset 62ff11f01119.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
Attached patch Aurora patchSplinter Review
l10n is a pain as usual, so I had to improvise and use the old charset titles instead, except for the default item, which is a complete hack, particularly the way of guessing the browser default encoding.
Attachment #8452020 - Flags: review?(iann_bugzilla)
Flags: needinfo?(neil)
Attachment #8452020 - Flags: review?(iann_bugzilla) → review+
Flags: needinfo?(iann_bugzilla)
Comment on attachment 8452020 [details] [diff] [review]
Aurora patch

[Approval Request Comment]
Regression caused by (bug #): 910192
User impact if declined: Broken UI in Languages and Composition pref panes.
Testing completed (on m-c, etc.): Landed on c-c in time for aurora uplift.
Risk to taking this patch (and alternatives if risky):
String changes made by this patch: None, unfortunately, because it needed some.

In case anyone's wondering, this patch as it stands is unsuitable for comm-release because charsetTitles moved between Gecko 31 and 32.
Attachment #8452020 - Flags: approval-comm-beta?
Comment on attachment 8452020 [details] [diff] [review]
Aurora patch

a=me
Attachment #8452020 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.