1434844 - Remaining time is displayed with Latin digits in Persian

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Francesco Lodolo [:flod]]

Attachment: DownloadRemainedTimeEnglish.jpg

Currently, the remaining time is displayed using Latin digit, while download units are correctly localized.

Comment 1

[Francesco Lodolo [:flod]]

Attachment: RemainedHouresAreLatin.jpg

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

This function - https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/toolkit/mozapps/downloads/DownloadUtils.jsm#257 - has to be refactored to use `Services.intl.NumberFormat` to format the number into the right numeral system.

Marking this as a good first bug.

Comment 3

[Amir Farsi]

Zibi, this problem is in Firefox for Android too.
Steps for reproduce:
1-Insatll Firefox for Android Nightly(Persian)
2-Select UI language as Persian
3-Download a file
4-Goto Download section of Firefox For Android via it's main menu.
5-You will see all of digits(Like download Percents) in English, not in Persian.
6-If you cehck download progress bar in notfication center of android, it's buttons are in Persian but Digits are in English.(This notification is download progress related to download file in Firefox)

8-I haven't iOS device to check. I think this problem is in Firefox for iOS too.
Please fix these problems too.

Note: I didn't create new bug about Fennec. I think we can follow all of problems related to show Latin digits instead of Persian digits in here. But if you wanted, create a new bug for Firefox for Android and Firefox for iOS.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Let's first fix the bug in toolkit's DownloadUtils and see how much it fixes. I don't know if Android is using the same code or something else. If the Fennec bug will not get fixed here, we can file a follow up.

Comment 5

[Peter Dodds]

Hi, I would like to try working on this as a good-first-bug. Is it possible to be assigned, and is there anything else I should know for this bug now as a new user?

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Sure! Thanks for stepping up Peter.

All you need to know is how to trigger it:

1) Build Firefox
2) Install `fa` langpack and switch to `fa` locale
2) Attempt to download a big file (for example: http://cdimage.ubuntu.com/daily-live/current/)
3) Notice the string about remaining bytes/time has bytes in eastern arabic numerals, but hours/minutes/seconds left in western arabic numerals.
4) Refactor the function I pointed out to format the numbers using `let nf = new Services.intl.NumberFormat(); nf.format(number);`

Let me know if you have any questions!

Comment 7

[Peter Dodds]

I do have a question about the langpack. I don't seem to be able to find a compatible version for my build. For reference, I'm running Linux Mint 18.3 and built Firefox with these instructions:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation

and I am trying to get the language pack here:
https://addons.mozilla.org/en-US/firefox/addon/persian-ir-%D9%81%D8%A7%D8%B1%D8%B3%DB%8C-%D8%A7%DB%8C%D8%B1%D8%A7%D9%86-lang/

I'm running Nightly 59.0a1, and the language pack page tells me the latest version, 58.0, is not compatible with this. Does this mean I need to find a way to downgrade, or I should be building/running something else, or some other solution?

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

You can find a nightly langpack here: https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/win64/xpi/ (look for fa langpack for 60!)

If you built Firefox on your own, you should have 60 as well.

Comment 9

[Peter Dodds]

Thank you for all the help so far. I've got 60 now and got the correct language pack, which I can see in my addons. However, I can't seem to find where I actually change the interface language.

I was first trying to follow this page: https://support.mozilla.org/en-US/kb/use-firefox-interface-other-languages-language-pack which mentions the config options intl.locale.matchOS and general.useragent.locale, but neither of those exist for me in about:config. I also tried looking up whether there are any Nightly-specific instructions, but had no luck. What can I do about this?

Comment 10

[Francesco Lodolo [:flod]]

1. Open about:config
2. Right click, create a new key with type String, name intl.locale.requested, set it to fa
3. Restart Firefox

general.useragent.locale was removed starting from 59 (bug 1414390), that doc still needs to be updated.

Comment 11

[Peter Dodds]

Attachment: 1434844-timeleft-locale.patch

I'll admit, I'm still a bit lost in the documentation regarding patches and reviews and the whole process, but I have the patch now and I figure a good start is at least getting it out here. If anyone can further advise on what next steps may be necessary, it would be much appreciated!

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks for the patch!

In the future, you can improve the interaction with your mentor (me in this case) in two ways:

1) Learn to use MozReview ( http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html ) - it gives us both a lot of tools to analyze the patch and test it!
2) Use "Need info" checkbox below the comment box to flag me when you want me to answer questions or set "f?" (feedback request) on the attachment to request my feedback on it.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8948178 [details] [diff] [review]
1434844-timeleft-locale.patch

Review of attachment 8948178 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +259,5 @@
>      if (aLastSec == null)
>        aLastSec = Infinity;
>  
>      if (aSeconds < 0)
> +      return [nf.format(gStr.timeUnknown), aLastSec];

`gStr.timeUnknown` is a variable defined here: https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#77

Its value is a localization id defined here: https://searchfox.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties#94

Your change passes the localization ID to `nf.format` which expects a number.

I believe in reality you don't want to touch this case, since it's just returning a string.

