Last Comment Bug 668756 - decimal separator according to TB locale version
: decimal separator according to TB locale version
Status: RESOLVED FIXED
: polish, ux-consistency
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
: 673761 (view as bug list)
Depends on: 779736 819271
Blocks: 673761
  Show dependency treegraph
 
Reported: 2011-07-01 02:47 PDT by Michal Stanke (Mozilla.cz) [:MikkCZ]
Modified: 2012-12-07 02:53 PST (History)
8 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (9.58 KB, image/png)
2011-07-01 02:47 PDT, Michal Stanke (Mozilla.cz) [:MikkCZ]
no flags Details
patch (3.94 KB, patch)
2012-08-01 13:03 PDT, :aceman
squibblyflabbetydoo: review-
Details | Diff | Splinter Review
patch v2 (2.27 KB, patch)
2012-08-01 14:05 PDT, :aceman
squibblyflabbetydoo: feedback+
standard8: feedback+
neil: feedback+
Details | Diff | Splinter Review
patch v3 (3.48 KB, patch)
2012-09-29 16:03 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review

Description Michal Stanke (Mozilla.cz) [:MikkCZ] 2011-07-01 02:47:15 PDT
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 ','.
Comment 1 Michal Stanke (Mozilla.cz) [:MikkCZ] 2011-07-23 23:28:42 PDT

*** This bug has been marked as a duplicate of bug 161116 ***
Comment 2 Jim Porter (:squib) 2011-07-24 00:57:09 PDT
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 2012-08-01 12:29:23 PDT
I'll implements squib's solution in comment 2 if there is no opposition.
Comment 4 :aceman 2012-08-01 13:03:15 PDT
Created attachment 648048 [details] [diff] [review]
patch
Comment 5 :aceman 2012-08-01 13:04:00 PDT
Comment on attachment 648048 [details] [diff] [review]
patch

Neil, please see if this fixes the problem in Seamonkey too.
Comment 6 :aceman 2012-08-01 13:04:11 PDT
*** Bug 673761 has been marked as a duplicate of this bug. ***
Comment 7 Jim Porter (:squib) 2012-08-01 13:09:53 PDT
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 2012-08-01 13:24:32 PDT
I am not sure ssnprintf will return anything containing a thousands separator. Also, what can be 1000GB insize TB? :)
Comment 9 Jim Porter (:squib) 2012-08-01 13:35:00 PDT
(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 2012-08-01 14:05:52 PDT
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.
Comment 11 Jim Porter (:squib) 2012-08-01 15:11:21 PDT
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 2012-08-01 21:52:01 PDT
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) 2012-08-02 01:24:50 PDT
Filed bug 779736 for the issue I mentioned in comment 9.
Comment 14 :aceman 2012-08-02 02:02:07 PDT
Thanks. Your patch will bitrot me so I set a dependency.
Comment 16 Philip Chee 2012-08-02 11:10:03 PDT
> 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) 2012-08-20 04:55:31 PDT
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 neil@parkwaycc.co.uk 2012-08-21 07:58:35 PDT
Comment on attachment 648073 [details] [diff] [review]
patch v2

>+  nsString decimalSeparator = NS_ConvertUTF8toUTF16(nsDependentCString(decimalPoint));
Nit: nsDependentCString isn't null-safe.
Comment 19 :aceman 2012-08-28 09:50:17 PDT
(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?
Comment 20 :aceman 2012-09-12 03:18:58 PDT
Neil?
Comment 21 :aceman 2012-09-28 13:33:17 PDT
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?
Comment 22 neil@parkwaycc.co.uk 2012-09-28 17:18:33 PDT
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 2012-09-29 16:03:42 PDT
Created attachment 666257 [details] [diff] [review]
patch v3
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-10-09 17:41:08 PDT
https://hg.mozilla.org/comm-central/rev/f993893786c0

Seems like this is something that should have tests?
Comment 25 Frank Wein [:mcsmurf] 2012-12-07 02:24:22 PST
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.

Note You need to log in before you can comment on or make changes to this bug.