The default bug view has changed. See this FAQ.

decimal separator according to TB locale version

RESOLVED FIXED in Thunderbird 19.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: MikkCZ, Assigned: aceman)

Tracking

({polish, ux-consistency})

unspecified
Thunderbird 19.0
polish, ux-consistency
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 543382 [details]
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
Keywords: polish, ux-consistency
QA Contact: general → front-end
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 161116

Comment 2

6 years ago
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: → bug 673761

Updated

6 years ago
Blocks: 673761
(Assignee)

Comment 3

5 years ago
I'll implements squib's solution in comment 2 if there is no opposition.
Assignee: nobody → acelists
OS: Other → All
(Assignee)

Comment 4

5 years ago
Created attachment 648048 [details] [diff] [review]
patch
Attachment #648048 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 5

5 years ago
Comment on attachment 648048 [details] [diff] [review]
patch

Neil, please see if this fixes the problem in Seamonkey too.
Attachment #648048 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 673761

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

5 years ago
Created attachment 648073 [details] [diff] [review]
patch v2

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 11

5 years ago
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+
(Assignee)

Comment 12

5 years ago
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)

Comment 13

5 years ago
Filed bug 779736 for the issue I mentioned in comment 9.
(Assignee)

Comment 14

5 years ago
Thanks. Your patch will bitrot me so I set a dependency.
Depends on: 779736

Comment 15

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

Comment 16

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

Comment 19

5 years ago
(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?
(Assignee)

Comment 20

5 years ago
Neil?
(Assignee)

Comment 21

5 years ago
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+
(Assignee)

Comment 23

5 years ago
Created attachment 666257 [details] [diff] [review]
patch v3
Attachment #648073 - Attachment is obsolete: true
Attachment #666257 - Flags: review?(mbanner)
Attachment #666257 - Flags: review?(mbanner) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f993893786c0

Seems like this is something that should have tests?
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 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.
(Assignee)

Updated

4 years ago
Depends on: 819271
You need to log in before you can comment on or make changes to this bug.