Error in properly localized plurals of number of matches in Addressbook search

RESOLVED FIXED in Thunderbird 53.0

Status

defect
--
trivial
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: piotrdrag, Assigned: aceman)

Tracking

Trunk
Thunderbird 53.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
+++ This bug was initially created as a clone of Bug #1313950 +++

Dashboard shows an error for this string, even though it has been translated correctly:

https://l10n.mozilla.org/dashboard/compare?run=650844

https://hg.mozilla.org/l10n-central/pl/rev/0cf430ee6773
Assignee

Comment 1

3 years ago
I wonder what is going on here. The %S placeholder seems to work fine for English.

Piotr, is the string actually causing any problems when running TB? If you go to the addressbook type some character into the search field, do you get "X matches found" in the status bar?


The change in bug 1313950 was:

-matchesFound=%S matches found
+# LOCALIZATION NOTE (matchesFoundCount): Semi-colon list of plural forms
+# $S is the number of matches found
+matchesFoundCount=%S match found;%S matches found

The we use the string as:
+        statusText = PluralForm
+          .get(total, gAddressBookBundle.getString("matchesFoundCount"))
+          .replace("%S", total);

So I have 2 theories:
1. there is a bug in the Localization note, that I mention $S while in the string it is really %S.
2. all other strings used with PluralForm module contain placeholders of the form #1. But I have used %S. Is that l10n tool so clever to expect #1 in a string used via PluralForm?

Piotr, can you determine which of these is true?
Can you change your translation to contain $S instead of %S, let the tool run, then change it to #1 and let the tool run again?
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Reporter

Comment 2

