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

Patch wx overriding the correct python locale #12214

Merged
merged 28 commits into from Mar 28, 2021
Merged

Patch wx overriding the correct python locale #12214

merged 28 commits into from Mar 28, 2021

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Mar 23, 2021

Link to issue number:

Closes #12197

Summary of the issue:

#12197

Control+SHIFT+R reports the remaining time in Foobar2000, but now it doesn't work and NVDA remains silent.

Relevant log traceback

error executing script: <bound method AppModule.script_reportRemainingTime of <'foobar2000' (appName 'foobar2000', process ID 5960) at address 640d850>> with gesture 'ctrl+šift+r'
Traceback (most recent call last):
  File "scriptHandler.pyc", line 208, in executeScript
  File "appModules\foobar2000.pyc", line 80, in script_reportRemainingTime
  File "appModules\foobar2000.pyc", line 41, in parseIntervalToTimestamp
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 655, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 618, in _load_backward_compatible
  File "<frozen zipimport>", line 259, in load_module
  File "_strptime.pyc", line 268, in <module>
  File "_strptime.pyc", line 182, in __init__
  File "_strptime.pyc", line 69, in __init__
  File "_strptime.pyc", line 28, in _getlang
  File "locale.pyc", line 591, in getlocale
  File "locale.pyc", line 499, in _parse_localename
ValueError: unknown locale: hr-HR

Description of how this pull request fixes the issue:

The thread executing this did not have the correct locale set by NVDA through languageHandler.setLanguage. This is due to the latest wxPython incorrectly overriding the locale with one not supported by python.

The Windows/System language option should also call locale.setlocale in setLanguage and the locale needs to be reset after wx changes it.

Testing strategy:

Ensure that foobar2000 reports remaining time properly as per #12197 on both NVDA using the system language, and with NVDA using a language set in preferences

Add unit tests for this functionality

Known issues with pull request:

None

Change log entry:

Bug Fixes

