Closed Bug 668756 Opened 13 years ago Closed 11 years ago

decimal separator according to TB locale version

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: mstanke, Assigned: aceman)

References

Details

(Keywords: polish, ux-consistency)

Attachments

(2 files, 2 obsolete files)

Attached image 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 ','.
Status: UNCONFIRMED → NEW
Component: General → Mail Window Front End
Ever confirmed: true
QA Contact: general → front-end
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
See Also: → 673761
Blocks: 673761
I'll implements squib's solution in comment 2 if there is no opposition.
Assignee: nobody → acelists
OS: Other → All
Attached patch patch (obsolete) — Splinter Review
Attachment #648048 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 648048 [details] [diff] [review]
patch

Neil, please see if this fixes the problem in Seamonkey too.
Attachment #648048 - Flags: review?(neil)
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.
Attachment #648048 - Flags: review?(squibblyflabbetydoo) → review-
I am not sure ssnprintf will return anything containing a thousands separator. Also, what can be 1000GB insize TB? :)
(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.
Attached patch patch v2 (obsolete) — Splinter Review
OK, this almost works, however I do not know how to get HAVE_LOCALECONV defined. It seems undefined on my Linux build.
Attachment #648048 - Attachment is obsolete: true
Attachment #648048 - Flags: review?(neil)
Attachment #648073 - Flags: feedback?(squibblyflabbetydoo)
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...
Attachment #648073 - Flags: feedback?(squibblyflabbetydoo) → feedback+
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.
Attachment #648073 - Flags: feedback?(mbanner)
Filed bug 779736 for the issue I mentioned in comment 9.
Thanks. Your patch will bitrot me so I set a dependency.
Depends on: 779736
> 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
Status: REOPENED → ASSIGNED
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).
Attachment #648073 - Flags: feedback?(mbanner) → feedback+
Comment on attachment 648073 [details] [diff] [review]
patch v2

>+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
Nit: nsDependentCString isn't null-safe.
(In reply to neil@parkwaycc.co.uk from comment #18)
> >+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
> Nit: nsDependentCString isn't null-safe.

So what would be the solution to that?
Neil?
Comment on attachment 648073 [details] [diff] [review]
patch v2

(In reply to neil@parkwaycc.co.uk from comment #18)
> >+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
> Nit: nsDependentCString isn't null-safe.

So what would be the solution to that?
Attachment #648073 - Flags: feedback?(neil)
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);
Attachment #648073 - Flags: feedback?(neil) → feedback+
Attached patch patch v3Splinter Review
Attachment #648073 - Attachment is obsolete: true
Attachment #666257 - Flags: review?(mbanner)
Attachment #666257 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f993893786c0

Seems like this is something that should have tests?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago11 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
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.
Depends on: 819271
You need to log in before you can comment on or make changes to this bug.