-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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 #10078
Conversation
/botio-windows preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no way to test this, but I believe it should work.
l10n/en-US/viewer.properties
Outdated
find_matches_count[other]={{current}} of {{total}} matches | ||
# LOCALIZATION NOTE (find_matches_count_limit): The supported plural forms are | ||
find_match_count={[ plural(n) ]} | ||
find_match_count[zero]={{current}} of {{total}} matches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular case should never happen, since when there's no matches the counter is hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it happen when https://github.com/mozilla/pdf.js/blob/master/web/pdf_find_controller.js#L514 is triggered then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope; please note that total
must be non-zero for the message to be displayed in the first place, see https://github.com/mozilla/pdf.js/blob/master/web/pdf_find_bar.js#L162 and https://searchfox.org/mozilla-central/source/toolkit/content/widgets/findbar.js#1131.
I'll admit that this was perhaps not as clear as it could have been, given that the line in pdf_find_bar.js
probably should have been e.g. if (total > 0) {
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the new commit.
l10n/en-US/viewer.properties
Outdated
find_matches_count[one]={{current}} of {{total}} match | ||
find_matches_count[other]={{current}} of {{total}} matches | ||
# LOCALIZATION NOTE (find_matches_count_limit): The supported plural forms are | ||
find_match_count={[ plural(n) ]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having looked at the Firefox findbar implementation again, see here, I'm no longer convinced that there's any attempt to format these numbers according to locale.
So it might be possible to simplify this by removing the toLocaleString()
calls from the JS code, and change this to just {[ plural(total) ]}
respectively {[ plural(limit) ]}
for for the other string below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll also check in the browser and if it doesn't do the formatting either, I'll indeed remove it and use total
/limit
in the plural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the new commit. The browser find bar indeed also doesn't do any formatting according to locale, so I've removed that here too.
e52f615
to
f711dbc
Compare
/botio-windows preview |
From: Bot.io (Windows)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/1781dcbe329b133/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/1781dcbe329b133/output.txt Total script time: 4.30 mins Published |
@rvandermeulen Could you do a new upstream merge that contains these changes? This is necessary for a correct localization of these strings. |
Fixes #10076.