Closed Bug 252475 Opened 20 years ago Closed 20 years ago

nsLocale is broken on Mac OS X / GetSystemLocale always return en-US

Categories

(Core :: Internationalization, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: asaf, Assigned: jhpedemonte)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, intl)

Attachments

(3 files, 4 obsolete files)

When trying to get the system locale (dates, currency etc.), GetSystemLocale
always return "en-US", even when I have set he-IL in System
Preferences->International->Formats.

To reproduce, try the following JS code (It does work properly on win32):
----
  var localService =
Components.classes["@mozilla.org/intl/nslocaleservice;1"].getService(Components.interfaces.nsILocaleService);
  var systemLocale =
localService.getSystemLocale().getCategory("NSILOCALE_MESSAGES");
       alert("NSILOCALE_MESSAGES: " + systemLocale);
----
(I'll attach an XPI to test the above code soon.)

Also notice I have the same result from each category here:
http://lxr.mozilla.org/seamonkey/source/intl/locale/idl/nsILocale.idl#41
This adds "What's my locale?" at the end of the View menu.
This must be the cause of quite a few bugs about Date and Time format: bug
#231280 and bug #233631 are just the two to mention. 
Blocks: 103669
(In reply to comment #2)
> This must be the cause of quite a few bugs about Date and Time format: bug
> #231280 and bug #233631 are just the two to mention. 

So how did they get fixed..!? Maybe this is for some languages only? (bug 120401
comes in mind).
(In reply to comment #3)
> (In reply to comment #2)
> > This must be the cause of quite a few bugs about Date and Time format: bug
> > #231280 and bug #233631 are just the two to mention. 
> 
> So how did they get fixed..!? Maybe this is for some languages only? (bug 120401
> comes in mind).

I think it's for some OS X distributions only. I.e. users with Japanese OS X
didn't seem to have the problem with those bugs. 

I fixed it by excluding the explicit instruction of what locale must be used. 

What practical problems do you have due to this bug? 
> 
> I think it's for some OS X distributions only. I.e. users with Japanese OS X
> didn't seem to have the problem with those bugs. 
> 
I use the primary dist.


> What practical problems do you have due to this bug? 

There is work being done for bug 85420. We are going to enable the additional UI
there (see http://bidiui.mozdev.org for what UI) only for some specific system
locales, this works properly on moz/win, but on OS X, I always get "en-US".

Can you please check if you can port your changes for Hebrew & Arabic?
(In reply to comment #5)
> Can you please check if you can port your changes for Hebrew & Arabic?

My changes were on the call-sites only. Have nothing to port, sorry. :-(
OK, here's what I've found:

If you change the language/format in the International pref panel, then you will
need to logout and back in again for the Script Manager to register the changes
(the script manager calls are at
http://lxr.mozilla.org/mozilla/source/intl/locale/src/nsLocaleService.cpp#283).
 However, even this doesn't seem to work perfectly.  For example, if I keep the
Language as 'English', but set the Format to 'United Kingdom', I still get back
the same values as when Format was set to 'United States'.  But I get the
correct locale is I set it to 'Español'/'Argentina'.

So I went searching for the correct way to get the user's selected locale. 
According to much of the discussion on the carbon-dev mailing list (and we
aren't the only ones having this problem), there was never really an official
way to the user's locale.  From an Apple manager:
 > 1. If you are running on a pre-10.3 system (determined at runtime),  
use
 > the old WorldScript calls (Script Manager, Text Utilities,
 > Date/Time/Measurement Utilities) and convert the results to Unicode.
 > 2. If you are running on 10.3 or later, then use the new APIs
 > (CFDateTimeFormatter, CFNumberFormatter, etc.). You will need to
 > weak-link these so your app loads on pre-10.3 systems.

So it looks like the Script Manager calls will need to stay for 10.1/10.2, but
we need to update the code properly for 10.3.  I'll look at putting together a
patch.
Hmm, it looks like it uglier than I thought.  It seems that the Script Manager
calls return values based only on the selected Language in the International
pref pane;  changing Region in the Format tab does nothing.  So every region
returns 'en_US' when 'English' is set as the Language;  they all return 'it_IT'
when Italian is set, etc.  I guess for the most part, the end user won't notice
anything wrong when the Language is set correctly, but it's still not very accurate.
Thanks for working on that!

Javier, users can't always choose associated lang to their prefered locale
(That's becuase Mac OS X has a lot of issues with preferd language, such as
problems with the Software Update app).

At least on 10.3+, we need to use the new API (see comment 7).
Attached patch patch (diff -w) (obsolete) — Splinter Review
This seems to work pretty well.  CFLocaleGetIdentifier returns a string that is
very close to what Mozilla wants;  just need to change the underscore to a
dash.  The two CFLocale functions need to be weak-linked.  I think that's what
I did, but I can't check since I don't have 10.2 to test this on.
Javier, it works fine, Thanks!

(See screenshot (i added "NSLOCALE_MESSAGES: ")).
Now, there is a problem which "PageInfo" etc. does not work in a Japanese
environment.(bug 239535)

Does anything improve by this correction?
Javier, can you request for r/sr?
crot0: No, this patch doesn't seem to make any difference with regards to bug
239535.  But there are some other places in the code that use the Script
Manager;  these may be at fault.  I'll investigate when I get the change.
Attachment #154517 - Flags: review?(sfraser)
+  if ((err == noErr) && (systemVersion >= 0x00001030))

is there a version# of foundation we can check against instead?

+    CFStringRef userLocaleStr = ::CFLocaleGetIdentifier(userLocaleRef);
+    ::CFRetain(userLocaleStr);
...
+    ::CFRelease(userLocaleStr);

I don't get this. Why retain/release the string? Also, I was under the
impression that anything with "Copy" in the name returned a retained instance
(that we're responsible for releasing). That's how it generally goes in appKit,
and since it's all the same under the hood...

+      char* p = strchr(buffer, '_');
+      if (p)
+        *p = '-';

are we guaranteed that CFStringGetCString will null terminate the string even if
it fills up the buffer?
The documentation for CFLocaleGetIdentifier says: "You are responsible for
retaining and releasing it as needed."  So I figured I would retain it before
messing around with it, just to be safe.

I have another patch that addresses the other issues.
Attached patch patch v1.1 (obsolete) — Splinter Review
Using a macro for the system version.  Getting the unicode chars from
CFStringRef rather than converting to a C string; makes the code simpler.
Attachment #154517 - Attachment is obsolete: true
Attachment #154517 - Flags: review?(sfraser)
Comment on attachment 154783 [details] [diff] [review]
patch v1.1

+  if ((err == noErr) && (systemVersion >= MAC_OS_X_VERSION_10_3))

hm... Maybe I'm missing something, but is this macro defined in <= 10.2? If it
is not, this is not even going to compile, isn't it? :-)
Attachment #154783 - Flags: review-
No, I think it's all right.  Even the 10.1.5 SDK defines macros for version 10.0
through 10.5, so we should be alright.
Attachment #154783 - Flags: review-
Actually, can someone with MacOSX 10.2 with the developer tools installed check
the file
/Developer/SDKs/MacOSX10.2.8.sdk/Developer/Headers/CFMCarbon/AvailabilityMacros.h
and make sure that, at the top of the file, "MAC_OS_X_VERSION_10_3" is defined.
FWIW, the SDKs don't exist on 10.2-based dev systems. Only on panther/XCode. So,
no, that won't be defined on a 10.2 machine.
Attached patch patch v1.2 (obsolete) — Splinter Review
Removed system version macro.
Attachment #154783 - Attachment is obsolete: true
Attachment #154787 - Flags: review?(pinkerton)
Comment on attachment 154787 [details] [diff] [review]
patch v1.2

>Index: nsLocaleService.cpp

>+// These functions are only available in 10.3+, so weak-link to them.
>+extern CFLocaleRef CFLocaleCopyCurrent(void) __attribute__((weak_import));
>+extern CFStringRef CFLocaleGetIdentifier(CFLocaleRef locale) __attribute__((weak_import));



>+  long systemVersion;
>+  OSErr err = ::Gestalt(gestaltSystemVersion, &systemVersion);
>+  if ((err == noErr) && (systemVersion >= 0x00001030))


How about using |CFBundleGetFunctionPointerForName| as in 

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&f
ile=nsLocalFileOSX.cpp&branch=&root=/cvsroot&subdir=mozilla/xpcom/io&command=DI
FF_FRAMESET&rev1=1.21&rev2=1.22  ? Perhaps, that's not such a good idea here...



>+    // Get string representation of locale
>+    int size = ::CFStringGetLength(userLocaleStr);
>+    UniChar *buffer = (UniChar*) PR_Malloc(size * sizeof(UniChar) + 1);
>+    CFRange range = ::CFRangeMake(0, size);
>+    ::CFStringGetCharacters(userLocaleStr, range, buffer);
>+    buffer[size] = 0;
>+
>+    if (buffer)
>+    {
>+      // Convert the locale string to the format that Mozilla expects
>+      nsAutoString xpLocale(buffer);
>+      xpLocale.ReplaceChar('_', '-');
>+
>+      nsresult rv = NewLocale(xpLocale, getter_AddRefs(mSystemLocale));
>+      if (NS_SUCCEEDED(rv)) {
>+        mApplicationLocale = mSystemLocale;
>+      }
>+
>+      PR_Free(buffer);
>+    }

You may want to use 'nsAutoBuffer' here to avoid malloc/free. If not, three
lines following |PR_Malloc| should go inside |if (buffer)|, shouldn't it?
Attached patch patch v1.3 (obsolete) — Splinter Review
I think I like using 'weak_import' rather than
|CFBundleGetFunctionPointerForName|;  the code looks cleaner to me.  But if you
think |CFBundleGetFunctionPointerForName| is better, I can create a patch for
that too.

Thanks for pointing me to nsAutoBuffer.  Had forgotten about that class.
Attachment #154787 - Attachment is obsolete: true
Attachment #154787 - Flags: review?(pinkerton)
(In reply to comment #25)
> Created an attachment (id=154810)
> patch v1.3
> 
> I think I like using 'weak_import' rather than
> |CFBundleGetFunctionPointerForName|;  the code looks cleaner to me. 

Yeah, it seems so. 

 
> Thanks for pointing me to nsAutoBuffer.  Had forgotten about that class.

No problem. BTW, you don't need to include "prmem.h" any more.
Comment on attachment 154810 [details] [diff] [review]
patch v1.3

I'll remove the prmem.h include when I checkin.
Attachment #154810 - Flags: superreview?(pinkerton)
Attachment #154810 - Flags: review?(jshin)
Attachment #154810 - Flags: superreview?(pinkerton) → superreview?(sfraser)
(In reply to comment #27)
> (From update of attachment 154810 [details] [diff] [review])
> I'll remove the prmem.h include when I checkin.
> 

A cleaner way to compile using weak link, is using AvailabilityMacros.

Instead of:

// These functions are only available in 10.3+, so weak-link to them.
extern CFLocaleRef CFLocaleCopyCurrent(void) __attribute__((weak_import));
extern CFStringRef CFLocaleGetIdentifier(CFLocaleRef locale)
__attribute__((weak_import));

Use:
#include <AvailabilityMacros.h>
extern CFLocaleRef CFLocaleCopyCurrent(void)
    AVAILABLE_MAC_OS_X_VERSION_10_3_AND_LATER;
extern CFStringRef CFLocaleGetIdentifier(CFLocaleRef locale)
    AVAILABLE_MAC_OS_X_VERSION_10_3_AND_LATER;

Just like its done on most Apple headers.

I did not check with Mozilla, but used these few month ago.

About confirming that you did get a weak link, you dont have to test on 10.2,
just run nm on your executable: 

% nm -mg |grep frameworkname
(undefined) weak external _Symbol (from frameworkname)
...

More info: 
 * http://developer.apple.com/technotes/tn2002/tn2064.html
 * /usr/include/AvailabilityMacros.h

Hope it helps.
Nir, thanks for the link.  That looks nicer.
Attached patch patch v1.4Splinter Review
So it turns out that weak-linking is only supported in 10.2+, so the last patch
would break on 10.1.  This new patch uses jshin's suggestion of manually
loading those functions.
Attachment #154810 - Attachment is obsolete: true
Attachment #155212 - Flags: superreview?(sfraser)
Attachment #155212 - Flags: review?(jshin)
Attachment #154810 - Flags: superreview?(sfraser)
Attachment #154810 - Flags: review?(jshin)
Comment on attachment 155212 [details] [diff] [review]
patch v1.4

r=jshin
Attachment #155212 - Flags: review?(jshin) → review+
Jshin, any chance you can also sr it?
Oh, i was sure your'e there :-)
maybe blizzard?
Attachment #155212 - Flags: superreview?(sfraser) → superreview?(blizzard)
Attachment #155212 - Flags: superreview?(blizzard) → superreview+
Patch checked in to trunk (with change to use function pointers rather than
actual functions!).  Now for aviary.
Flags: blocking-aviary1.0mac?
Comment on attachment 155212 [details] [diff] [review]
patch v1.4

requesting approvals
Attachment #155212 - Flags: approval1.7.3?
Attachment #155212 - Flags: approval-aviary?
Whiteboard: [fixed on trunk], needed-aviary1.0
The patch didn't build on the tinderbox, which has 10.2 installed (I think).  So
I made these changes to the file in order to get it to build:

 #ifdef XP_MACOSX
+#if !defined(__COREFOUNDATION_CFLOCALE__)
+typedef void* CFLocaleRef;
+#endif
 typedef CFLocaleRef (*fpCFLocaleCopyCurrent_type) (void);
 typedef CFStringRef (*fpCFLocaleGetIdentifier_type) (CFLocaleRef);
 #endif

|__COREFOUNDATION_CFLOCALE__| is only defined in the 10.3 SDK.  I tested this on
my 10.3 machine by changing all |CFLocaleRef| instances to |void*|, and it
worked just fine.
Comment on attachment 155212 [details] [diff] [review]
patch v1.4

a=mkaply for 1.7 and aviary
Attachment #155212 - Flags: approval1.7.3?
Attachment #155212 - Flags: approval1.7.3+
Attachment #155212 - Flags: approval-aviary?
Attachment #155212 - Flags: approval-aviary+
Whiteboard: [fixed on trunk], needed-aviary1.0 → [fixed on trunk], fixed-aviary1.0
javier: please use resolution:fixed for trunk.
Keywords: fixed-aviary1.0
Whiteboard: [fixed on trunk], fixed-aviary1.0 → [fixed on trunk]
Checked in everywhere.  ->FIXED
Status: NEW → RESOLVED
Closed: 20 years ago
Keywords: fixed1.7.x
Resolution: --- → FIXED
Thanks.
Status: RESOLVED → VERIFIED
Flags: blocking-aviary1.0mac?
Whiteboard: [fixed on trunk]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: