668756 - decimal separator according to TB locale version

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Attachment: screenshot

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330



Actual results:

Wherever TB shows a filesize, decimal separator is used. But in some countries people are using decimal "comma" instead of decimal point.
TB uses decimal point always and everywhere.

(reported http://forum.mozilla.cz/viewtopic.php?f=4&t=4385)


Expected results:

The decimal separators should be 'localizable' or customizable - '.' or ','.

Comment 2

[Jim Porter (:squib)]

Not a duplicate. Bug 161116 is a core issue, and (probably) wouldn't fix this one. I have a fix in mind for this, but it's super-hacky (basically find "." in the size and replace it with "," if need be). Incidentally, that's how Firefox handles the decimal in file sizes too.

Comment 3

[:aceman]

I'll implements squib's solution in comment 2 if there is no opposition.

Comment 4

[:aceman]

Attachment: patch

Comment 5

[:aceman]

Comment on attachment 648048 [details] [diff] [review]
patch

Neil, please see if this fixes the problem in Seamonkey too.

Comment 7

[Jim Porter (:squib)]

Comment on attachment 648048 [details] [diff] [review]
patch

Review of attachment 648048 [details] [diff] [review]:
-----------------------------------------------------------------

We should probably get the decimal separator from the system, like so: http://mxr.mozilla.org/comm-central/source/mozilla/js/src/jsnum.cpp#1069

We'll also want to check if the thousands separator is used (try formatFileSize on something >1000 GB), and convert that if so.

Comment 8

[:aceman]

I am not sure ssnprintf will return anything containing a thousands separator. Also, what can be 1000GB insize TB? :)

Comment 9

[Jim Porter (:squib)]

(In reply to :aceman from comment #8)
> I am not sure ssnprintf will return anything containing a thousands
> separator. Also, what can be 1000GB insize TB? :)

It turns out there's an off-by-one error in formatFileSize, so >999.5 GB sizes can't be printed. I fixed that in a local patch, which I will post later today. However, you don't need to worry about the thousands separator.

Comment 10

[:aceman]

Attachment: patch v2

OK, this almost works, however I do not know how to get HAVE_LOCALECONV defined. It seems undefined on my Linux build.

Comment 11

[Jim Porter (:squib)]

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

This seems ok. Standard8 might know more about the build system and what we need to do for that...

Comment 12

[:aceman]

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

The feature worked in my Linux build when I loaded localeconv() unconditionally (no ifdef). But I have no idea how to get proper HAVE_LOCALECONV value from the build system.

Comment 13

[Jim Porter (:squib)]

Filed bug 779736 for the issue I mentioned in comment 9.

Comment 14

[:aceman]

Thanks. Your patch will bitrot me so I set a dependency.

Comment 15

[Philip Chee]

This might be overkill http://mxr.mozilla.org/comm-central/source/mozilla/gfx/cairo/cairo/src/cairo-output-stream.c#293

Comment 16

[Philip Chee]

> But I have no idea how to get proper HAVE_LOCALECONV value from the build system.
Perhaps something like this?
http://mxr.mozilla.org/comm-central/source/mozilla/configure.in#3790

Comment 17

[Mark Banner (:standard8)]

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

I'd support this, Philip is right, that you'd need to port something like:

http://hg.mozilla.org/mozilla-central/annotate/c676b554c7bb//configure.in#l3737

to the c-c configure (which is actually part of the patch from bug 565089).

Comment 18

[[e-mail]]

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

>+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
Nit: nsDependentCString isn't null-safe.

Comment 19

[:aceman]

(In reply to [e-mail] from comment #18)
> >+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
> Nit: nsDependentCString isn't null-safe.

So what would be the solution to that?

Comment 20

[:aceman]

Neil?

Comment 21

[:aceman]

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

(In reply to [e-mail] from comment #18)
> >+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
> Nit: nsDependentCString isn't null-safe.

So what would be the solution to that?

Comment 22

[[e-mail]]

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

>+  // The ssprintf may return a decimal number using a dot (.) as the decimal
>+  // separator. Now we try to localize the separator.
Nit: no point localising the separator if there is no dot.

>+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
NS_ConvertUTF8toUTF16 decimalSeparator(decimalPoint);

Comment 23

[:aceman]

Attachment: patch v3

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f993893786c0

Seems like this is something that should have tests?

Comment 25

[Frank Wein [:mcsmurf]]

Also see Bug 819271 on this, the test_formatFileSize.js xpcshell test fails on systems which use a locale that does not use "." as decimal separator. Maybe that test can be modified to check different locales.