211004 - mailnews.reply_header_locale de-DE not working

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Lars Hugentobler]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624

according to independent documentation, the user can set
mailnews.reply_header_type to "2" to include the date.
the user can furthermore configure some of the used string, .reply_header_locale
sets the date format.
when set to "de-DE", mozilla 1.4rc3 falls back to en-US.


Reproducible: Always

Steps to Reproduce:
1.set mailnews.reply_header_type to 2 and mailnews.reply_header_locale to
neither your LC_TIME setting nor en-US. for example ja-JP or de-DE.
2. reply to mail of choice and check reply header.

Actual Results:  
if .reply_header_locale is not set, the system default (LC_TIME) is used. if
anything is entered, mozilla uses fallback to en-US (mm/dd/yyyy).

Expected Results:  
my system is configured to use LC_TIME=ja_JP. this is used if i reset the value
(yyyyKmmKddK , K=kanji.)
setting to de-DE should change this to dd.mm.yyyy.

fallback is nice. on windows, an illegal value would return an empty string.
setting severity to normal since i consider it a geek feature (no ui to change
pref).

Comment 1

[Christian Eyrich]

Yeah, calls to locale->GetCategory() in nsDateTimeFormatUnix::Initialize()
always fail on Unix because of the aCategory.
Following simple patch makes it work again:
-  NS_NAMED_LITERAL_STRING(aCategory, "NSILOCALE_TIME##PLATFORM");
+  NS_NAMED_LITERAL_STRING(aCategory, "NSILOCALE_TIME");

BUT, I assume the ##PLATFORM postfix added in bug 61108 has a meaning. I see a
difference between building the nsLocale object in nsLocaleService::NewLocale()
and nsLocaleService::nsLocaleService. Shouldn't they both be the same if their
result is used from the same function?
NewLocale() is used to create the locale in
QuotingOutputStreamListener::QuotingOutputStreamListener() which is used in
FormatTMTime()

Jungshik, I cc'ed you because you did some patches in this area and I don't know
if Brian is still active.

Comment 2

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 3

[Christian Eyrich]

Attachment: proposed patch

Ok, this one removes the ##PLATFORM prefix from aCategory as I mentioned in comment 1.

As far as I can see aCategory is only used as key for lookup in a hash table. But the current string is nowhere else used -> no entry has this key and so looking up the hash table with that string will always fail.

Requesting review to either get this fixed this way or at least get a comment on what this thingy is here for.

Comment 4

[Benjamin Smedberg]

Comment on attachment 270037 [details] [diff] [review]
proposed patch

Moving review request to an intl peer

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 270037 [details] [diff] [review]
proposed patch

This isn't the right fix. That "##PLATFORM" was already removed in bug 153586 and restored in bug 257511 because it broke locales like ja_JP.UTF-8

Comment 6

[Christian Eyrich]

Attachment: patch v2

This patch works the other way around. The demand still is to have the key the values are stored with the same that's used to later retrieve them.
So if it has to be the one with ##PLATFORM in Initialize(), let us additionally store the locales using that postfix in NewLocale().
As far as I can see that is the same what's done in the constructor of nsLocaleService and nothing should get borked.

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 270722 [details] [diff] [review]
patch v2

This looks OK except for 2 nits:

> 	if (!resultLocale) return NS_ERROR_OUT_OF_MEMORY;
>-
> 	for(i=0;i<LocaleListLength;i++) {

If you don't have a good reason to remove the blank line, don't.

> 		result = resultLocale->AddCategory(category, aLocale);
>+#if (defined(XP_UNIX) && !defined(XP_MACOSX)) || defined(XP_BEOS)
>+        category.AppendLiteral("##PLATFORM");
>+		result = resultLocale->AddCategory(category, aLocale);

There's an indentation problem here. Don't use tabs.

What locales have you tested this on?

Comment 8

[Christian Eyrich]

Attachment: patch addressing the nits

Oh sorry, I've no idea how that line got deleted or those tabs creeped in. Removed those old tabs as well.

I've tested it with environment variable LANG set to de_DE.UTF-8 and en_GB.UTF-8. Locales I testet via the pref were de-DE, en-GB, en-DK, sv-SE and en-US, each with and without ".utf8". Those are the locales installed on my Linux machine.

Comment 9

[Christian Eyrich]

Is sr needed for this at all?

Comment 10

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

Comment on attachment 270945 [details] [diff] [review]
patch addressing the nits

OK, while you're touching this and reindenting everything, can you clean it up a bit? That is:

- Remove the cast when in initializing *_retval
- the style for the variables in the for-loop is also somewhat unusual, but this file is a mess of inconsistent styles, so you can leave this as-is...

- instead of this ->QueryInterface call, just do:
  NS_ADDREF(*_retval = resultLocale);
  return NS_OK;

Also, don't you need to check the return value of the first AddCategory call too?

Comment 11

[Christian Eyrich]

Attachment: addressing biesis comment

This addresses everything you wrote. I additionally moved the declaration of i into the loop.
Since this patch actually has more changes than my original one, I guess I need a new review, yes?

Comment 12

[Phil Ringnalda (:philor)]

intl/locale/src/nsLocaleService.cpp 1.66