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

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Piotr Drąg [:piotrdrag]]

+++ 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

Comment 1

[:aceman]

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?

Comment 2

[Piotr Drąg [:piotrdrag]]

(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?

Comment 3

[Stefan Plewako [:stef]]

(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?

Comment 4

[Axel Hecht [:Pike]]

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).

Comment 5

[:aceman]

(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 ?

Comment 6

[Axel Hecht [:Pike]]

(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.

Comment 7

[:aceman]

(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).

Comment 8

[Axel Hecht [:Pike]]

You got the answer.

Debating doesn't change it.

Change your broken code.

Comment 9

[:aceman]

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.

Comment 10

[Adrian Kalla [:adriank]]

(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...

Comment 11

[Piotr Drąg [:piotrdrag]]

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

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

Comment 12

[:aceman]

Attachment: patch

Comment 13

[:aceman]

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

Thanks for analysing the problem and the explanation.

Comment 14

[Piotr Drąg [:piotrdrag]]

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

[Jorg K (CEST = GMT+2)]

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?

Comment 16

[Francesco Lodolo [:flod]]

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

Comment 17

[:aceman]

(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?

Comment 18

[:aceman]

(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.

Comment 19

[:aceman]

(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).

Comment 20

[Piotr Drąg [:piotrdrag]]

(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.

Comment 21

[Piotr Drąg [:piotrdrag]]

(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.

Comment 22

[:aceman]

(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.

Comment 23

[:aceman]

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?

Comment 24

[Piotr Drąg [:piotrdrag]]

We translate on central, so I can test the next build tomorrow.

Comment 25

[Francesco Lodolo [:flod]]

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

[Jorg K (CEST = GMT+2)]

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.

Comment 27

[Stefan Plewako [:stef]]

(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

[Jorg K (CEST = GMT+2)]

(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]?

Comment 29

[Stefan Plewako [:stef]]

(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

[Jorg K (CEST = GMT+2)]

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

Comment 31

[Piotr Drąg [:piotrdrag]]

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

Comment 32

[:aceman]

(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.

Comment 33

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 34

[:aceman]

Thanks, looks fine now. r=me

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8815049 [details] [diff] [review]
patch

Note that there were two landings in this bug.

Comment 36

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/e4048c5210628c6cf2942d6f3323e0069568f3d3
https://hg.mozilla.org/releases/comm-aurora/rev/4435e81e70e6b875ae15e3a70120b28ee031db2a

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8815049 [details] [diff] [review]
patch

Note that two changesets were pushed to Aurora.