Skip to content

Don't instal gettext and pgettext before initializing languageHandler#14677

Merged
seanbudd merged 1 commit into
nvaccess:masterfrom
lukaszgo1:noGettextBeforeLanguageHandlerInit
Mar 2, 2023
Merged

Don't instal gettext and pgettext before initializing languageHandler#14677
seanbudd merged 1 commit into
nvaccess:masterfrom
lukaszgo1:noGettextBeforeLanguageHandlerInit

Conversation

@lukaszgo1

Copy link
Copy Markdown
Contributor

Link to issue number:

Related to #14660

Summary of the issue:

When starting NVDA installs a 'fake' gettext and pgettext functions which translates to the language set as the current Windows locale. Later in the startup process languageHandler overwrites these installed translation functions with a new one which translate strings to the language chosen by the user (either in the configuration or from the CLI). Unfortunately if there is a module with translatable strings at its top level and it is imported before languageHandler is initialized these strings are either translated to the default system language, or for locales for which Python's locale.getdefaultlocale fails they remain in English. Similar problems were reported in #7770 and in #14657. Both of these are fixed, but the main problem is that bugs like this are difficult to identify before the release, since most contributors either use both NVDA and Windows in English, or at least their NVDA is in the same language as their operating system.

Description of user facing changes

This should not have any user visible impact.

Description of development approach

When starting NVDA or nvda_slave gettext and pgettext are no longer installed before initializing languageHandler.

Testing strategy:

Made sure NVDA starts - this require a longer period of testing in Alpha anyway.

Known issues with pull request:

None known

Change log entries:

None needed - this has no impact for add-ons developers since add-ons were initialized after languageHandler.

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.

@lukaszgo1 lukaszgo1 requested a review from a team as a code owner February 27, 2023 02:45
@lukaszgo1 lukaszgo1 requested a review from seanbudd February 27, 2023 02:45
@CyrilleB79

Copy link
Copy Markdown
Contributor

What about keeping these functions as a fallback but logging an error instead (log.error)? This would allow to avoid NVDA not starting at all in case such error is encountered.

Of course, the log should be delayed in some way because the log handler may not be initialized when the fake translation (or rather fallback translation) is called.

@lukaszgo1

Copy link
Copy Markdown
Contributor Author

What about keeping these functions as a fallback but logging an error instead (log.error)? This would allow to avoid NVDA not starting at all in case such error is encountered.

Of course, the log should be delayed in some way because the log handler may not be initialized when the fake translation (or rather fallback translation) is called.

This would add quite a lot of complexity for very little benefit IMHO.
I see three scenarios worth considering for this work:

  1. When implementing a new feature developer mistakenly imports a module with localized variables at the top level before initializing translations - in that case NVDA failing to start rather than logging an error is a good thing.
  2. In some scenario which we haven't tested before merging this PR NVDA imports a module with translatable variables - if it fails in Alpha NVDA failing to start is an acceptable outcome and users tent to ignore errors in the log anyway.
  3. For stable NVDA errors in the log aren't visible for most users, - while NVDA failing to start is not ideal we would at least know that something has failed.

@CyrilleB79

Copy link
Copy Markdown
Contributor

@lukaszgo1 your reasoning makes sense and, at the end, I agree with you.

@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Mar 2, 2023
@seanbudd seanbudd merged commit 9ea9af6 into nvaccess:master Mar 2, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Mar 2, 2023
@lukaszgo1 lukaszgo1 deleted the noGettextBeforeLanguageHandlerInit branch March 2, 2023 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-early Merge Early in a developer cycle

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants