597852 - Internationalize decimal separator in Download Manager

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[u60234]

In the Download Manager window, file sizes are displayed with a period as a decimal separator. In many locales this should be a comma.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Internationalize the number format returned by DownloadUtils.convertByteUnits().

Also fix comment explaining convertByteUnits's signature and improve test coverage.

Comment 2

[Philipp von Weitershausen [:philikon]]

Pushed to try, doesn't seem to cause any weird test failures (convertByteUnits is used not only in Download Manager). Try builds are available at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-dc0b30808e63/ if anybody wants to try it out on a non-English Linux or Windows installation.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 476762 [details] [diff] [review]
v1

>+const kDecimalSymbol = Number(5.4).toLocaleString().match(/\D/);
I suspect we don't want to pay this cost when we just import this jsm.  Make this a lazy getter please?

Also, Mardak would be a good person to do this review (also likely faster).

Comment 4

[Ed Lee :Mardak]

toLocaleString is based on the OS's locale vs the firefox locale, no? So if you use en-US, you might still get somethign like 5,4?

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> toLocaleString is based on the OS's locale vs the firefox locale, no? So if you
> use en-US, you might still get somethign like 5,4?

Indeed, but it's the only solution we have available right now it seems, and various other parts of Firefox use it as well.

Note that I think it *kind of* makes sense to listen to the OS here. Date and number formatting usually are OS-wide settings and most programs adhere to it, even if they're not running in the OS's language. If anything, this would get us parity with other software.

Comment 6

[Ed Lee :Mardak]

Comment on attachment 476762 [details] [diff] [review]
v1

>diff --git a/toolkit/mozapps/downloads/DownloadUtils.jsm b/toolkit/mozapps/downloads/DownloadUtils.jsm
>+++ b/toolkit/mozapps/downloads/DownloadUtils.jsm
>- * [double convertedBytes, string units]
>+ * [string convertedBytes, string units]
Oh hrm. Seems like .toFixed was always returning a string already.

>   convertByteUnits: function DU_convertByteUnits(aBytes)
>+    if (kDecimalSymbol != ".")
>+      aBytes = aBytes.replace(".", kDecimalSymbol);
Doh. I guess we can't just call .toLocaleString because that requires a number to convert to a string, so then we can't call .toFixed.

Yeah a lazy getter could be useful as not all functions will use this.

Comment 7

[Shawn Wilsher :sdwilsh]

Does this also change the decimal separator for the progress status?  If so, we win in the app update prompt too!

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)
> Oh hrm. Seems like .toFixed was always returning a string already.

Exactly :)

> Doh. I guess we can't just call .toLocaleString because that requires a number
> to convert to a string, so then we can't call .toFixed.

Precisely...

> Yeah a lazy getter could be useful as not all functions will use this.

Will do!

(In reply to comment #7)
> Does this also change the decimal separator for the progress status?

If you're referring to the progress status in the download manager, then yes. In fact, DownloadUtils.convertByteUnits() is used in a bunch of places that should all benefit from this.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

It's generally best to avoid tests whose results depend on platform/locale differences, where possible. We've already sort of failed at that, but not making the problem worse would likely be appreciated by the person who's eventually tasked with getting out unit tests run on localized builds!

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> It's generally best to avoid tests whose results depend on platform/locale
> differences, where possible. We've already sort of failed at that, but not
> making the problem worse would likely be appreciated by the person who's
> eventually tasked with getting out unit tests run on localized builds!

It shouldn't be too hard to make these tests OS locale agnostic. App locale is a different beast of course... A lot of those tests depend on the strings from the en-US locale!

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Addressed sdwilsh's and gavin's review comments:
* Make gDecimalPoint a lazy getter
* Make new tests OS locale agnostic.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 476878 [details] [diff] [review]
v2

We ended up deciding (in the case of the About Firefox window, where we were trying to localize the date) that this sort of change can lead to cases where the Firefox locale and the OS locale differ, and the net is that someone using a French localized Firefox on an English Windows machine wouldn't get the appropriate localization, or vice versa.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

FWIW, I'm actually fine with this change, but not with the inconsistent reasoning that's being applied here.

We either use the system locale for these things, or we find some other way of doing it within our own localization infrastructure, period.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 476878 [details] [diff] [review]
v2

This can go in. Phillip points out that we're ALREADY inconsistent everywhere else w.r.t numbers, so I'm probably pushing a rock up a hill.

I'll file a separate bug on what I see as a glaring inconsistency in our approach to localization. It will begin with 600 if you're looking for it.

Comment 15

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/b7500203ad6d

Comment 16

[[:Aleksej]]

Downloaded a page using today's en-US nightly on Linux i686.
- with the en_US locale, dot was used
- with the eo locale, comma was used (correct)