@@ +314,4 @@
>        }
>      }
>  
> +    return [nf.format(timeLeft), aSeconds];

Here, on the other hand, `timeLeft` is a variable created based on `gBundle.formatStringFromName`.

I expect it to look something like "5 min 35 sec left".

Passing such a string to `NumberFormat.format()` will not work either.

You need to format the number (5 and 35) as you pass them to `formatStringFromName` instead.

Comment 14

[Peter Dodds]

Attachment: Bug 1434844 - Download timeLeft is now formatted according to locale, and tests updated to accomodate thousands separator; ,rs=paolo

Review commit: https://reviewboard.mozilla.org/r/224920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224920/

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8955974 [details]
Bug 1434844 - Download timeLeft is now formatted according to locale, and tests updated to accomodate thousands separator; ,rs=paolo

https://reviewboard.mozilla.org/r/224920/#review235938

This looks great! Thank you!

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Peter - feel free to set "checkin-needed" keyword on the bug, which will trigger release managers to land your patch!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c91f7e66f342
Download timeLeft is now formatted according to locale; r=gandalf

Comment 18

[Stefan Hindli [:stefan_hindli]]

Backed out changeset c91f7e66f342 (bug 1434844) for failures in toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/d29a10db52dc261f204f25d3eb555c27d8f74f8f
Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d29a10db52dc261f204f25d3eb555c27d8f74f8f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Problematic push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c91f7e66f3424890f13fe80e44a34df44ecccfa6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=169777333&repo=autoland&lineNumber=2643

[task 2018-03-23T00:21:00.534Z] 00:21:00     INFO -  TEST-PASS | toolkit/components/url-classifier/tests/unit/test_listmanager.js | took 3829ms
[task 2018-03-23T00:21:00.543Z] 00:21:00     INFO -  TEST-START | toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js
[task 2018-03-23T00:21:01.040Z] 00:21:01  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js | xpcshell return code: 0
[task 2018-03-23T00:21:01.041Z] 00:21:01     INFO -  TEST-INFO took 497ms

Comment 19

[Peter Dodds]

It appears the test failed because this test expects "1979d" but receives "1,979d":
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js#158

If I'm not mistaken, the "days of time left" case is the only case in which a number large enough to have commas can appear in the timeleft values (since seconds/minutes/hours will cap at 60/60/24). In addition, the comma-inclusive number is arguably a better and more readable format, regardless of how rare it is.

In this case where we can see the intention of both the test case and the patch's result, would an acceptable solution be to alter the expected value to match? Or would it be preferred to find some solution that would strictly return numbers without commas?

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yeah, I'd be in favor of altering the test. Let's ask someone more familiar with the code and practices :)

Paolo - we start formatting numbers in the UI and by default English uses thousand separator, so "1979d" turns into "1,979d".

We can either turn off the separators (useSeparators: false) or alter the test to establish a practice.

Would you be ok with that? Do you think we should involve UX/DSG?

Comment 21

[:Paolo Amadini]

Yes, I think changing the test is fine since Zibi is in favor too :-)

No need to ask for UX review here, and rs=me (meaning no need for further review) for the necessary test code changes, assuming it is just adding a comma.

Comment 22

[Peter Dodds]

I just wanted to clarify, is rs a flag like "r?reviewer" is? If so, would "rs=me" mean using "rs?[e-mail]" in my commit?

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Peter Dodds from comment #22)
> I just wanted to clarify, is rs a flag like "r?reviewer" is? If so, would
> "rs=me" mean using "rs?[e-mail]" in my commit?

you'd use `rs=paolo` as in `Bug XXXX - Lorem ipsum dolor sit amet. r=gandalf,rs=paolo`

but please, wait till the week opens up, I'd like to cross-check with the UX that they're ok with us starting to use thousand separators.

Comment 24

[Peter Dodds]

Hi, I wanted to check up on your cross-check, and also ask about the process for resubmitting a commit because I seem to be having some trouble with this.

My assumption was that I should create a new commit with all changes (downloadUtils and testDownloadUtils), then follow the same process for pushing to review. However, this gives me back the message "abort: Review request is submitted or discarded", and when I check the review online it is indeed marked as submitted. What's the correct course of action here?

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hi,

You don't have to create a new commit. You can use `hg ci --amend` to modify your commit to the following sequence:

```
Bug 1434844 - Download timeLeft is now formatted according to locale; r=gandalf,rs=paolo
```

You should do this once you add the test change and we should be good to land this.

Thanks!

Comment 26

[Peter Dodds]

Comment on attachment 8955974 [details]
Bug 1434844 - Download timeLeft is now formatted according to locale, and tests updated to accomodate thousands separator; ,rs=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224920/diff/1-2/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bce62afe395
Download timeLeft is now formatted according to locale, and tests updated to accomodate thousands separator; r=gandalf,rs=paolo

Comment 28

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0bce62afe395

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you Peter for your contribution! It's been a pleasure working with you and if you ever need any help in your future work, do not hesitate to NI/CC/r?/f? me :)

Comment 30

[Peter Dodds]

I can already only look back and groan and how long it took me, but I'm definitely glad to have done it. Thank you too for all your help!