Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve plural support for the matches counter #10076

Closed
timvandermeij opened this issue Sep 13, 2018 · 4 comments
Closed

Improve plural support for the matches counter #10076

timvandermeij opened this issue Sep 13, 2018 · 4 comments
Labels

Comments

@timvandermeij
Copy link
Contributor

It turns out that only specifying one and other in #10070 is not enough, so we may need to add all other options too for other locales (note that there are no bugs because of this, it should only be more helpful for other languages this way).

@flodolo
Copy link
Contributor

flodolo commented Sep 15, 2018

Important note: at this point you need to change the string ID for all these strings (it should have been changed in #10070).

It would be great if we could land this fix soon, and merge it back to m-c. If that requires a lot of time, I'll need to ask to backout https://hg.mozilla.org/mozilla-central/rev/5f9d6d1fa6cc

@flodolo
Copy link
Contributor

flodolo commented Sep 15, 2018

One more issue

find_matches_count={[ plural(n) ]}
find_matches_count[one]={{current}} of {{total}} match
find_matches_count[other]={{current}} of {{total}} matches

You define n as the variable defining plural, but it should be total.

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Sep 15, 2018

To summarize:

  • plural(n) should be changed to plural(total) and plural(limit);
  • along with [one] and [other] we should also provide [zero], [two], [few] and [many], which will just be copies;
  • after doing this, we need change all string IDs to be unique (for example find_match_count).

Correct? If so, this can certainly be fixed on our side. Would that require another upstream merge or would the current patch need to be backed out upstream anyway?

The only problem with changing plural(n) to plural(limit) is that we're setting limit: limit.toLocaleString() when making the string. This is problematic because in Dutch 1000 is formatted as 1.000 and the plural function will then think it's 1 with three decimals, thereby using the [one] string, which is incorrect. Note that we do in fact provide n at https://github.com/mozilla/pdf.js/pull/10070/files#diff-df7d4f6a0cce5f82de42345bfb7b6c4fR165.

@flodolo
Copy link
Contributor

flodolo commented Sep 15, 2018

Correct? If so, this can certainly be fixed on our side.

Yes

Would that require another upstream merge or would the current patch need to be backed out upstream anyway?

It depends on the timing. If it requires more than a few days to have the updated patch in m-c, we should back that out, then land the updated version of PDF.js when it's ready (there's no real need to back-out from PDF.js too).

The only problem with changing plural(n) to plural(limit) is that we're setting limit: limit.toLocaleString() when making the string.

I haven't used webl10n strings in years, so I'm not exactly sure. If limit is determined from n, it might still work, but needs testing, and a comment for localizers explaining what's used to determine the plural.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants