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

Ensure that NVDA sets Python locale to a valid Windows locale when initializing language. #12753

Merged
merged 15 commits into from
Aug 26, 2021

Conversation

lukaszgo1
Copy link
Contributor

@lukaszgo1 lukaszgo1 commented Aug 19, 2021

Link to issue number:

Fix-up of #12214
Follow up to #12250
Fixes root cause of #12160

Summary of the issue:

After NVDA sets its interface language it also attempts to set Python locale to the current language. Before PR #12214 we were just passing the current language code to locale.setlocale and catching all possible exceptions. Unfortunately Python setlocale is pretty useless on Windows as it tries to normalize the locale name according to the Unix standards (see Python issue 37945. When we were using wxPython older than 4.1 this was generally not an issue (except for some specific locales under a particular configurations such as one described in #12160 ) since wxPython was setting locale to a valid one so the problem remained unnoticed. This broke with wxPython 4.1 and the fix was attempted in #12214 but it still was setting Python locale to Unix locales. This is causing two problems:

Description of how this pull request fixes the issue:

Since Python locale.setlocale and locale.getlocale cannot be trusted on Windows the approach I took avoids all the normalization they offer by creating a valid Windows locale string using getLocaleInfoEx in a form which Python no longer normalizes. This string is then passed to locale.setlocale. The new functionality has pretty solid unit test coverage, previous unit tests for languageHandler are also updated to account for these changes. As suggested during the review this PR also consolidates all NLS constants we use when retrieving various locale related information from Windows into an enum in languageHandler. The old constants are deprecated and would no longer be available in 2022.1.
Specific fix for #12160 from #12250 cannot be removed because even though we're setting only complete locales i.e. with a code page and for languages such as Russian or German_Switzerland this crash would no longer occur NVDA supports some locales such as Hindi for which code page is indeed Utf8 and they would still cause a crash.

Testing strategy:

Manual testing:
On Windows 7, Server 2012, Windows 10 1809 (on which I was previously able to reproduce #12160 ) and latest preview of Windows 11 started NVDA with both specific language set in preferences and with the language set to default. Made sure that when 'user default' is selected appropriate language is chosen and locale (as retrieved by locale.setlocale(locale.LC_ALL)) is set into a locale of a form englishLanguageName_englishCountryName.ANSICodePage. Repeated these tests with some languages known to cause troubles in the past:

Executed unit tests on all above mentioned versions of Windows made sure they pass.

Known issues with pull request:

None known

Change log entries:

Bug fixes:

  • NVDA no longer sets invalid Python locales

Changes for developers:

  • LOCALE_SLANGUAGE, LOCALE_SLIST and LOCALE_SLANGDISPLAYNAME are moved to the LOCALE enum in languageHandler. They are still available at the module level but deprecated and would be removed in NVDA 2022.1

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@lukaszgo1
Copy link
Contributor Author

@seanbudd Given your work on #12214 you may be interested in taking a look at this.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @lukaszgo1 for your impressive work on this.

I've left some notes on code styling, and requested some additional tests, but the logic here looks solid.
Once we can get confirmation that this works for users on other systems, as requested here, I think this can be merged. If this can't be done in a reasonable timeframe, we can move forward to alpha testing.

source/languageHandler.py Show resolved Hide resolved
source/languageHandler.py Show resolved Hide resolved
source/languageHandler.py Show resolved Hide resolved
source/languageHandler.py Show resolved Hide resolved
source/languageHandler.py Outdated Show resolved Hide resolved
source/languageHandler.py Show resolved Hide resolved
source/languageHandler.py Outdated Show resolved Hide resolved
source/languageHandler.py Outdated Show resolved Hide resolved
source/languageHandler.py Outdated Show resolved Hide resolved
@lukaszgo1
Copy link
Contributor Author

@seanbudd I believe all your review comments are now addressed. I've simplified the implementation of setLocale though had to do so differently than you've suggested since after applying your suggestions Linter started to complain about it being too complex. Contrary to my initial belief I also had to keep the code from #12250 - the updated PR description explains in which cases it is necessary. The flip side of this is that it is now much safer to merge this into Alpha.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the enum and deprecations and pointing out the tests I missed. This looks almost ready to merge.

I've requested some further changes to setLocale to make it easier to visually parse. I'm not sure if this sort of code-styling is helpful for vision impaired developers - if it is detrimental I am happy to ignore it.

source/languageHandler.py Outdated Show resolved Hide resolved
source/languageHandler.py Outdated Show resolved Hide resolved
source/languageHandler.py Outdated Show resolved Hide resolved
source/logHandler.py Outdated Show resolved Hide resolved
@lukaszgo1
Copy link
Contributor Author

@seanbudd All addressed.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @lukaszgo1, this change will be much appreciated

seanbudd pushed a commit that referenced this pull request Sep 13, 2021
…rent than the one used for building NVDA (#12835)

Fixes regression from #12617. Discovered when working on #12753 as that PR required executing unit tests under various versions of Windows

Summary of the issue:
It is sometimes useful to execute unit tests under various versions of Windows where it is impractical, or even impossible to have full build environment for NVDA. With current master some tests are failing because when importing COM interfaces comtypes complains about them being created on different version of Windows. Before PR #12617 this worked by chance since our monkey patches for compypes were applied when importing core and it just so happened that core was imported before any COM interface was.

Description of how this pull request fixes the issue:
Since we have monkey patch which stops comtypes from complaining about typelib being different than COM interface it has been applied to comtypes before tests starts. To avoid copying code of this monkey patch it was necessary to refactor our monkey patches to make each of them into a separate function - that allows us to apply them selectively rather than just all of them during import.

Testing strategy:
Ensured that unit tests pass under a never version of Windows than the one under which virtual environment for NVDA has been created
Ensured that comtypes monkey patches are properly applied for NVDA in particular that core.CallCancelled is raised when appropriate.
seanbudd pushed a commit that referenced this pull request Sep 13, 2021
…nguage in core.main (#12837)

Follow up from #12753

Issues:
- wx.Locale object was called locale effectively shadowing built-in Python module. 
While normally there is no reason to have locale imported in that scope if it is necessary for debugging the name is rather unfortunate.
- exception handling for importing languageHandler and setting language was too broad hiding eventual issues.

Fix:
- locale was renamed to wxLocaleObj.
- We're no longer catching any exceptions when importing languageHandler and setting language - if any of these fails NVDA is non functional and the exception should propagate.
michaelDCurran pushed a commit that referenced this pull request Nov 21, 2021
…er after abandoned #10089) (#12958)

Fixes #10044
Supersedes #10089

Summary of the issue:
It can sometimes be useful to specify NVDA's language from the CLI. One use case which has been mentioned in #10044 is when language has been accidentally changed to a foreign one. This can also be useful for developers (it would certainly be a welcome addition for me when working on #12250 and #12753).

Description of how this pull request fixes the issue:
Add a new --lang command line parameter.
It accepts either "Windows" or the usual "en" / "de_CH" codes, with a little tolerance regarding to case and dashes.
As discussed during the review in #10089 the parameter is treated as a temporary override (i.e. it does not affect configuration at all and is effective only until NVDA is restarted).
As a bonus this PR adds unit tests for languageHandler.normalizeLanguage and languageHandler.getAvailableLanguages (tests for the former were necessary to confirm it provides us with normalization we need, and since I had to split the latter one into two functions added tests made it possible to confirm that current behavior has not changed).
seanbudd pushed a commit that referenced this pull request Dec 8, 2021
Removes code marked as deprecated in #12753

Summary of the issue:
As part of #12753 various constants in languageHandler were moved into an enum but kept at the module level for backwards compatibility. Since NVDA 2022.1 is a backwards compatibility breaking release it makes sense to remove them.

Description of how this pull request fixes the issue:
Deprecated constants are removed.
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.

None yet

3 participants