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...
What do you have in config.sys for country and codepage?
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).
jshin, in the past, platform nsFontMetrics was never told that x-unicode is the langgroup. Did your patch change this?
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
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.
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?
(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).
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.
(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 )
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.
Created attachment 160847 [details] [diff] [review] 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 on attachment 160847 [details] [diff] [review] Use separator variable in nsOS2Locale::ParseLocaleString Nice. r=mkaply, sr=blizzard (platform specific) a=mkaply for everywher.e
(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
Created attachment 160923 [details] [diff] [review] 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.
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=locale_string; - country=locale_string; - country='\0'; + strcpy(extra, &locale_string);
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.
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.
I have no idea why that code is the way it is. It's lost in history.
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?
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
Created attachment 161201 [details] [diff] [review] 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...
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 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 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.
Fix checked in. Thanks!
Did you think of removing intl/locale/src/os2/nsILocaleOS2.h from CVS, too, if you could verify that it is unused?