170514 - Language name representation not localisable

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, defect)

Description

[Karl Ove Hufthammer]

Description of bug
------------------

The formatting of the language names in 'Preferences | Browser | Languages | Add' is not localisable, which is a problem for some languages (e.g. Norwegian Nynorsk). The list is formatted this way:

LanguageName/CountryName [LanguageCode]

In Norwegian Nynorsk, the '/' character should never be used this way. Instead the list should look like this:

LanguageName (Countryname) [LanguageCode]

or

LanguageName – Countryname (LanguageCode)

I'm sure this is a problem for other languages too.

How to fix
----------

Make the formatting of the language entries localisable, using variables. E.g.:

%lang%/%country% [%code%]

For Norwegian Nynorsk, we can then localise this to:

%lang% (%country%) [%code%]

Comment 1

[rchen]

-> naoki.

Comment 2

[Karl Ove Hufthammer]

Note that language strings are also used other places in the UI, e.g. for
displaying the value of 'hreflang' attribute ('target language') when
right-clicking and choosing 'Properties' on links. The language representation
should be the same here. (Oddly enough, it seems to default to 'LanguageName
(CountryName)'.)

Comment 3

[Kasumi]

I can't reproduce.
Please take a look attachment.

Comment 4

[Kasumi]

Attachment: 2002-10-18-08-1.0

Comment 5

[Kasumi]

I tested on 2002-10-18-08-1.0 on Win XP Pro. JA

Comment 6

[Karl Ove Hufthammer]

Reopening.

I have no idea what your attachment are meant to show.

Comment 7

[Kasumi]

It means no '/' is in Preferences | Browser | Languages | Add.

Comment 8

[Karl Ove Hufthammer]

> It means no '/' is in Preferences | Browser | Languages | Add.

Yes, there is. It's clearly visible in the screenshot for 'Portugese/Brazil' and
'Macedonian/Macedonia, F.Y.R. of'.

It's currently not possible to localise how this is displayed.

Comment 9

[Marcel Gosselin]

Attachment: Patch - enables localization of language name representations

This patch enables the localization of language name representations.
You can change the symbol '/' between language and region names.
You can also switch their order, it might be better in some languages to write 

e.g. "Brazil Portuguese" instead of "Portuguese/Brasil".

Comment 10

[Marcel Gosselin]

Attachment: Adds the language-region code to the localisation

I modified the patch to include the language-region code part of the string in
the
localization. It makes it possible to set to 
"LanguageName – Countryname (LanguageCode)"
as specified in the original bug report.

Comment 11

[Marcel Gosselin]

Attachment: Proposed patch - Adds hreflang localisation requested in comment#2

This patch includes localisation of:
1- The languages preferences language representation
2- The link properties href attribute (Target language)

Karl (or anybody), can you review this patch?

Comment 12

[Marcel Gosselin]

Comment on attachment 127933 [details] [diff] [review]
Proposed patch - Adds hreflang localisation requested in comment#2

Requesting review on attachment #127933 [details] [diff] [review]

Comment 13

[jag (Peter Annema)]

Comment on attachment 127933 [details] [diff] [review]
Proposed patch - Adds hreflang localisation requested in comment#2

>Index: xpfe/components/prefwindow/resources/content/pref-languages.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-languages.js,v
>retrieving revision 1.31
>diff -u -r1.31 pref-languages.js
>--- xpfe/components/prefwindow/resources/content/pref-languages.js	2 Jul 2003 21:59:22 -0000	1.31
>+++ xpfe/components/prefwindow/resources/content/pref-languages.js	15 Jul 2003 03:28:40 -0000
>@@ -154,28 +154,39 @@
> 
>              if (stringLangRegion[0]) {
>                  var tit;
>+                 var language;
>+                 var region;
>+                 var use_region_format = false;
>+
>                  try {
>-                    tit = languagesBundle.GetStringFromName(stringLangRegion[0]);
>+                   language =
>+                     languagesBundle.GetStringFromName(stringLangRegion[0]);
>                  }
> 
>                  catch (ex) {
>-                    tit = "";
>+                   language = "";
>                  }
> 
> 
>                  if (stringLangRegion.length > 1) {
> 
>                    try {
>-                    tit += "/" + regionsBundle.GetStringFromName(stringLangRegion[1]);
>+                     region =
>+                       regionsBundle.GetStringFromName(stringLangRegion[1]);
>+                     use_region_format = true;
>                    }
> 
>                    catch (ex) {
>-          tit += "["+str+"]";
>                    }
>-
>-                 } //if region
>-
>-                 tit = tit + "  [" + str + "]";
>+                }
>+            

Nit: It looks like you need to indent the code below by 4 more spaces.
Indentation in all this (old) code is rather yucky.

>+             if (use_region_format) {
>+                tit = prefLangBundle.formatStringFromName(
>+                       "languageRegionCodeFormat", [language, region, str], 3);
>+             } else {
>+                tit = prefLangBundle.formatStringFromName(
>+                       "languageCodeFormat", [language, str], 2);
>+             }
> 
>              } //if language
> 

>Index: xpfe/browser/resources/content/metadata.js
>===================================================================
>@@ -557,17 +561,24 @@
>             {
>                 // We don't add it on to the result immediately
>                 // because we want to get the spacing right.
>-                tokens[0] = gRegionBundle.getString(tokens[0].toLowerCase());
>+                region = gRegionBundle.getString(tokens[0].toLowerCase());

At this point do we not care about tokens[1..n-1]? The spec seems to suggest
otherwise. The old code joined all of them back together again with spaces, the
new code only does it in the catch case.

>             }
>             catch (e) 
>             {
>                 // Region not present in region bundle
>+                region = tokens.join(" ");
>             }
> 
>-            result += " (" + tokens.join(" ") + ")";
>+            is_region_set = true;
>         }
>     }

>Index: xpfe/browser/resources/locale/en-US/metadata.properties
>===================================================================
>@@ -10,3 +10,5 @@
> imageSizeUnknown=Unknown (not cached)
> imageWidth=%Spx
> imageHeight=%Spx
>+# LOCALIZATION NOTE: first %S is language, second is region
>+languageRegionFormat=%S (%S)

You could use %1$S and %2$S here like you did in the other .properties file to
make your comment slightly better.

Comment 14

[Marcel Gosselin]

Attachment: Updated patch

Includes modifications asked for in comment#13.

In pref-languages.js, the only modifications added from previous attachment is
the indentation which has been completeley remade in the 
ReadAvailableLanguages() function.

Comment 15

[jag (Peter Annema)]

Comment on attachment 128210 [details] [diff] [review]
Updated patch

sr=jag

Comment 16

[Marcel Gosselin]

Comment on attachment 128210 [details] [diff] [review]
Updated patch

Requesting approval for 1.5b.
I think it is important to have this bug fixed (as any l12y bug).
I don't think this could be breaking anything since it is almost only GUI.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 128210 [details] [diff] [review]
Updated patch

fixing flag to make it a request rather than an approval

Comment 18

[Marcel Gosselin]

Sorry for the mistake about the 1.5b+ instead of 1.5b?, I should learn to verify
before clicking on submit...

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 128210 [details] [diff] [review]
Updated patch

a=asa (on behalf of drivers) for checkin to 1.5beta.  Time is short, please
land ASAP. Thanks.

Comment 20

[Benjamin Smedberg]

.

Comment 21

[Benjamin Smedberg]

checked-in on behalf of Marcel

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

Nit: why the extra whitespace in this declaration in pref-languages.js?

+  availLanguageDict   = new Array();

Comment 23

[Marcel Gosselin]

I simply did not see it when doing the patch.
This line was already like this, I just corrected the indentation.
There is also a missing space before the 0 in
+  var i =0;