Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not ignore locale CLDR file when locale symbol file is missing #14433

Merged
merged 4 commits into from
Dec 13, 2022

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Dec 9, 2022

Link to issue number:

Found while investigating #14417; it does not fix it but it is related.

Summary of the issue:

In NVDA some languages do not have a symbol.dic file but have a cldr.dic file. Today, these are the following languages: 'af_ZA', 'am', 'as', 'gu', 'id', 'kok', 'ml', 'mni', 'ne', 'te', 'th', 'ur'.

For these languages the symbols belonging to CLDR file (e.g. "☺") are reported in English rather than in the locale language.

This is because we detect if a language has no symbol file and in this case, we store it in _noSymbolLocalesCache and raise a LookupError in the future, even if the language has a CLDR file.

Description of user facing changes

Languages with no symbol file will be able to report CLDR in the locale language if available.

Description of development approach

Use two caches, one for languages with no symbol file and one for languages with no CLDR file.

Testing strategy:

  • Wrote two unit tests
    • Confirm the current issue with commit e38fe30 (before the fix), i.e. test_processSpeechSymbol_withoutSymbolFile fails.
    • Upon commit e38fe30 (before the fix), checked that the languages in test_processSpeechSymbol_withoutSymbolFile report the CLDR in English by modifying locally line 157 to
      self.assertEqual(
      and running successfully the unit tests
    • Checked after the fix (commit ba01bc8) that unit tests pass.
  • Manual tests:
    • Checked with eSpeak Amharic that "☺" is reported in the locale language and no more in English. Note: I do not know this language, so I have just checked that the symbol and its description in cldr.dic sound the same with eSpeak.

Known issues with pull request:

CLDR symbols are still reported in English with eSpeak for Afrikaans and Portuguese (Portugal). This is because:

  • eSpeak use 'af' and 'pt' (no country) for these languages
  • but in NVDA we cannot fallback to these languages, we only have 'af_ZA' on one side and 'pt_PT' or 'pt_BR'.

Since the solution to this issue is not trivial, I prefer discuss it in a separate issue and have the issue already fixed for the other languages. I guess that the issue would also be fixed for OneCore since I think that these voices are country-aware.

I have opened #14434 to track this specific issue with eSpeak Portuguese (Portugal) and Afrikaans.

Change log entries:

Bug fixes
Emojis should now be reported in more languages. (#14433)

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review December 10, 2022 10:05
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner December 10, 2022 10:05
@AppVeyorBot
Copy link

See test results for failed build of commit e5f8538e5e

@michaelDCurran michaelDCurran merged commit 06a671d into nvaccess:master Dec 13, 2022
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Dec 13, 2022
@CyrilleB79 CyrilleB79 deleted the missingSymbols branch December 14, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants