1154448 - Use different placeholders for calendar-extract.properties

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

There are expected l10n errors in calendar-extract.properties due to double ordered arguments. Its not trivial to fix bug 890900 and we need to remove all expected errors so that l10n-merge doesn't overwrite the strings when we land bug 1153327.

The solution would be to change the strings to not use the real placeholders (%1$S) but instead different placeholders. These would be treated as normal text by the string bundle, and need to be processed manually in javascript.

I don't mind which placeholder we use. PluralForm.jsm uses the #1 format, other alternatives include $1 {1} or if needed {name}.

Comment 1

[Merike Sell (:merike)]

Attachment: use #1/#2/#3 instead

There, let's try using #1/#2/#3 instead.

Axel, can you confirm if using # should work without issues with l10n tools used by localizers?
Also, I haven't changed entity names yet. Should I? Most of these are optional and wouldn't show up on dashboard, I think. Would an automatic update in localizations be doable to save localizers doing extra work without semantic change they need to look at?

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8671589 [details] [diff] [review]
use #1/#2/#3 instead

Review of attachment 8671589 [details] [diff] [review]:
-----------------------------------------------------------------

Codewise this looks fine, but I do think you need to change the entity names. Even if they are optional, otherwise there is no way to detect that something needs to be changed. Pike can correct me if I missed something though.

