Copy complete locale translations - #4521
Conversation
mathjazz
left a comment
There was a problem hiding this comment.
Thanks for the patch!
The changes looks good. Please note however that setEditorFromHistory applies no plural remapping, so other locale's plural variants are copied verbatim. Copying a Russian one/few/other entry gives 3 editor fields regardless of the number of plural categories in the target locale — and nothing validates variants on save.
We should remap the parsed selector keys to the current locale (as getEmptyMessage already does).
In a followup, we should also look into showing the entire string in the Locales tab, similar to the Source string panel and the composed Machinery entries (see #2114).
|
Added plural remapping in 567356a. Locale copies now use the target categories, keeping matching patterns and using the source catchall for missing categories. Numeric variants and non-plural selectors are preserved; history copies keep their existing behavior. Both clicks and keyboard shortcuts use the remapping. The Russian-to-English editor regression fails before this change and passes after it. Added coverage for Arabic expansion, Chinese collapse, attributes, numeric variants and combined selectors. The full frontend suite passed (748 tests, 3 skipped), followed by the added combined-selector test; TypeScript, lint and build also passed. |
There was a problem hiding this comment.
Excellent work, thanks for the patch! As far as I'm concerned, this is good to go.
The patch is deployed to pontoon.allizom.org/.
But before we merge, I'd like to ask @flodolo, who reported the issue, to comment on the behaviour of this patch in the following cases:
- If a pluralized string is translated without any plural forms, we'll copy it as such, i.e. deleting any plural forms the target locale has.
- If a term has local attributes, they are copied verbatim into the editor. They usually contain locale-specific syntax traits.
Should we keep this behaviour?
Or fix it (here or in a followup)?
I don't think that's ideal, because it's very hard to restore missing plural forms in Pontoon (need to switch to advanced mode, and manually add the missing forms). Ideally, clicking would carry over only the forms defined for the locale, and leave others empty if the "source" locale (from the LOCALES tab) doesn't have them. If the source locale doesn't have plural forms, maybe copy in the default plural form?
Parameterized terms would fall back at run-time, so probably OK to copy without spending too much time fixing. P.S. there is a reason why we ask folks to comment in the issue before starting to work on one. This work wasn't really defined. |
@Hanabi9248, could you please tweak plural mapping to account for that? |
|
Updated in 277ccb1. Missing target plural categories now stay empty. A plain copied translation goes into the default plural form using the original entry as the template, so the other target fields remain available. Local attributes are still copied, and history restores keep their previous behavior. The two reported cases fail before this revision and pass after it. The full frontend suite passed with 751 tests and 3 skipped; an additional attribute/template immutability test also passed. TypeScript, ESLint and the build passed. You're right about clarifying the scope first. I should have discussed the copying behavior in the issue before starting; I'll do that for future work. |
mathjazz
left a comment
There was a problem hiding this comment.
Thanks for the update! The requested change is implemented, nice work!
However, there's one regression, see the comment inline.
| ); | ||
| }); | ||
| candidates = | ||
| exact.length || plurals.has(i) |
There was a problem hiding this comment.
Requiring an exact key match for plural selectors drops the source catchall when the target's catchall is named differently: getPluralCategories returns ['one','few','many'] for ru/uk/pl/be/szl.
So copying:
[one] One item
*[other] Many items
into ru now gives ["One item", "", ""]. The general plural is lost, while 567356a gave ["One item","Many items","Many items"].
Locales with an other catchall are fine (sl → ["One item","","","Many items"]), so matching catchall-to-catchall before falling through would fix it. The tests only use zh/ar/en/en-US, none of which have a many catchall.
|
Fixed in 4808c5a. Exact categories still take precedence; a target catchall now falls back to the source catchall when their names differ. Missing non-default plural forms remain empty. Added regressions for ru, uk, pl, be and szl, a Slovenian control, and a many-catchall source copied into a single-form locale. Six cases fail before the fix; all 15 helper tests now pass. The full frontend suite passed with 759 tests and 3 skipped, along with TypeScript, ESLint and formatting. |
mathjazz
left a comment
There was a problem hiding this comment.
Excellent job! Thank you for the patch and welcome to Pontoon! :)
Fix #4520.
Copying from the Locales tab currently sends the preview text to the focused field. For a Fluent entry with
.labeland.accesskey, this replaces the label but leaves the old access key. Select expressions also lose their other variants.Copy the complete entry for both clicks and Ctrl+Shift+Up/Down, keeping the preview unchanged. Plural forms are mapped to the target locale: exact categories take precedence, default forms fall back to the source catchall, and missing non-default categories stay empty. A plain translation goes into the default plural form using the original entry as the template. Local attributes are copied; history restores keep their existing behavior.
Regression tests cover both editor paths, attributes, plural remapping, differently named catchalls and plain translations. On 4808c5a, 759 frontend tests passed (3 skipped), along with TypeScript, ESLint and formatting. The prior revision also passed the build and all 10 upstream checks; CI for the latest revision awaits maintainer approval. Backend tests were not run locally.
Please squash the commits when merging.