Note: There are a few cases of duplicates in user autocompletion which are being worked on.

decimal separator according to TB locale version

RESOLVED FIXED in Thunderbird 19.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
6 years ago
5 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

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