- The python locale is set to match the language selected in preferences consistently, and will occur when using the User default language. (#12214)

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@josephsl
Copy link
Collaborator

josephsl commented Mar 23, 2021 via email

@seanbudd
Copy link
Member Author

seanbudd commented Mar 23, 2021

Hi @josephsl, the change is to make the behaviour consistent with using a language overriden by NVDA. The fix should work with both and I'll add that to the testing notes

@josephsl
Copy link
Collaborator

josephsl commented Mar 23, 2021 via email

@seanbudd seanbudd added this to the 2021.1 milestone Mar 23, 2021
@seanbudd seanbudd self-assigned this Mar 23, 2021
source/languageHandler.py Outdated Show resolved Hide resolved
source/appModules/foobar2000.py Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Contributor

Could you please confirm whether this broke as a result of: 073826a

@feerrenrut
Copy link
Contributor

Will changing the python locale affect the default formatting. It's typically important to maintain an "invariant" locale so that non-user visible parsing or formatting is predictable.

@AppVeyorBot
Copy link

See test results for failed build of commit 5f52a76b57

@seanbudd
Copy link
Member Author

Could you please confirm whether this broke as a result of: 073826a

I reproduced this on https://ci.appveyor.com/project/nvaccess/nvda/build/alpha-21882,528d5708, @josephsl noted somehwere that this was a bug on wxPython 4.1.1, looking to be fixed in wxPython 4.1.2 so I figure this is where it was likely introduced.

Will changing the python locale affect the default formatting. It's typically important to maintain an "invariant" locale so that non-user visible parsing or formatting is predictable.

The locales were already being set by wx, and previously they were being set when using a custom language. The python locale does affect default formatting, such as how time and dates are reported. This should just be fixing a regression though, not changing any behaviour.

@AppVeyorBot
Copy link

See test results for failed build of commit 3d5d5fe106

@seanbudd
Copy link
Member Author

seanbudd commented Mar 24, 2021

randomly failing tests again

@seanbudd seanbudd changed the title Set python locale to windows system locale Patch wx overriding the correct python locale Mar 24, 2021
@AppVeyorBot
Copy link

See test results for failed build of commit 3d5d5fe106

@feerrenrut
Copy link
Contributor

I reproduced this on https://ci.appveyor.com/project/nvaccess/nvda/build/alpha-21882,528d5708,

Ok, and commit 073826a was build alpha-22006,073826ae

@seanbudd
Copy link
Member Author

I reproduced this on https://ci.appveyor.com/project/nvaccess/nvda/build/alpha-21882,528d5708,

Ok, and commit 073826a was build alpha-22006,073826ae

Yes, the alpha build I tested it on is from earlier than 073826a and the issue was still present, so it wasn't introduced by 073826a

@feerrenrut
Copy link
Contributor

Yep, I intended my last comment as a confirmation, but I can see how it could be read in other ways. Apologies for that. The number before the comma is the build number, which is incremental and provided by appveyor. The second part is the SHA.

@CyrilleB79
Copy link
Collaborator

I do not know if it is relevant here, but just asking. Did you test the 2 following cases:

  • run NVDA in a language that is not available in Windows (e.g. Aragonese); this had caused specific issues in the past.
  • run NVDA on a Windows locale for which no translation exists; probably Estonian would suit.

@seanbudd
Copy link
Member Author

Yep, I intended my last comment as a confirmation, but I can see how it could be read in other ways. Apologies for that. The number before the comma is the build number, which is incremental and provided by appveyor. The second part is the SHA.

Oh right, I hadn't thought of comparing the build numbers. Good to know, I was checking the commit log

source/core.py Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member Author

seanbudd commented Mar 26, 2021

@kvark128 would you be able to confirm #12160 is fixed with installer artifact from the latest build on this PR? download link

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @seanbudd

@kvark128
Copy link
Contributor

@kvark128 would you be able to confirm #12160 is fixed with installer artifact from the latest build on this PR? download link

No, in this build, the problem with the Russian locale remains. NVDA still crashes at startup.

@dpy013
Copy link
Contributor

dpy013 commented Mar 26, 2021

@kvark128 would you be able to confirm #12160 is fixed with installer artifact from the latest build on this PR? download link

No, in this build, the problem with the Russian locale remains. NVDA still crashes at startup.
Can you provide the logs?

@kvark128
Copy link
Contributor

It seems that NVDA itself now sets the locale incorrectly.
nvda-old.log

@kvark128
Copy link
Contributor

If explicitly set "language = ru" in the config file, then NVDA also crashes, but later, after creating wx.App.
nvda.log

@seanbudd
Copy link
Member Author

It seems that NVDA itself now sets the locale incorrectly.
nvda-old.log

If you are using 'User Default' as your language, that appears to be intended behaviour.

@kvark128 would you be able to confirm #12160 is fixed with installer artifact from the latest build on this PR? download link

No, in this build, the problem with the Russian locale remains. NVDA still crashes at startup.

If explicitly set "language = ru" in the config file, then NVDA also crashes, but later, after creating wx.App.

Strange, I still can't reproduce this. I've removed the references to that issue from this PR, to investigate separately.

@seanbudd seanbudd merged commit d9ccf26 into master Mar 28, 2021
@seanbudd seanbudd deleted the fix-12197 branch March 28, 2021 23:50
@amirsol81
Copy link

@seanbudd In case it's useful, #12197 still persists.

@seanbudd
Copy link
Member Author

@amirsol81 is that using the build artifact from this branch or alpha? An alpha with these changes has not been published yet - see #12197 (comment)

seanbudd pushed a commit that referenced this pull request Aug 26, 2021
…itializing language. (#12753)

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:

We are Windows only so it is important to set locales to something reasonable
If the locale that is unknown to Windows is set some versions of CRT assumes that the code page of such locale is Utf8 which causes a crash when using time.localtime (that was the underlying cause of Crash of the latest alpha NVDA builds on a system with Russian locale #12160)
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:

Aragonese - locale set to Windows locale
German_Switzerland and Hindi - made sure that Crash of the latest alpha NVDA builds on a system with Russian locale #12160 has not been reintroduced
Executed unit tests on all above mentioned versions of Windows made sure they pass.
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.

NVDA's reporting of the remaining time is broken in Foobar2000
9 participants