260427 - non-ASCII characters displayed wrong

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Peter Weilbacher]

The patch from bug 256383 _on OS/2_ has the effect that umlaut characters at
least in the chrome, e.g. menu entries in the file menu in the German language
pack, do not display as the characters that they should, i.e. "Datei öffnen" is
displayed as "Datei ÷ffnen" (I hope that these come out right now, otherwise see
screenshot that I will attach shortly).

At least in the standard configuration this happens, if the Innotek Font Engine
is installed and activated for Mozilla everything is OK.

This was seen in a 1.7 branch build that I created, but is probably there in the
trunk as well, I just don't find a language pack to test there...

Comment 1

[Peter Weilbacher]

Attachment: A screenshot showing the problem

Comment 2

[Jungshik Shin]

What do you have in config.sys for country and codepage? 

Comment 3

[Peter Weilbacher]

I have CODEPAGE=850,1004, so 850 is active. Other people where this is happening
say that 850,437 does not work, but if 1004 is the active codepage then it is
supposed to work fine, but I didn't try this myself yet. It does not seem to
depend on country settings (I have 044 but the others probably have 049).

Comment 4

[Mike Kaply [:mkaply]]

jshin, in the past, platform nsFontMetrics was never told that x-unicode is the
langgroup.

Did your patch change this?

Comment 5

[Mike Kaply [:mkaply]]

Strange. I just did a 1.7 build from the branch and installed the German
langpack, and it works fine for me.

I disabled Innotek font engine just in case, and it still works fine.

My active codepage is 850

Comment 6

[Peter Weilbacher]

This might be an optimization issue again. The 1.7.3 builds I made available
were actually done with --enable-optimize="-O2" instead of just
--enable-optimize. I will test this.

Comment 7

[Peter Weilbacher]

No, that was not it. I made completely fresh builds from both the 1.7 branch and
the trunk, the former both optimized and unoptimized, and I see this problem
every time when the Font Engine is switched off. I also see it when I do a "chcp
1004" on the command line before I start mozilla.exe from there.

Can I get your build from somewhere to see how it behaves here?

Comment 8

[Jungshik Shin]

(In reply to comment #4)
> jshin, in the past, platform nsFontMetrics was never told that x-unicode is the
> langgroup.
> 
> Did your patch change this?

  Sorry for the late reply.
  My patch for bug 256383 meant to change that, but due to bug 91190 (about
which I forgot in bug 256383), my change in bug 256383 doesn't reach
nsFontMetrics*. What effectively got there is the langGroup corresponding to the
current locale. In this case, with or without my patch, it should geet x-western
and everything should work fine, I think. 

  BTW, would it be a bad idea to make the Innotek font engine a requirement(at
least a recommendation) for  Mozilla on OS/2? (if it's freely available).  

Comment 9

[Mike Kaply [:mkaply]]

the chcp 1004 is a red herring. I don't think that should have worked.

I did a debug build. I'll build release on Monday.

Comment 10

[Jungshik Shin]

(In reply to comment #8)
> (In reply to comment #4)
> > jshin, in the past, platform nsFontMetrics was never told that x-unicode 
> > is the langgroup. Did your patch change this?

> What effectively got there is the langGroup corresponding to the
> current locale. In this case, with or without my patch, it should geet x-western
> and everything should work fine, I think. 

  I was not entirely correct about this. I missed one of two places where
LookupLanguage is called. 

http://lxr.mozilla.org/seamonkey/source/gfx/src/os2/nsFontMetricsOS2.cpp#227

calls |GetLocaleLanguageGroup()| which calls |LookupLanguage| (that I changed in
bug 256383). |GetLocaleLanguageGroup()| passes to |LookupLanguage| the xp locale
 corresponding to the current application locale which is obtained, in case of
OS/2, at

http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLocaleService.cpp#238

Therefore, unless what's passed to |LookupLanguage| is something unknown to
Mozilla, 'x-unicode' won't reach nsFontMetricsOS2. 

Can you see what value |xplocale| holds at 
 http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLocaleService.cpp#281
?  It seems like it is not in the form expected by |LookupLanguage| which leads
it to fall back to 'x-unicode'. |LookupLanguage| expects it to have the form of
 'll_RR', but |nsOS2Locale::GetXPLocale()| returns 'xplocale' in a different
form. See

 http://lxr.mozilla.org/seamonkey/source/intl/locale/src/os2/nsOS2Locale.cpp#113

I think I have to fix |LookupLanguage| because not just nsOS2Locale but also
other  implementations of nsILocale returns 'xplocale' in a form different from
'll_RR'. (see 
http://lxr.mozilla.org/seamonkey/ident?i=GetXPLocale )

Comment 11

[Peter Weilbacher]

OK, the difference to Mike's system is most likely the LANG variable. I have it
set to en_GB_EURO (I know, doesn't make much sense until GB uses the Euro), the
others seeing this most likely have de_DE_EURO, and this is probably what
LookupLanguage() gets, the debug build hasn't finished yet to check this. If I
change it to en_GB before starting Mozilla then it works correctly.

Comment 12

[Peter Weilbacher]

Attachment: Use separator variable in nsOS2Locale::ParseLocaleString

I think this gets the OS/2 locale into the right format. I just made use of the
separator char given to nsOS2Locale::ParseLocaleString, so that locales like
en_GB_EURO (the OS/2 standard) are also recognized, in addition to e.g.
en_GB.EURO, if ParseLocaleString is called with '_'.
This fixes the character display bug for me on OS/2, perhaps there is a
similarly simple solution for MacOSX?

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 160847 [details] [diff] [review]
Use separator variable in nsOS2Locale::ParseLocaleString

Nice.

r=mkaply, sr=blizzard (platform specific)
a=mkaply for everywher.e

Comment 14

[Jungshik Shin]

(In reply to comment #10)
http://lxr.mozilla.org/seamonkey/source/intl/locale/src/os2/nsOS2Locale.cpp#113
> 
> I think I have to fix |LookupLanguage| because not just nsOS2Locale but also
> other  implementations of nsILocale returns 'xplocale' in a form different from
> 'll_RR'. (see 
> http://lxr.mozilla.org/seamonkey/ident?i=GetXPLocale )

I was wrong again. Only OS/2 has the problem and other platforms are all right.
 The OS/2 problem was fixed by the patch uploaded. here so that there's no need
to touch LookupLanguage.

BTW, ParseLocale for OS/2 has a dubious code here (line 183 through 187) that
perhaps we should get rid of while we're at it. 
http://lxr.mozilla.org/seamonkey/source/intl/locale/src/os2/nsOS2Locale.cpp#183

Comment 15

[Peter Weilbacher]

Attachment: new patch that removes the strange case and honors extra's size

Yes, I noticed that, too. It seems to be a way to make use of xx.Extra like
locale settings, and divides those strangely in to "xx", "Ex", and "Extra". I
assumed there was a reason for this, but if there is not then this new patch
should be used (I am not yet obsoleting the first one because I am not sure).

Thinking a bit further, it might be a good idea to use strncpy instead of
strcpy and match that to the length of extra, in case someone has some really
long locale strings, although it never seems to reach ParseLocaleString in that
case.

Comment 16

[Jungshik Shin]

As you wrote, the code in question seems to be for 'll.Extra'. In that case, the
following would be better than removing it altogether, wouldn't it? 

-    country[0]=locale_string[3];
-    country[1]=locale_string[4];
-    country[2]='\0';
+    strcpy(extra, &locale_string[3]);

Comment 17

[Jungshik Shin]

Sorry I haven't read your comment #15 to the end. So, there's a possibility of
buffer overrun. I also missed 'strcpy'  for the'len >= 4' case. I'm wondering if
locale names like 'en.Extra' is actually used in OS/2. In that case, we may just
keep 'len >= 4' case but use strncpy there as well. 

Comment 18

[Peter Weilbacher]

I have tried every LANG setting I could think of to provoke the 'len >= 4' case.
But in all likely cases like en.EURO it never even entered the method in
question. I didn't find any programming documentation on this, so I am waiting
to see if Mike remembers why this was done in the first place.

Comment 19

[Mike Kaply [:mkaply]]

I have no idea why that code is the way it is. It's lost in history.

Comment 20

[Mike Kaply [:mkaply]]

The ParseLocaleString concept was mostly lifted from nsPosixLocale.cpp. We did a
lot of porting of Unix stuff to get the OS/2 stuff working.

jshin, if a locale has the form xx_XX.Extra, is the extra used anywhere?

Comment 21

[Mike Kaply [:mkaply]]

Maybe we should be using the code from nsPosixLocale. It seems a little more robust.

http://lxr.mozilla.org/seamonkey/source/intl/locale/src/unix/nsPosixLocale.cpp#139

Comment 22

[Peter Weilbacher]

Attachment: Use ParseLocaleString from nsPosixLocale in nsOS2PosixLocale

(In reply to comment #21)
> Maybe we should be using the code from nsPosixLocale. It seems a little more
robust.
> 
>
http://lxr.mozilla.org/seamonkey/source/intl/locale/src/unix/nsPosixLocale.cpp#139


OK, I copied that method into nsOS2Locale.cpp.

But then en_gb_euro appears in LookupLanguage again and we are back where this
bug started because the separator variable is not used. Probably because some
Posix standard says '_' is not a valid separator between country and extra
code. But on OS/2 it obviously is... So, I updated that code to use the
separator. But what about the difference between ll-CC.extra and ll-CC@extra,
is that something in the Posix standard? Both are handled slightly differently
and because the '@' case mentions "euro" that's where I added the separator
(that is passed as '_')... Seems to work on my machine with LANG=en_GB_EURO at
least.

Btw, I found out that intl/locale/src/os2/nsILocaleOS2.h seems to be obsolete
and should probably be deleted. With lxr I cannot find any reference and intl
compiles without it. I tried "cvs diff -N" but that does not add anything in
the patch to remove it...

Comment 23

[Mike Kaply [:mkaply]]

I found this page:

http://www.tldp.org/HOWTO/Euro-Char-Support/x45.html

But I can't find any description of how a locale string should look.

Comment 24

[Peter Weilbacher]

Comment on attachment 161201 [details] [diff] [review]
Use ParseLocaleString from nsPosixLocale in nsOS2PosixLocale

That page only refers to glibc locales. OS/2 really seems to allow ll_CC_Extra
instead of ll_CC@Extra. And after a hint from comp.os.os2.programmer.misc I
tried the UniMapCtryToLocale API which only outputs ll_CC strings with the
exception of fi_FI_E (not _EURO)!

In any case, the patch using the Posix strings with the additional separator
seems to work. Asking for reviews and approvals again.

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 161201 [details] [diff] [review]
Use ParseLocaleString from nsPosixLocale in nsOS2PosixLocale

r=mkaply
sr=blizzard (platform specific code)
a=mkaply for the branches.

I'll get this in today.

Comment 26

[Mike Kaply [:mkaply]]

Fix checked in.

Thanks!

Comment 27

[Peter Weilbacher]

Did you think of removing intl/locale/src/os2/nsILocaleOS2.h from CVS, too, if
you could verify that it is unused?