Skip to content

Remove windowsPrimaryLCIDsToLocaleNames#13342

Merged
seanbudd merged 9 commits into
masterfrom
fixLCIDLookup
Feb 17, 2022
Merged

Remove windowsPrimaryLCIDsToLocaleNames#13342
seanbudd merged 9 commits into
masterfrom
fixLCIDLookup

Conversation

@seanbudd

@seanbudd seanbudd commented Feb 16, 2022

Copy link
Copy Markdown
Member

Link to issue number:

None

Summary of the issue:

The latest PR (#13338) to merge beta to master has failed, due to Bangla translations being introduced and windowsPrimaryLCIDsToLocaleNames not listing the locale. AppVeyor build failure.

On #13339, @CyrilleB79 raised that this variable is poorly documented and outdated.
The initial variable provided a mapping of LCIDs to language codes, without the rest of the locale.
We now normalize the locale as necessary in normalizeLanguage instead.
On further inspection, it appears that the only additions from locale.windows_locale are as follows:

{
	1170: 'ckb',
	1109: 'my',
	1143: 'so',
+      2117: 'bn',
	9242: 'sr',
}

As locale.windows_locale is incomplete, these were introduced to ensure languages translated in NVDA could be mapped.
Instead, the Windows function LCIDToLocaleName can be used to get each of these locales.
This was suggested in #4203.

However Windows maps 1170 to "ku-Arab-IQ" not "ckb", and a translation is added for Central Kurdish in localesData.LANG_NAMES_TO_LOCALIZED_DESCS["ckb"]. NVDA may drop "Arab-IQ" from this locale to get the language, losing the locality of "Central Kurdish".

Description of how this pull request fixes the issue:

Removes windowsPrimaryLCIDsToLocaleNames, instead use LCIDToLocaleName after checking for an internal mapping (eg for "ckb").

Testing strategy:

  • Smoke test setting Windows locale in NVDA as an English user (still mapped with locale.windows_locale)
  • Smoke test setting English locale in NVDA (still mapped with locale.windows_locale)
  • Smoke test setting ckb Kurdish locale in NVDA (hardcoded mapping)
  • Smoke test setting my locale in NVDA (previously hardcoded mapping)
  • Confirm beta builds successfully with this code merged: https://ci.appveyor.com/project/NVAccess/nvda/builds/42590231
  • Smoke test (on alpha?) with various Windows locales

Known issues with pull request:

None

Change log entries:

For Developers

- ``languageHandler.windowsPrimaryLCIDsToLocaleNames`` has been removed, instead use ``languageHandler.windowsLCIDToLocaleName`` or ``winKernel.LCIDToLocaleName``

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

Comment thread source/winKernel.py
@seanbudd seanbudd requested a review from feerrenrut February 16, 2022 02:57
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit b344b8b2bf

Comment thread source/languageHandler.py
Comment thread source/languageHandler.py Outdated
Comment thread source/winKernel.py Outdated
# Conflicts:
#	user_docs/en/changes.t2t
@seanbudd seanbudd requested a review from feerrenrut February 16, 2022 22:30

@lukaszgo1 lukaszgo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for applying these changes @seanbudd ! LGTM

Comment thread source/winKernel.py
@seanbudd seanbudd merged commit ba11e8a into master Feb 17, 2022
@seanbudd seanbudd deleted the fixLCIDLookup branch February 17, 2022 22:49
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 17, 2022
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.

6 participants