Remaining time is displayed with Latin digits in Persian

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: flod, Assigned: peter.sa.d)

Tracking

({good-first-bug})

Trunk
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

a year ago
Currently, the remaining time is displayed using Latin digit, while download units are correctly localized.
(Reporter)

Comment 1

a year ago
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.
Keywords: good-first-bug

Comment 3

a year ago
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.
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.
(Assignee)

Comment 5

a year ago
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?
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!
Assignee: nobody → peter.sa.d
(Assignee)

Comment 7

a year ago
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?
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.
(Assignee)

Comment 9

a year ago
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?
(Reporter)

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
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!
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 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 15

a year ago
mozreview-review
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!
Attachment #8955974 - Flags: review?(gandalf) → review+
Peter - feel free to set "checkin-needed" keyword on the bug, which will trigger release managers to land your patch!
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 17

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c91f7e66f342
Download timeLeft is now formatted according to locale; r=gandalf
Keywords: checkin-needed
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
Flags: needinfo?(peter.sa.d)
(Assignee)

Comment 19

a year ago
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?
Flags: needinfo?(peter.sa.d) → needinfo?(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?
Flags: needinfo?(gandalf) → needinfo?(paolo.mozmail)

Comment 21

a year ago
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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 22

a year ago
I just wanted to clarify, is rs a flag like "r?reviewer" is? If so, would "rs=me" mean using "rs?paolo.mozmail@amadzone.org" in my commit?
Flags: needinfo?(paolo.mozmail)
(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?paolo.mozmail@amadzone.org" 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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 24

a year ago
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?
Flags: needinfo?(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!
Flags: needinfo?(gandalf)
Comment hidden (mozreview-request)

Comment 27

a year ago
Pushed by zbraniecki@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0bce62afe395
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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 :)
(Assignee)

Comment 30

a year ago
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!
You need to log in before you can comment on or make changes to this bug.