Patch wx overriding the correct python locale #12214

seanbudd · 2021-03-23T05:35:29Z

Link to issue number:

Summary of the issue:

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:

josephsl · 2021-03-23T05:38:03Z

Hi, can we make sure that we cover cases where folks may choose to run NVDA with a user interface language different than Windows language? This fix, if it works, will also benefit translators who are testing alpha builds. Thanks.

seanbudd · 2021-03-23T06:12:06Z

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 · 2021-03-23T06:14:31Z

Hi, and manual testing (setting NVDA interface language to Korean and English) passes. Thanks.

source/languageHandler.py

source/appModules/foobar2000.py

feerrenrut · 2021-03-23T09:53:19Z

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

feerrenrut · 2021-03-23T09:56:21Z

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 · 2021-03-24T00:42:42Z

System test failure
Build (for testing PR)

See test results for failed build of commit 5f52a76b57

seanbudd · 2021-03-24T00:52:26Z

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 · 2021-03-24T01:16:31Z

System test failure
Build (for testing PR)

See test results for failed build of commit 3d5d5fe106

seanbudd · 2021-03-24T01:22:54Z

randomly failing tests again

AppVeyorBot · 2021-03-24T02:18:28Z

System test failure
Build (for testing PR)

See test results for failed build of commit 3d5d5fe106

feerrenrut · 2021-03-24T02:46:16Z

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 · 2021-03-24T02:56:52Z

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 · 2021-03-24T08:25:01Z

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 · 2021-03-24T09:39:35Z

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 · 2021-03-24T23:20:45Z

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/languageHandler.py

tests/unit/test_languageHandler.py

source/languageHandler.py

source/core.py

seanbudd · 2021-03-26T04:29:52Z

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

feerrenrut

Looks good to me. Thanks @seanbudd

outdated

ghost · 2021-03-26T11:24:50Z

@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 · 2021-03-26T11:35:26Z

@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?

ghost · 2021-03-26T12:02:36Z

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

ghost · 2021-03-26T12:20:49Z

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

seanbudd · 2021-03-28T23:49:02Z

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.

amirsol81 · 2021-03-29T02:59:01Z

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

seanbudd · 2021-03-29T03:43:17Z

@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)

…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.

seanbudd added 3 commits March 23, 2021 16:20

set python locale with Windows System locale

1585a6d

ensure locale is set for formatting time

d426ad2

fix lint

3db9764

seanbudd added this to the 2021.1 milestone Mar 23, 2021

seanbudd self-assigned this Mar 23, 2021

seanbudd requested review from michaelDCurran and feerrenrut March 23, 2021 06:23

michaelDCurran previously requested changes Mar 23, 2021

View reviewed changes

source/languageHandler.py Outdated Show resolved Hide resolved

source/appModules/foobar2000.py Outdated Show resolved Hide resolved

seanbudd added 3 commits March 24, 2021 11:09

moved locale code out language setting

e2282d3

Merge branch 'master' into fix-12197

cf47335

reset the locale after wx breaks it

29e6a9f

seanbudd added 3 commits March 24, 2021 12:00

improve set locale function

846628a

fix lint

9e671f5

fix lint

3e33267

seanbudd changed the title ~~Set python locale to windows system locale~~ Patch wx overriding the correct python locale Mar 24, 2021