Closed Bug 1382193 Opened 3 years ago Closed 3 years ago

Issues with about:telemetry strings landed for bug 1370513

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

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
Assignee: nobody → flyinggrub
Status: NEW → ASSIGNED
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.
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.
(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 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 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 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 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 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 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 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
The ping name is the ping timestamp and the ping type can be something like "main" "saved-session".
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
> 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 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+
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.
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
(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 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+
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.
(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.
Keywords: checkin-needed
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
Summary: Issues with strings landed for bug 1370513 → Issues with about:telemetry strings landed for bug 1370513
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.