::: calendar/base/modules/calExtract.jsm
@@ +1268,5 @@
>          this.collected.push(guess);
>      }
>  };
>  
>  String.prototype.sanitize = function() {

I wasn't aware we're modifying the String prototype here. This needs to be fixed because it can have adverse affects on other code, e.g. for..in loops.

Can you file a bug (with patch, preferably) to move this to separate functions?

Comment 3

[Merike Sell (:merike)]

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Codewise this looks fine, but I do think you need to change the entity
> names. Even if they are optional, otherwise there is no way to detect that
> something needs to be changed. Pike can correct me if I missed something
> though.
Right, this depends on whether we force each localizer to apply changes theirself or if it can be automated for them. There isn't a context nor content change so batch replace is possible but historically such automation has been avoided with very few exceptions. If so then I definitely need to update entities.

> I wasn't aware we're modifying the String prototype here. This needs to be
> fixed because it can have adverse affects on other code, e.g. for..in loops.
> 
> Can you file a bug (with patch, preferably) to move this to separate
> functions?
Sure.

Comment 4

[Axel Hecht [:Pike]]

To the point of string ID changes, one thing we could do is to support both the old and the new way in parallel. I still need to check that l10n files wouldn't trigger the printf checks, but I think those only kick in when en-US uses them.

Comment 5

[Merike Sell (:merike)]

(In reply to Axel Hecht [:Pike] from comment #4)
> To the point of string ID changes, one thing we could do is to support both
> the old and the new way in parallel. I still need to check that l10n files
> wouldn't trigger the printf checks, but I think those only kick in when
> en-US uses them.

At least on my machine where comm-central is patched to use new ones and et still uses old ones compare-locales doesn't complain. Supporting both would complicate code in a way I'd prefer to avoid still. Scripting changes for localizers seems a better way to me but I'm likely not aware of all aspects that need to be considered for this.

Comment 6

[Axel Hecht [:Pike]]

Sounds good to me.

For locales in pontoon, we can easily fix this in version control. For locales in pootle, not yet, but maybe by the time we get there.

Let's get this in to comm-central and let it ride the trains.

Fallen, flod, do you have an opinion how/when/which we could help which locales to follow this change? Not sure how to select which locales we could already fix up on central, and which are better served by waiting for aurora.

Comment 7

[Francesco Lodolo [:flod]]

My list of locales that do work on central (i.e. have direct commits) is really short these days: da, eo, es-AR, es-CL, es-ES, fr, it, pl, ru, sk.

To be honest I'm always pretty scared when it comes to automate changes involving localization pieces. The list of changes is short and focused on one file, so the diff would be manageable, but still scary.

If we decide to script changes, I think we should wait for Aurora, and double check the list of locales above.

Comment 8

[Merike Sell (:merike)]

Looks like this can be checked in then.

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/207e8d6c494ed786f1ce9354e20e507dea45c874
Bug 1154448 - Use different placeholders for calendar-extract.properties. r=philipp

Comment 10

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Hi. Should we make the same change in the localized files, or not?

Also seems to me, this has fixed bug 890900, or bug 1141233 at least.

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ] from comment #10)
> Hi. Should we make the same change in the localized files, or not?
> 
> Also seems to me, this has fixed bug 890900, or bug 1141233 at least.

Considering you work on mozilla-aurora, not until this moves there on merge day (Dec 14).

Comment 12

[Merike Sell (:merike)]

(In reply to Francesco Lodolo [:flod] from comment #7)
> To be honest I'm always pretty scared when it comes to automate changes
> involving localization pieces. The list of changes is short and focused on
> one file, so the diff would be manageable, but still scary.
Feel free to let me know if you think a second pair of eyes would help.

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Merike (:merike) from comment #12)
> (In reply to Francesco Lodolo [:flod] from comment #7)
> > To be honest I'm always pretty scared when it comes to automate changes
> > involving localization pieces. The list of changes is short and focused on
> > one file, so the diff would be manageable, but still scary.
> Feel free to let me know if you think a second pair of eyes would help.

To avoid any possible confusion: I didn't volunteer to make those changes and don't have resources to look into it before merge day.

I still don't even know if the right way is to run a script for all locales. For sure this change should be communicated clearly to the mailing list, especially to avoid confusion between central and aurora.

So far nobody has picked up that change on central
https://transvision.mozfr.org/string/?entity=calendar/chrome/calendar/calendar-extract.properties:from.ordinal.date&repo=central

Comment 14

[Merike Sell (:merike)]

(In reply to Francesco Lodolo [:flod] from comment #13)
> To avoid any possible confusion: I didn't volunteer to make those changes
> and don't have resources to look into it before merge day.
Ok.

> I still don't even know if the right way is to run a script for all locales.
What does this decision depend on? Personally, as a localizer I'd cringe if I was told I need to manually change replaceables in a file when it can be scripted. Whether I'd need to do that in Pootle or text file wouldn't matter.

> For sure this change should be communicated clearly to the mailing list,
> especially to avoid confusion between central and aurora.
> 
> So far nobody has picked up that change on central
> https://transvision.mozfr.org/string/?entity=calendar/chrome/calendar/
> calendar-extract.properties:from.ordinal.date&repo=central
What problems does it cause if someone picks it up on central and switches over to the new format? Or did you only mean that locales working on aurora should not touch central? Writing to the list is not hard but at the moment I don't understand what exactly should be communicated. As far as I can tell there's roughly three categories of localizations: these who work on central, these who work on aurora but commit directly and others using localization tools. What they should or should not do is different and depends on whether scripting will be used, no? From Lightning's perspective it doesn't matter too much where it's fixed first and by who as long as the change reaches all channels.

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Merike (:merike) from comment #14)
> What does this decision depend on? Personally, as a localizer I'd cringe if
> I was told I need to manually change replaceables in a file when it can be
> scripted. Whether I'd need to do that in Pootle or text file wouldn't matter.

I've been yelled at before for touching productization files (not even file containing localized strings).
IMO we should provide a reliable script, as multi-platform as possible, and get a list of locales that don't want (or can't) run it and fix it for them.

As for the last questions, I was only pointing out that nobody noticed the change yet, and applied it to their locale. It's also true that these days we have very few locales working there.

I was stressing the importance of making clear where to make changes because last time (/devtools) we had a localizer applying a l10n-central fix to mozilla-aurora (the instructions weren't clear enough).

Localizers working on mozilla-aurora should not touch l10n-central.
Localizers working on tools (Pootle, Pontoon) should not touch l10n-central, and probably don't even have Mercurial access.
Only localizers actively working on l10n-central should touch l10n-central, and they're responsible for moving things to mozilla-aurora on their own.

At this point it might make sense to do this communication at the very beginning of the aurora cycle: it applies to all, and localizers working on l10n-central are technically capable of moving the fix around or run the script.

Comment 16

[Stefan Sitter [:ssitter]]

This change is now live on comm-aurora. I assume that all locales are now broken until the strings are updated. How will you proceed with the required change?

Comment 17

[Merike Sell (:merike)]

I've written to dev.l10n list and will be monitoring which locales need help and which have made the change themselves.

Comment 18

[Masahiko Imanaka [:marsf]]

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Codewise this looks fine, but I do think you need to change the entity
> names. Even if they are optional, otherwise there is no way to detect that
> something needs to be changed. 

+1

Merike:
IMO, changing the entity name is primitive and the best solution here. So, you don't need to track changes for all locale.
Sorry for my late comment.

Comment 19

[Stefan Sitter [:ssitter]]

I downloaded Lightning 4.7b1 <https://addons.mozilla.org/thunderbird/addon/lightning/versions/4.7b1>

Searching for %1$S in file calendar-extract.properties results in the following languages that are still using the old format:

  ca
  de
  dsb
  en-GB
  es-AR
  et
  fi
  ga-IE
  gd
  hsb
  hu
  is
  it
  ko
  nb-NO
  nn-NO
  pl
  pt-BR
  sk
  sq
  sv-SE
  uk
  zh-CN
  zh-TW

Will they be automatically converted to the new format? 
Or will they be excluded from the Lightning 4.7 release?

Comment 20

[Merike Sell (:merike)]

Neither, as far as i know.

Automatic conversion on aurora repositories wasn't possible as Pootle wouldn't have taken it well with locales working through Pootle. On beta it might have been possible except that Pootle removed previously localized strings from files based on the fact that original string had changed (despite the keys not changing). If there are still strings using old format on beta (I haven't checked yet) automatic conversion can be discussed.

Exclusion is not required as I'm unaware of any cases where old strings would break conversion. Rather conversion isn't as efficient as it could be. That's a matter of localization quality, really. I'm not sure which rules apply to sign-offs for Thunderbird but certainly Philipp (who I think approves them) could require old format replaced or patterns added if a locale has very few of them as a requirement for approval if he decides to do so.

Also, for example et is currently shipping old format because of a missing sign-off, perhaps there are more locales like that?