Closed
Bug 1195196
Opened 9 years ago
Closed 6 years ago
Page info: string "securityNVisits" (count of previous visits) should use a proper plural form
Categories
(Firefox :: Page Info Window, defect)
Firefox
Page Info Window
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: a.polivanchuk, Assigned: flod)
Details
Attachments
(2 files)
The following string does not have plural forms: browser -> chrome -> browser -> pageInfo.properties -> securityNVisits As a result, localized string looks wrong.
Assignee | ||
Updated•9 years ago
|
Component: uk / Ukrainian → Page Info Window
Product: Mozilla Localizations → Firefox
Summary: [uk] Firefox: String "securityNVisits" (Yes, %S times) does not have plurals → Page info: string "securityNVisits" (count of previous visits) should use a proper plural form
Assignee | ||
Comment 1•9 years ago
|
||
String originally introduced in bug 377076.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
@johannh Sending this in your direction, based on the file's log. I don't usually write JavaScript code, and had a hard time finding clues on how to format a multiline ternary expression (or if that's even frowned upon). At this point, I'm only sure that it passes eslint. Note on the new string for "No": reusing Yes/No is a bad idea, there are languages that don't have that, and reply with the entire sentence, e.g. "Do you want a cookie? I want a cookie".
Assignee: nobody → francesco.lodolo
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8944234 [details] Bug 1195196 - Use proper plural form for number of visits in Page Info https://reviewboard.mozilla.org/r/214510/#review220170 This works for me (the ternary expression looks a tiny bit unwieldy but still fine), but please fix the comments below. ::: browser/base/content/pageinfo/security.js:7 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > Components.utils.import("resource://gre/modules/BrowserUtils.jsm"); > +Components.utils.import("resource://gre/modules/PluralForm.jsm"); You don't need to import this when you do the lazy getter below. ::: browser/base/content/pageinfo/security.js:236 (Diff revision 1) > - pageInfoBundle.getString("securityOneVisit")); > - } else { > - setText("security-privacy-history-value", noStr); > - } > > + var visitCountStr = visitCount === 0 Nit: we usually use let instead of var for new code ::: browser/locales/en-US/chrome/browser/pageInfo.properties:48 (Diff revision 1) > feedXML=XML > > securityNoOwner=This website does not supply ownership information. > -securityOneVisit=Yes, once > -securityNVisits=Yes, %S times > +# LOCALIZATION NOTE (securityVisitsNumber): > +# Semi-colon list of plural forms. > +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals Nit: can you use an https link, please? :)
Attachment #8944234 -
Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #4) > > Components.utils.import("resource://gre/modules/BrowserUtils.jsm"); > > +Components.utils.import("resource://gre/modules/PluralForm.jsm"); > > You don't need to import this when you do the lazy getter below. Ugh, too much copy and paste :-\ > ::: browser/base/content/pageinfo/security.js:236 > (Diff revision 1) > > - pageInfoBundle.getString("securityOneVisit")); > > - } else { > > - setText("security-privacy-history-value", noStr); > > - } > > > > + var visitCountStr = visitCount === 0 > > Nit: we usually use let instead of var for new code Thanks, I could only find 'var' used in the file, didn't considered that was due to legacy code. > ::: browser/locales/en-US/chrome/browser/pageInfo.properties:48 > (Diff revision 1) > > feedXML=XML > > > > securityNoOwner=This website does not supply ownership information. > > -securityOneVisit=Yes, once > > -securityNVisits=Yes, %S times > > +# LOCALIZATION NOTE (securityVisitsNumber): > > +# Semi-colon list of plural forms. > > +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals > > Nit: can you use an https link, please? :) Fixed (double checked that compare-locales doesn't look at the full URL for plural checks)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8944234 [details] Bug 1195196 - Use proper plural form for number of visits in Page Info https://reviewboard.mozilla.org/r/214510/#review220190 ::: browser/base/content/pageinfo/security.js:235 (Diff revisions 2 - 3) > setText("security-privacy-passwords-value", > realmHasPasswords(uri) ? yesStr : noStr); > > var visitCount = previousVisitCount(info.hostName); > > - let visitCountStr = visitCount === 0 > + let visitCountStr = visitCount > 0 Noticed an issue while giving a last test to the updated patch: I completely missed that visitCount can be false (no host, e.g. for about: pages).
Pushed by francesco.lodolo@mozillaitalia.org: https://hg.mozilla.org/integration/autoland/rev/980a3a48f2af Use proper plural form for number of visits in Page Info r=johannh
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/980a3a48f2af
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in
before you can comment on or make changes to this bug.
Description
•