Closed Bug 1412066 Opened 7 years ago Closed 6 years ago

The count of requests in Network pane is undefined for the 3rd plural form in Czech

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Version:
58 Branch
Type:
defect

Tracking

(firefox60 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: mstanke, Assigned: abhinav.koppula)

References

(
URL
)

Details

(Keywords: nightly-community, Whiteboard: [transvision-feedback])

Attachments

(1 file)

Bug 1412066 - Fix for count of requests in Network pane being undefined for the 3rd plural form in Czech;
59 bytes, text/x-review-board-request
Pike
: review+
Details 
The string:
One request;%S requests

Is translated as:
Jeden požadavek;%S požadavky;%S požadavků (in Czech we have 3 forms, see https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#Plural_rule_8_(3_forms))

However in the devtools only the first and second form work. Once the number of request is 5+, I see "undefined požadavků".

STR:
1) Open DevTools (F12) and go to the Sít (=Network) panel.
2) Check the Protokol natrvalo (=Keep the log).
3) In the address bar paste an arbitrary image link (e.g. https://bugzilla.mozilla.org/images/tabzilla.png).
4) Do that multiple times and watch the number of request growing in the bottom left corner of DevTools.
5) After the count reaches five, it will show undefined.

Feedback via Transvision:
https://transvision.mozfr.org/?sourcelocale=en-US&locale=cs&repo=gecko_strings&search_type=entities&recherche=devtools/client/netmonitor.properties:networkMenu.summary.requestsCount&entire_string=entire_string
Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171026100047
Component: cs / Czech → Developer Tools: Netmonitor
Product: Mozilla Localizations → Firefox
Version: unspecified → 58 Branch
@Michal, thanks for the report!

Honza
Priority: -- → P2
@Francesco, any tips how to fix this?

Honza
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] (traveling, slow answers, back Nov 22) from comment #2)
> I tried to follow the code but couldn't spot any glaring error explaining
> the issue
> http://searchfox.org/mozilla-central/rev/
> dd47bee6468de7e1221b4d006342ad6b9813d0e5/devtools/client/netmonitor/src/
> components/StatusBar.js#44
Ah, missed this comment. Could the problem be in Czech translation?
Where are other locales actually stored?

Honza
It sounds like an issue with languages with more than 2 plural forms. If that's the case, Czech should not be the only locale having this problem
https://transvision.mozfr.org/string/?entity=devtools/client/netmonitor.properties:networkMenu.summary.requestsCount&repo=gecko_strings

Plural rules: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals

As said, I tried to follow the code but got lost.

Also, I didn't realize that DevTools has its own version of pluralform. That will get out of sync very quickly (it already is)
https://searchfox.org/mozilla-central/source/devtools/shared/plural-form.js
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] (traveling, slow answers, back Nov 22) from comment #6)
> Also, I didn't realize that DevTools has its own version of pluralform. That
> will get out of sync very quickly (it already is)
> https://searchfox.org/mozilla-central/source/devtools/shared/plural-form.js
What is the right version we should use?

Here is the place where we format the string:
https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/shared/l10n.js#105-140

You might see the rest of the file.

Honza
Flags: needinfo?(francesco.lodolo)
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> (In reply to Francesco Lodolo [:flod] (traveling, slow answers, back Nov 22)
> from comment #6)
> > Also, I didn't realize that DevTools has its own version of pluralform. That
> > will get out of sync very quickly (it already is)
> > https://searchfox.org/mozilla-central/source/devtools/shared/plural-form.js
> What is the right version we should use?

That file was forked from
https://hg.mozilla.org/mozilla-central/log/tip/intl/locale/PluralForm.jsm

It doesn't get a lot of updates, but it gets them (there is at least another bug pending review).

> Here is the place where we format the string:
> https://searchfox.org/mozilla-central/rev/
> 9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/shared/l10n.js#105-140
> 
> You might see the rest of the file.

Sorry, my expertize is in localization more than development, and I can't spot anything obvious nor debug the issue.

Based on previous cases, I would expect the issue to be in this part of the code, where the string is split using ';' as a separator: either it's not split correctly, or the code tries to access an out of index value (which shouldn't be possible)
https://searchfox.org/mozilla-central/source/devtools/shared/plural-form.js#119
Flags: needinfo?(francesco.lodolo)
@Abhinav, do you have a bandwidth to look at this one?


Honza
Flags: needinfo?(abhinav.koppula)
@Abhinav: In order to test this, you might want to download cz version of Firefox:
https://www.mozilla.cz/stahnout/firefox/

Not, sure if there is easier way, but I wasn't able to switch the locale in English Firefox
(nor add cz language pack)

Honza
@Francesco, how can we switch locale in Firefox (I used an extension before 57)?

We can download cz version, but there is no way to test the patch.

Honza
Flags: needinfo?(francesco.lodolo)
1) All Nightly localized builds are available here
https://www.mozilla.org/firefox/nightly/all/

2) You can build your own localized build, or create a language pack, but it won't work with artifacts builds
https://firefox-source-docs.mozilla.org/build/buildsystem/locales.html

e.g.
./mach build installers-cs

For Czech.

3) There might be langpacks available on FTP, but they're really brittle on Nightly (YSOD are common), and I can't even look for them right now, since the FTP is basically broken (e.g. bug 1416668). 

I suggest to either go with 1 or 2
Flags: needinfo?(francesco.lodolo)
Thanks Francesco!

@Abhinav, are you able to reproduce the issue now?