3 years ago
(In reply to :aceman from comment #1)
> Piotr, is the string actually causing any problems when running TB? If you
> go to the addressbook type some character into the search field, do you get
> "X matches found" in the status bar?
> 

On current nightly searching works, but the "X matches found" string is in English when it should be in Polish, which is the real bug here. I didn't test nightly before, sorry.

> So I have 2 theories:
> 1. there is a bug in the Localization note, that I mention $S while in the
> string it is really %S.
> 2. all other strings used with PluralForm module contain placeholders of the
> form #1. But I have used %S. Is that l10n tool so clever to expect #1 in a
> string used via PluralForm?
> 

I don't know.

> Piotr, can you determine which of these is true?
> Can you change your translation to contain $S instead of %S, let the tool
> run, then change it to #1 and let the tool run again?

Stef, is this something we could do?
(In reply to Piotr Drąg from comment #2)
> > Piotr, can you determine which of these is true?
> > Can you change your translation to contain $S instead of %S, let the tool
> > run, then change it to #1 and let the tool run again?
> 
> Stef, is this something we could do?

Sure we could but wouldn't it be better to ask about dashboard behavior?
Flags: needinfo?(l10n)
The bug is in the original patch, it doesn't have the right comment.

It needs to contain the MDN url, https://developer.mozilla.org/docs/Mozilla/Localization/Localization_and_Plurals, notably the "Localization_and_Plurals" part. The comment is the only reliable way to tell if an entity is a pluralized string is that comment.

Also note that within plural forms, #1 etc is the checked parameter.

Anything else is not supported by compare-locales and the dashboard.

Also, .repace %S? That's horrific, there's no guarantee that tools will hand that back to you (and not %1$S, for example).
Flags: needinfo?(l10n)
Assignee

Comment 5

3 years ago
(In reply to Axel Hecht [:Pike] from comment #4)
> Also note that within plural forms, #1 etc is the checked parameter.

Why, where is such a rule in the PluralForm.jsm code?

> Anything else is not supported by compare-locales and the dashboard.
> 
> Also, .repace %S? That's horrific, there's no guarantee that tools will hand
> that back to you (and not %1$S, for example).

Why, what's wrong with %S ?
Flags: needinfo?(l10n)
(In reply to :aceman from comment #5)
> (In reply to Axel Hecht [:Pike] from comment #4)
> > Also note that within plural forms, #1 etc is the checked parameter.
> 
> Why, where is such a rule in the PluralForm.jsm code?

There's no need for that. The variable format is documented on MDN.

> > Anything else is not supported by compare-locales and the dashboard.
> > 
> > Also, .repace %S? That's horrific, there's no guarantee that tools will hand
> > that back to you (and not %1$S, for example).
> 
> Why, what's wrong with %S ?

What's wrong is that the localized message is looking like printf, but that's not what the code is doing.

Either printf or not. In the case of plurals, not, use #1 (and #2 etc if needed).

That's the way it's documented.
Flags: needinfo?(l10n)
Assignee

Comment 7

3 years ago
(In reply to Axel Hecht [:Pike] from comment #6)
> > > Also note that within plural forms, #1 etc is the checked parameter.
> There's no need for that. The variable format is documented on MDN.

There is not. Nowhere in the page it is written that the format of the placeholder must be #N.

> > Why, what's wrong with %S ?
> 
> What's wrong is that the localized message is looking like printf, but
> that's not what the code is doing.

It is doing that in a way. PluralForms.get().replace() seems semantically the same as printf().

> Either printf or not. In the case of plurals, not, use #1 (and #2 etc if
> needed).
> 
> That's the way it's documented.

It is not.

Considering the PluralForms DOES NOT actually replace the placeholders, it has no business to dictate any format of the placeholders. It even doesn't (in the code), you just read more into the mdn page than there is (it is only an example).

If it would replace the placeholders (e.g. like bundle.getFormattedString()) then yes, the format of the placeholders would be set in stone (in the code).
You got the answer.

Debating doesn't change it.

Change your broken code.
Assignee

Comment 9

3 years ago
The code is working.
It's some broken i10n tool that is forcing non-existent restrictions on the strings. There is also no mention on the page that the string must be preceded by some specially crafted comment and the url.

But yes, it is not worth debating.
Let's punish translators for nothing.
(In reply to :aceman from comment #9)
> The code is working.
> It's some broken i10n tool that is forcing non-existent restrictions on the
> strings. There is also no mention on the page that the string must be
> preceded by some specially crafted comment and the url.
> 
> But yes, it is not worth debating.
> Let's punish translators for nothing.

It's no news that the Mozilla documentation is often incomplete and outdated... That's life.

Now go and fix your code...
Reporter

Comment 11

3 years ago
(In reply to :aceman from comment #9)
> The code is working.

It is not. The string is in English in the UI.
Assignee

Comment 12

3 years ago
Posted patch patchSplinter Review
Attachment #8815049 - Flags: review?(philip.chee)
Attachment #8815049 - Flags: review?(jorgk)
Assignee

Comment 13

3 years ago
(In reply to Axel Hecht [:Pike] from comment #8)
> You got the answer.

Thanks for analysing the problem and the explanation.
Reporter

Comment 14

3 years ago
The summary line in the patch is factually incorrect. It's not a restriction of the l10n dashboard tool - that tool has nothing to do with the fact that the string doesn't get translated.

Comment 15

3 years ago
Comment on attachment 8815049 [details] [diff] [review]
patch

You have my blessing to land this without Ratty's review. Would you like an uplift asap (as soon as possible)?

Do we use %S elsewhere where we shouldn't?
Attachment #8815049 - Flags: review?(philip.chee)
Attachment #8815049 - Flags: review?(jorgk)
Attachment #8815049 - Flags: review+
You really want to change that string ID. If you don't, existing localizations will be used and people won't even notice the problem.

(In reply to :aceman from comment #7)
> It is doing that in a way. PluralForms.get().replace() seems semantically
> the same as printf().

In printf() (and .getFormattedString) you can swap ordered and unordered variables, as long as you use them consistently. That would break your .replace('%S')

A comment about the need for localization notes is available here
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms
Assignee

Comment 17

3 years ago
(In reply to Piotr Drąg from comment #14)
> The summary line in the patch is factually incorrect. It's not a restriction
> of the l10n dashboard tool - that tool has nothing to do with the fact that
> the string doesn't get translated.

How do you know?

The placeholder is getting replaced properly in the English version. I guess it would work too with Polish version. If some tool has stopped the PL translated string to get into the localized TB package, then yes, it will display in English.

How did you determine what did actually happen?
Assignee

Comment 18

3 years ago
(In reply to Francesco Lodolo [:flod] from comment #16)
> You really want to change that string ID. If you don't, existing
> localizations will be used and people won't even notice the problem.

I think I did change it.

> In printf() (and .getFormattedString) you can swap ordered and unordered
> variables, as long as you use them consistently. That would break your
> .replace('%S')

How? If I needed 2 ordered placeholders I could use %1$S or #1, but for one placeholder %S seems to work.
Assignee

Comment 19

3 years ago
(In reply to Jorg K (GMT+1) from comment #15)
> You have my blessing to land this without Ratty's review. Would you like an
> uplift asap (as soon as possible)?

I did not plan to uplift this as it was supposed to only be a warning.
I now hear for the first time that the string is actually not working in (not getting into) the app due to that. So yes, uplift seems needed.

> Do we use %S elsewhere where we shouldn't?
I thought I saw an occurrence from which I did copy the pattern. But I was reviewing the cases again recently and couldn't find it.

But I saw some plural strings that had no placeholders (not even in the plural form) which looked strange. But as long as localizers understood what to do in such a case, it should work (I checked my language and it was fine).
Reporter

Comment 20

3 years ago
(In reply to :aceman from comment #17)
> (In reply to Piotr Drąg from comment #14)
> > The summary line in the patch is factually incorrect. It's not a restriction
> > of the l10n dashboard tool - that tool has nothing to do with the fact that
> > the string doesn't get translated.
> 
> How do you know?
> 
> The placeholder is getting replaced properly in the English version. I guess
> it would work too with Polish version. If some tool has stopped the PL
> translated string to get into the localized TB package, then yes, it will
> display in English.
> 
> How did you determine what did actually happen?

I don't believe dashboard even touches the translation. The error it shows is another symptom of the same bug, not the cause of it.
Reporter

Comment 21

3 years ago
(In reply to :aceman from comment #19)
> I did not plan to uplift this as it was supposed to only be a warning.
> I now hear for the first time that the string is actually not working in
> (not getting into) the app due to that. So yes, uplift seems needed.
> 

I have mentioned it in comment #2.
Assignee

Comment 22

3 years ago
(In reply to Piotr Drąg from comment #21)
> I have mentioned it in comment #2.

Sorry, I missed that.

(In reply to Piotr Drąg from comment #20)
> > How did you determine what did actually happen?
> I don't believe dashboard even touches the translation. The error it shows
> is another symptom of the same bug, not the cause of it.

Sorry, I don't believe that, unless you show me which code in https://dxr.mozilla.org/comm-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/mozilla/intl/locale/PluralForm.jsm causes rejection of even fetching the translated string and just returns the english one if the placeholders are not "valid". I think you can't because AFAIK e.g. a PL localized TB package only contains the PL strings, it does not contain the original en-US strings. So there is no way a bug in the TB code would cause English string to be shown.
On the other hand I can imagine this happening in packaging, where the l10n tool producing the error on dashboard intentionally does not pass the PL string into the localized TB package and intentionally passes the original en-US string, while all other strings are PL.
Assignee

Comment 23

3 years ago
https://hg.mozilla.org/comm-central/rev/07d6927af34925d4072d6b961b6bed8a353a4ea6

Piotr, do you translate also c-c trunk, or will you see the new string only when it gets into Aurora/Earlybird branch?
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Reporter

Comment 24

3 years ago
We translate on central, so I can test the next build tomorrow.
compare-locales is expected to drop strings with errors, and add en-US strings for those that are missing, in order to avoid YSOD.
If you check your omni.jar, you should find that English string at the bottom of the localization file.

Comment 26

3 years ago
Uplift?

Yesterday I was playing with https://l10n.mozilla.org. You can select a language and see what errors the dashboard complains about.

If you check for example es-ES (https://l10n.mozilla.org/dashboard/compare?run=653154) you see:
  not all variables used in l10n at line 72, column 15 for matchesFound
which appears to be the wrong line number, hmm.
(In reply to Jorg K (GMT+1) from comment #26)
> Uplift?

No uplift needed, the correction patch needs an correction on central.

On aurora we have old string https://hg.mozilla.org/releases/comm-aurora/file/tip/mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties#l114,l115 and on central old id was reused in correction patch, https://hg.mozilla.org/comm-central/diff/07d6927af349/mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties

When merge will happen, translators using string id based tools may not notice that change and use old, single form version ("matchesFound=%S matches found").

> Yesterday I was playing with https://l10n.mozilla.org. You can select a
> language and see what errors the dashboard complains about.
> 
> If you check for example es-ES
> (https://l10n.mozilla.org/dashboard/compare?run=653154) you see:
>   not all variables used in l10n at line 72, column 15 for matchesFound
> which appears to be the wrong line number, hmm.

It's not an error (red iirc) but warning, check columns on https://l10n.mozilla.org/shipping/dashboard?locale=es-ES

Comment 28

3 years ago
(In reply to Stefan Plewako [:stef] from comment #27)
> No uplift needed, .. 
Sorry, right, bug 1313950 only landed on TB 53.

> ... the correction patch needs an correction on central.
Please explain, still not right now?
https://hg.mozilla.org/comm-central/rev/07d6927af34925d4072d6b961b6bed8a353a4ea6

Or did you mean to say:
The correction patch [from bug 1313950] *needed* a correction on central [which is now done]?
(In reply to Jorg K (GMT+1) from comment #28)
> > ... the correction patch needs an correction on central.
> Please explain, still not right now?
> https://hg.mozilla.org/comm-central/rev/
> 07d6927af34925d4072d6b961b6bed8a353a4ea6
> 
> Or did you mean to say:
> The correction patch [from bug 1313950] *needed* a correction on central
> [which is now done]?

No, I mean that patch from attachment 8815049 [details] [diff] [review] landed as https://hg.mozilla.org/comm-central/rev/07d6927af34925d4072d6b961b6bed8a353a4ea6 changes string id to "matchesFound" that happens to be the old id, used on central before bug 1313950 and still used on aurora.

When merge will happen, l10n tools (based on string ids and set up for aurora) will only check the current state (no change in string id) and not the changes history ("matchesFound" → "matchesFoundCount" → "matchesFound") and may not present the string as needing translation.

Comment 30

3 years ago
Understood. Aceman, can you please prepare another patch to change the name to matchesFound2 or so.
Reporter

Comment 31

3 years ago
I can confirm that Polish translation of this string works in today's nightly.
Assignee

Comment 32

3 years ago
(In reply to Stefan Plewako [:stef] from comment #29)
> When merge will happen, l10n tools (based on string ids and set up for
> aurora) will only check the current state (no change in string id) and not
> the changes history ("matchesFound" → "matchesFoundCount" → "matchesFound")
> and may not present the string as needing translation.

Interesting that you even need to have stringIDs changed between Auroras (not just on trunk), but OK, we can do that.
Blocks: 1313950

Comment 33

3 years ago
Landed follow-up to change the name to matchesFound1:
https://hg.mozilla.org/comm-central/rev/e3219462a85c0300b40ddd34d36fde68b5f148dd

This should bring this jolly good bug to a final close ;-)
Assignee

Comment 34

3 years ago
Thanks, looks fine now. r=me

Comment 35

3 years ago
Comment on attachment 8815049 [details] [diff] [review]
patch

Note that there were two landings in this bug.
Attachment #8815049 - Flags: approval-comm-aurora+

Updated

3 years ago
Attachment #8815049 - Flags: approval-comm-aurora+ → approval-comm-aurora?
Assignee

Updated

3 years ago
Depends on: 1322026

Comment 37

3 years ago
Comment on attachment 8815049 [details] [diff] [review]
patch

Note that two changesets were pushed to Aurora.
Attachment #8815049 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.