233631 - (osxtime) GetApplicationLocale should not be used with FormatTime methods in order to support user's customisation (OS X is affected)

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Constantine A. Murenin]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-GB; rv:1.6) Gecko/20040113

Mozilla does not honour the long date and time format, defined by the OS X. In
the test case, I tried to set the Date format to the one used in any other
country, and on the screenshot I used French with some modifications. The OS X
itself is en-US and version is 10.2.6. 

Reproducible: Always
Steps to Reproduce:
1. Open System Prefs, choose International, change Date format to French (just
as an example, and assuming that your default locale is not French). Change Time
format to French as well. 
2. One Page Info in Mozilla. 
Actual Results:  
One sees the wrong format of the long date (in Page Info, for example).

Expected Results:  
One sees the right format of the long date (in Page Info, for example).

Comment 1

[Constantine A. Murenin]

Attachment: Long Date format in French with OS X 10.2.6

Sorry for JPEG, the PNG file was too large...

Comment 2

[Jungshik Shin]

This is an interesting bug. Modern OS' offer a way to customize date and time
format (among others) regardless of the locale.  Mozilla needs to obtain that
informaion from the OS.  I'm wondering what mozilla has to do in the following case:

  1. the os locale is set to en-US
  2. 'fr-CA' language/region pack is active
  3. the data time format configured in the OS is Japanese 

Which should take the highest priority? 

Comment 3

[Constantine A. Murenin]

(In reply to comment #2)
> This is an interesting bug. Modern OS' offer a way to customize date and time
> format (among others) regardless of the locale.  Mozilla needs to obtain that

...

> Which should take the highest priority? 

I think the customisation must have the highest priority. For instance, I am
Russian, I live in the US, I use en-GB locale, and I like the ISO time notation.
:-) Weird, huh? 

BTW, not only OS X has problems with long date and time format --- apparently,
UNIX builds do not have any kind of long date and time. Moreover, the short one
is based on the locale, rather than the user settings of the desktop (KDE, as
example). 

Comment 4

[Jungshik Shin]

Mozilla doesn't have any support for KDE interoperability at the moment although
it has some for Gnome2. Anyway, that has to be a separate bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

See bug 231280 comment 21

Comment 6

[Constantine A. Murenin]

Attachment: Camino honours long Date and Time customisation in the bookmarks info

As you can see, Camino still honours the long date and time customisation (I
only found one window with the long date and time in Camino so far), while
Mozilla does not (as far as the windows I have found are concerned). This is
probably related to the previous Comment 5, i.e. we should not pass the locale
pointer to the FormatTime/FormatPRTime on Mac OS X. In my search for the use of
these functions, I found the code that might create problems similar to the
ones described in the previous comment: 
http://lxr.mozilla.org/mozilla/source/intl/locale/src/nsScriptableDateFormat.cpp#94

Comment 7

[Constantine A. Murenin]

Attachment: clean-up patch for /intl/locale/src/nsScriptableDateFormat.cpp

This clean-up patch should fix the problem. 

In the original code, we were trying to get the application's locale and pass
it to the dateTimeFormat methods. This is incorrect --- we should use the
user's customisation regardless of the locale (i.e. we should pass nsnull as a
locale for FormatTime method). 

One way or the other, Mac-specific code (including nsDateTimeFormatMac.cpp) IS
correct (!), but the use of the dateTimeFormat methods outside of their class
is not. This patch should fix the problem on Mac and leave all other OS'
behaviour as it is of today.

Comment 8

[Jungshik Shin]

Comment on attachment 142657 [details] [diff] [review]
clean-up patch for /intl/locale/src/nsScriptableDateFormat.cpp


>-  if (localeName.IsEmpty())
>-    localeService->GetApplicationLocale(getter_AddRefs(locale));

You should keep this. Otherwise, locale would be left uninitialized.

Comment 9

[Constantine A. Murenin]

(In reply to comment #8)
> (From update of attachment 142657 [details] [diff] [review])
> 
> >-  if (localeName.IsEmpty())
> >-    localeService->GetApplicationLocale(getter_AddRefs(locale));
> 
> You should keep this. Otherwise, locale would be left uninitialized.

No, we should not keep this. This bug is about removing exactly that code! See
my comment #7.

I intended to use nsnull as locale, unless !localeName.IsEmpty(). 

As a reference, take a look at bug #231280, which is resolved with a similar patch. 

Comment 10

[Constantine A. Murenin]

Attachment: patch for /mailnews/compose/src/nsMsgCompose.cpp

I think this is the last misuse of locale for Date and Time formatting. Again,
the idea of the patch is the same as of the ones before --- we leave locale set
to nsnull by default.

Comment 11

[Jungshik Shin]

Comment on attachment 142657 [details] [diff] [review]
clean-up patch for /intl/locale/src/nsScriptableDateFormat.cpp

>-  if (!locale)
>-    return NS_ERROR_NOT_AVAILABLE;
>+    NS_ENSURE_TRUE(locale, NS_ERROR_FAILURE);

Please, use NS_ERROR_NOT_AVAILABLE. It might be better to keep |if (!locale)
return ....| unless you think it's 'assertion-worthy'.


>-  if (!dateTimeFormat)
>-    return NS_ERROR_NOT_AVAILABLE;
>+  NS_ENSURE_TRUE(dateTimeFormat, NS_ERROR_FAILURE);

ditto.

With that, r=jshin

Comment 12

[Constantine A. Murenin]

Attachment: clean-up patch v.2 for intl/locale/src/nsScriptableDateFormat.cpp

This patch addresses jshin comments. We are now using the real 'rv' value.

Comment 13

[Jungshik Shin]

Comment on attachment 143768 [details] [diff] [review]
clean-up patch v.2 for intl/locale/src/nsScriptableDateFormat.cpp

r=jshin (if you insist...)

Comment 14

[chris hofmann]

Comment on attachment 143768 [details] [diff] [review]
clean-up patch v.2 for intl/locale/src/nsScriptableDateFormat.cpp

a=chofmann for 1.7b

Comment 15

[chris hofmann]

Comment on attachment 142690 [details] [diff] [review]
patch for /mailnews/compose/src/nsMsgCompose.cpp

a=chofmann for 1.7b

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

both patches checked in

Checking in intl/locale/src/nsScriptableDateFormat.cpp;
/cvsroot/mozilla/intl/locale/src/nsScriptableDateFormat.cpp,v  <-- 
nsScriptableDateFormat.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.422; previous revision: 1.421
done

Comment 17

[Constantine A. Murenin]

Both patches were checked in by biesi on 2004-03-16 13:55 PST. 
Mozilla App Suite 1.7b and later will contain the fix.  

Comment 18

[HARUNAGA Hirotoshi]

I think this check-in caused Bug 239535.

Comment 19

[Constantine A. Murenin]

(In reply to comment #18)
> I think this check-in caused Bug 239535.

What makes you think this way? Have you tried to reproduce it before and after
the patch was applied? I think the nightlies are still around... The patch is
simple and does not look to contain any errors. If the bug exists, it must be
elsewhere. The code of this patch is platform-independent. 

Comment 20

[Constantine A. Murenin]

Sorry, I see the description of your bug now. This patch might have activated
some other bug in FormatTime.