Honza
Thanks for the pointers, Francesco.
Yes, Honza, I am able to reproduce the issue on nightly build of Czech.
I have started digging into the same.
Flags: needinfo?(abhinav.koppula)
Hi Honza/Francesco,
We are calling 
sprintf(this.getStr(name), ...args) -> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/l10n.js#122

Here name is `"networkMenu.summary.requestsCount"` and args is `Array [ "7" ]`
on evaluating -> `sprintf(this.getStr(name), ...args)` yields "Jeden požadavek;7 požadavky;undefined požadavků"
However if `sprintf(this.getStr(name), 7, 7)` is evaluated we get the expected -> "Jeden požadavek;7 požadavky;7 požadavků"

So first quick hack I did was changed 
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/StatusBar.js#42
to 
`PluralForm.get(
      count, L10N.getFormatStrWithNumbers("networkMenu.summary.requestsCount", count, count) //passing count 2 times
  );`

The above hack fixed the issue in Czech build.

Next I thought that this might need to be generically fixed for the cases where we are receiving less number of arguments while calling sprintf. So I am constructing newArgs so that the number of parameters sprintf expects is equal to the number of arguments passed.

I was able to apply my patch and check on Czech build using method[2] of comment-12.

Let me know your thoughts on this.
Comment on attachment 8931074 [details]
Bug 1412066 - Fix for count of requests in Network pane being undefined for the 3rd plural form in Czech;

https://reviewboard.mozilla.org/r/202164/#review207694

Thanks a lot for taking the time to explain the issue and fix. Having said that, I'm not a good pick to advise on this kind of patches, :gandalf or :pike might be able to help more.

While I'm sure this fixes the issue at hand, I don't think it's the right approach. For example, you're determining pluralFormLength in your code by counting %S, but that's not the only format for placeholders. Also, why are we formatting the whole string (unless I'm misunderstanding the code), instead of formatting the one we need?
Attachment #8931074 - Flags: review?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #17)
> While I'm sure this fixes the issue at hand, I don't think it's the right
> approach. For example, you're determining pluralFormLength in your code by
> counting %S, but that's not the only format for placeholders. Also, why are
> we formatting the whole string (unless I'm misunderstanding the code),
> instead of formatting the one we need?

I agree, this doesn't sounds right.

Gandalf, any tips for what is the right way to fix this?

Honza
Flags: needinfo?(gandalf)
Well, in principle, we're all converging around Intl.PluralRules ECMA402 API https://github.com/tc39/proposal-intl-plural-rules which is now Stage 4 and landed in Firefox and Chrome already.

Is there a chance you could work your way toward using it?

Long term, I assume the path forward is to migrate whole devtools to fluent-web (for DOM l10n) + fluent-react (for React part) which uses Intl.PluralRules, so that's covered.

But in the short term, I'm not sure how far you can take dirty hacking around APIs. You could probably use the same API as StringBundle to cut the string by `;`, then use PluralForms.jsm to get the right string for a given locale and then format it. But then, why not just use StringBundle?

I'm sorry for asking instead of helping, but I feel very unfamiliar with the devtools/l10n.js and the reasons behind it.
Flags: needinfo?(gandalf)
Comment on attachment 8931074 [details]
Bug 1412066 - Fix for count of requests in Network pane being undefined for the 3rd plural form in Czech;

https://reviewboard.mozilla.org/r/202164/#review215814

Unassigning myself from the review. I am not sure how to properly implement and test this issue.

Honza
Attachment #8931074 - Flags: review?(odvarko)
@:pike, perhaps you can help here?

Honza
Flags: needinfo?(l10n)
Sorry, I got lost in reverse engineering all the forked or new l10n code. No idea where the bug is, but flod's idea of splitting first and then formatting sounds about right.

Also, when using the plural form in properties, #1 etc are the expected placeholders, and the only ones supported by our infrastructure. Which, apparently, only networkMenu.summary.requestsCount in https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/netmonitor.properties#162 is not doing, using %S instead.

Maybe that's actually the culprit?
Flags: needinfo?(l10n)
Hi Axel,
Thanks for the help. Yes, I think it is happening because of the reason you mentioned.

I have created a review-request where I have updated the key and value of `networkMenu.summary.requestsCount` so that it can be updated in the other locales. 

When I tried testing my fix in my local with the czech build, it didn't work because 
the czech translations kept using %S - https://dxr.mozilla.org/l10n-central/source/cs/devtools/client/netmonitor.properties#165 and I couldn't find any way to change the czech translation in my local. 

However, if I replace 
`PluralForm.get(count, L10N.getStr("networkMenu.summary.requestsCount2")).replace("#1", count);` with 
`PluralForm.get(count, L10N.getStr("networkMenu.summary.requestsCount2")).replace("%S", count);`, the fix works perfectly in my local czech build.

Let me know what you think.
Comment on attachment 8931074 [details]
Bug 1412066 - Fix for count of requests in Network pane being undefined for the 3rd plural form in Czech;

https://reviewboard.mozilla.org/r/202164/#review221656

lgtm, thanks.
Attachment #8931074 - Flags: review?(l10n) → review+
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
TRY is green. Marking as checkin-needed
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ff7c9110dd0
Fix for count of requests in Network pane being undefined for the 3rd plural form in Czech; r=Pike
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ff7c9110dd0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
I have pushed the new string to l10n-central/cs for the next Nightly. NI myself to verify the fix.

https://hg.mozilla.org/l10n-central/cs/rev/1fd39ede3ab4
Flags: needinfo?(mstanke)
Works in Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 ID:20180128220152
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Flags: needinfo?(mstanke)
@Abhinav: great job here!

Honza
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: