Add support for ngettext

CyrilleB79 commented 1 year ago

However, it seems that our PO validation code in the translation system is not handling plural forms well, and is incorrectly classing translations as missing brace format variables, as it is incorrectly comparing the translation with the message before that one. It then outputs the following error: Message starting on line 7169 Original: "{startTime} to {endTime}" Translated: "category {categories}" Error: missing brace format interpolation, extra brace format interpolation Expected: these brace format interpolations: {endTime}, {startTime} Got: these brace format interpolations: {categories}

Changes in this PR

This PR adds the support of ngettext messages in the PO validation script.

Tests

Test 1

Run new scripts/poChecker.py on various files:

A complete recent (French) translation file with no issue: nvda_OK.po.txt
A complete file with a plural string translated and no translation issue: nvda-61403_OK_plural3_translated.po.txt
A file with plural string untranslated: nvda-61397_NOK_plural2_untranslated.po.txt

The files containing the plural string have been taken from SVN SRT repo when a first attempt to introduce ngettext in NVDA was made.

Test 2

Comparison of the output of the modified script with the output of the original one. The comparison has been made on the nvda.po files of all languages available in SVN repo screenreadertranslations (commit 74391) as well as with all the nvda.po files available in the last master version of NVDA's Git repo (commit 96764959510a19d390f4493b78510df3a353cd50). The comparison has been made thanks to a test script made for this task.

Some diffs have been found in the order in which the placeholders are listed in the report, e.g.:

-Expected: these brace format interpolations: {hours:d}, {minutes:d}
+Expected: these brace format interpolations: {minutes:d}, {hours:d}

The placeholders are stored in a Python set in scripts/poChecker.py, thus their order is not significant. I do not know however why the order has changed from one version of scripts/poChecker.py to another. Thus I have amended my test script to reorder alphabetically the placeholders in the output of scripts/poChecker.py so that we can ignore the order of placeholders on a same line. After this reordering post-processing, there was no diff between the outputs of the old and the new version of the script.

For reference here is the (quick and dirty) test script that I have used (updated and re-tested on June 9 2023): launch_poChecker.py.txt

CyrilleB79 commented 1 year ago

@michaelDCurran and @seanbudd: If possible for you, I would see the following planning regarding ngettext introduction in NVDA:

Before 2023.2 beta phase:
- (You) Review and merge this PR
- (I) Submit a PR against NVDA to introduce a limited subset of ngettext usage, e.g. 2 or 3 occurrences on strings frequently used
- (You) Merge this last PR (limited usage of ngettext)
During 2023.2 beta phase:
- Check with translators that there is no issue using the translation framework
In NVDA 2023.2beta2 or higher (2023.2 rc or stable)
- Check with users that the translated strings of the test subset are reported as expected in all languages
During 2023.3 alpha phase:
- (I) Submit another PR with the remaining strings requiring the use of ngettext. NVDA 2023.3 would thus be the first version with full usage of ngettext.

Note: I may also separate simple cases of ngettext usage and more complex ones (e.g. strings containing two numbers) as explained in https://github.com/nvaccess/nvda/issues/12445.

CyrilleB79 commented 1 year ago

@seanbudd, @michaelDCurran: Now that add-on store work has been merged and before beta phase, would you provide a feedback on this planning? Thanks.

michaelDCurran commented 1 year ago

We'll talk about this internally early next week and get back with a decision on if we want to proceed this way. Thanks for your ideas and work on this.

seanbudd commented 1 year ago

The plan looks good to us, feel free to go ahead with it

CyrilleB79 commented 1 year ago

Thanks @seanbudd. In this case, the first step is on your side: review and merge the current PR.

nvaccess / mrconfig