mailnews.reply_header_locale de-DE not working

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Lars Hugentobler, Assigned: Christian Eyrich)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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).
(Assignee)

Comment 1

14 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: MailNews → Core
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
(Assignee)

Comment 3

10 years ago
Created attachment 270037 [details] [diff] [review]
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.
Assignee: nobody → ch.ey
Status: NEW → ASSIGNED
Attachment #270037 - Flags: review?(benjamin)
(Assignee)

Updated

10 years ago
Component: MailNews: Composition → Internationalization

Comment 4

10 years ago
Comment on attachment 270037 [details] [diff] [review]
proposed patch

Moving review request to an intl peer
Attachment #270037 - Flags: review?(benjamin) → review?(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
Attachment #270037 - Flags: review?(smontagu) → review-
(Assignee)

Comment 6

10 years ago
Created attachment 270722 [details] [diff] [review]
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.
Attachment #270037 - Attachment is obsolete: true
Attachment #270722 - Flags: review?(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?
(Assignee)

Comment 8

10 years ago
Created attachment 270945 [details] [diff] [review]
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.
Attachment #270722 - Attachment is obsolete: true
Attachment #270945 - Flags: review?(smontagu)
Attachment #270722 - Flags: review?(smontagu)

Updated

10 years ago
Attachment #270945 - Flags: review?(smontagu) → review+
(Assignee)

Updated

10 years ago
Attachment #270945 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 9

10 years ago
Is sr needed for this at all?
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?
Attachment #270945 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 11

10 years ago
Created attachment 273821 [details] [diff] [review]
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?
(Assignee)

Updated

10 years ago
Attachment #273821 - Flags: superreview?(cbiesinger)
Attachment #273821 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Updated

10 years ago
Attachment #273821 - Flags: review?(smontagu)

Updated

10 years ago
Attachment #273821 - Flags: review?(smontagu) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Attachment #270945 - Attachment is obsolete: true
intl/locale/src/nsLocaleService.cpp 1.66
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.