Closed
Bug 1382193
Opened 7 years ago
Closed 7 years ago
Issues with about:telemetry strings landed for bug 1370513
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: stef, Assigned: flyingrub)
References
Details
Attachments
(3 files)
Bug 1370513 comment 55: > # - %1$S will be replaced by either telemetryEnabled or telemetryDisabled > # - %2$S will be replaced by either telemetryEnabled or telemetryDisabled > homeExplanation = Telemetry is %1$S and extended telemetry is %2$S. > > I'm sorry but such substitutions completely fail in languages like Polish - we > need separate forms for %1$S and %2$S. This gives something like: > homeExplanation=Telemetria jest %1$S i rejestrowanie rozszerzonych danych telemetrii jest %2$S. > telemetryEnabled=włączona > telemetryDisabled=wyłączona and it should be something like: > homeExplanation=Telemetria jest %1$S i rejestrowanie rozszerzonych danych telemetrii jest %2$S. > telemetryEnabled=włączona > telemetryDisabled=wyłączona > extendedTelemetryEnabled=włączone > extendedTelemetryDisabled=wyłączone There are also other problems mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1370513#c54
Updated•7 years ago
|
Assignee: nobody → flyinggrub
Status: NEW → ASSIGNED
Comment 1•7 years ago
|
||
Copying over the other comment for clarity (In reply to Ton from comment #54) > 1) %1$S displays as "pings", i.e. the quotation marks are hardcoded. Locales > may however require quotation marks to be single, as well as curly (‘pings’). > 2) The "the %2$S ping" part may cause l10n issues, as it displays as "the > current ping" or "the 2017/07/08 10:40:46 - saved-session ping" which is > merely fine by coincidence in English, but locales may need to use "the ping > %2$S" for the latter, i.e. "the ping 2017/07/08 10:40:46 - saved-session" > suits them better, and hence "the ping current" will be wrong. > 3) The "current" text is not available for l10n.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
Making string changes to existing strings https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings Also, not sure I see logic dealing with "current", and why you would need a replacement there, instead of just pingExplanationCurrent = Each piece of information is sent bundled into “%1$S“. You are looking at the current ping.
The word current is clickable... We add html around the word to make it clickable.
Comment 7•7 years ago
|
||
(In reply to flyingrub from comment #6) > The word current is clickable... We add html around the word to make it > clickable. That makes sense, but can you point me to the code doing it (that's what I meant with logic above)?
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8888250 [details] Bug 1382193 - Fix Strings issue in about:telemetry https://reviewboard.mozilla.org/r/159204/#review164676
Attachment #8888250 -
Flags: review?(chutten)
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8888251 [details] Bug 1382193 - Fix hardcoded quote in about:telemetry https://reviewboard.mozilla.org/r/159206/#review164678
Attachment #8888251 -
Flags: review?(chutten)
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8888252 [details] Bug 1382193 - Avoid l10n issue in about:telemetry https://reviewboard.mozilla.org/r/159208/#review164680 Please have these reveiwed by someone who knows l10n more than me. The JS code looks fine, but I can't pass judgement on whether the strings'll work :)
Attachment #8888252 -
Flags: review?(chutten)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8888251 [details] Bug 1382193 - Fix hardcoded quote in about:telemetry https://reviewboard.mozilla.org/r/159206/#review164688 ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:18 (Diff revision 1) > homeExplanation = Telemetry is %1$S and extended telemetry is %2$S. > > # Note to translators: > # - %1$S will be replaced by a link with pingExplanationLink > # - %2$S will be replaced by the ping name > -pingExplanation = Each piece of information is sent bundled into %1$S. You are looking at the %2$S ping. > +pingExplanation = Each piece of information is sent bundled into “%1$S“. You are looking at the %2$S ping. You need to change this ID, since you’re introducing “” that were hard-coded before.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8888250 [details] Bug 1382193 - Fix Strings issue in about:telemetry https://reviewboard.mozilla.org/r/159204/#review165154
Attachment #8888250 -
Flags: review?(francesco.lodolo) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8888251 [details] Bug 1382193 - Fix hardcoded quote in about:telemetry https://reviewboard.mozilla.org/r/159206/#review165158
Attachment #8888251 -
Flags: review?(francesco.lodolo) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8888252 [details] Bug 1382193 - Avoid l10n issue in about:telemetry https://reviewboard.mozilla.org/r/159208/#review165162 ::: toolkit/content/aboutTelemetry.js:365 (Diff revision 2) > - if (pingName !== "current") { > + if (!this.viewCurrentPingData) { > pingType.hidden = false; > older.hidden = false; > newer.hidden = false; > pingType.textContent = this._getSelectedPingType(); > + pingName += ", " + this._getSelectedPingType(); I know this is a piece of existing code, but can you explain how this is used? For non current pings let pingName = this._getSelectedPingName(); pingName += ", " + this._getSelectedPingType(); What are the values of getSelectedPingName and getSelectedPingType? ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:35 (Diff revision 2) > > extendedTelemetryEnabled = enabled > > extendedTelemetryDisabled = disabled > > +pingExplanationLink = pings String is already in the file, remove this one
Assignee | ||
Comment 19•7 years ago
|
||
The ping name is the ping timestamp and the ping type can be something like "main" "saved-session".
Comment 20•7 years ago
|
||
Thanks. Ideally we should not have part of the text hard-coded. Option 1: If I understand the code correctly, pingDetails is only used for named pings at this point? So it could become # Note to translators: # - %1$S will be replaced by a link with pingExplanationLink # - %2$S will be replaced by the ping timestamp, e.g. "2017/07/08 10:40:46" # - %3$S will be replaced by the ping name, e.g. "saved-session" pingDetails = Each piece of information is sent bundled into “%1$S“. You are looking at the %2$S, %3$S ping. Option 2: Drop pingDetailsCurrent, build %2$S in pingDetails dinamycally a) if it's current b) if it's a named ping To remove the hard coded text in b), you'll need another string to build the ping name # Note to translators: # - %1$S will be replaced by the ping timestamp, e.g. "2017/07/08 10:40:46" # - %2$S will be replaced by the ping name, e.g. "saved-session" namedPing = %1$S, %2$S
Assignee | ||
Comment 21•7 years ago
|
||
> 2) The "the %2$S ping" part may cause l10n issues, as it displays as "the
> current ping" or "the 2017/07/08 10:40:46 - saved-session ping" which is
> merely fine by coincidence in English, but locales may need to use "the ping
> %2$S" for the latter, i.e. "the ping 2017/07/08 10:40:46 - saved-session"
> suits them better, and hence "the ping current" will be wrong.
I added pingDetailsCurrent because of this :).
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8888252 [details] Bug 1382193 - Avoid l10n issue in about:telemetry https://reviewboard.mozilla.org/r/159208/#review165592 Thanks, this looks much better. ::: toolkit/content/aboutTelemetry.js:373 (Diff revisions 2 - 3) > + explanation = bundle.formatStringFromName("pingDetails", parameters, 3); > } else { > pingType.hidden = true; > older.hidden = true; > newer.hidden = true; > - pingDate.textContent = pingName + " ping"; > + pingDate.textContent = bundle.GetStringFromName("currentPingSidebar"); Ah, didn't realize there was another piece hard coded. ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:38 (Diff revisions 2 - 3) > > -pingExplanationLink = pings > - > currentPing = current > > +currentPingSidebar = current ping I would add a short comment, since it took me a while to find. # Used as a tooltip for the "current" ping title in the sidebar
Attachment #8888252 -
Flags: review?(francesco.lodolo) → review+
Comment 24•7 years ago
|
||
On a side note: the last iteration shows another "issue" in the design. The current ping name is displayed on top of the sidebar, and it looks out of place: it's lowercase, while all the other titles are uppercase, and it's not much different from the other items. I wonder if it should be "Ping: current", or styled differently.
Assignee | ||
Comment 25•7 years ago
|
||
https://framapic.org/Gr9ap9PnumTd/JJSGE7fkYEJW.png I don't like that much adding a "Ping:" in the sidebar, but we definitely need to find something to differentiate it from the rest of the sidebar. Also in the home page only the timestamp is underlined, whereas before the ping type was too. I find it harder to understand that the ping type is also part of the ping. So I don't know if we should keep this part hardcoded or add a separator string ? https://framapic.org/n0QjGys6b1cC/rkmDHJPldjR4.png
Comment 26•7 years ago
|
||
(In reply to flyingrub from comment #25) > Also in the home page only the timestamp is underlined, whereas before the > ping type was too. I find it harder to understand that the ping type is also > part of the ping. So I don't know if we should keep this part hardcoded or > add a separator string ? If you need both underlined, a clean solution would be # Note to translators: # - %1$S will be replaced by a link with pingExplanationLink # - %2$S will be replaced by the namedPing pingDetails = Each piece of information is sent bundled into “%1$S“. You are looking at the %2$S ping. # Note to translators: # - %1$S will be replaced by the ping timestamp, e.g. "2017/07/08 10:40:46" # - %2$S will be replaced by the ping name, e.g. "saved-session" namedPing = %1$S, %2$S Having a different pingDetailsCurrent might not be needed, but at this point safer so keep it separated (more flexibility).
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8888252 [details] Bug 1382193 - Avoid l10n issue in about:telemetry https://reviewboard.mozilla.org/r/159208/#review165820 At :flod's request I've taken a look at the JS, since it's changed a bit since the initial version. It looks good.
Attachment #8888252 -
Flags: review+
Comment 29•7 years ago
|
||
The second patch has a typo:
> pingDetails = Each piece of information is sent bundled into “%1$S“. You are looking at the %2$S ping.
Note the repeated opening quotation marks (““ instead of “”). Please correct that before landing.
Comment 30•7 years ago
|
||
(In reply to Adolfo Jayme from comment #29) > Note the repeated opening quotation marks (““ instead of “”). Please correct > that before landing. Ugh, good catch, really hard to see with this font.
Comment hidden (mozreview-request) |
Keywords: checkin-needed
Comment 32•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d6d2bce75387 Fix Strings issue in about:telemetry r=flod https://hg.mozilla.org/integration/autoland/rev/753f56ca9c9f Fix hardcoded quote in about:telemetry r=flod https://hg.mozilla.org/integration/autoland/rev/8b274bfe7905 Avoid l10n issue in about:telemetry r=chutten,flod
Keywords: checkin-needed
Updated•7 years ago
|
Summary: Issues with strings landed for bug 1370513 → Issues with about:telemetry strings landed for bug 1370513
Updated•7 years ago
|
Priority: -- → P1
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d6d2bce75387 https://hg.mozilla.org/mozilla-central/rev/753f56ca9c9f https://hg.mozilla.org/mozilla-central/rev/8b274bfe7905
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•