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

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?

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

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

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

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

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

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

(In reply to :aceman from comment #9) > The code is working. It is not. The string is in English in the UI.

(In reply to Axel Hecht [:Pike] from comment #8) > You got the answer. Thanks for analysing the problem and the explanation.

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

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

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

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

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

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

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

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

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?

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.

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

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

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

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

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

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

Thanks, looks fine now. r=me

Comment on attachment 8815049 [details] [diff] [review] patch Note that there were two landings in this bug.

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

Comment on attachment 8815049 [details] [diff] [review] patch Note that two changesets were pushed to Aurora.