Skip to content

make sure About dialog text is shown localized - #7773

Merged
michaelDCurran merged 2 commits into
rcfrom
i7770
Nov 22, 2017
Merged

make sure About dialog text is shown localized#7773
michaelDCurran merged 2 commits into
rcfrom
i7770

Conversation

@michaelDCurran

@michaelDCurran michaelDCurran commented Nov 18, 2017

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #7770

Summary of the issue:

The versionInfo module contains some localized strings at module level. If this module is imported too early, these strings will not be localized correctly. Commit 1fcc7fe added the import of versionInfo to the top of logHandler, thereby causing this issue to occur.
logHandler required versionInfo.version to calculate the correct path for nvdaHelperRemote.dll for remote logging.
As versionInfo was imported before languageHandler was initialized, text such as the message in the About dialog was always displayed in English.

Description of how this pull request fixes the issue:

This PR splits out non-localizable version data from versionInfo into a new buildVersion module. This allows importing of buildVersion in places where the version is needed before languageHandler is initialized. Specifically, this is now used in logHandler to stop versionInfo being accidentally imported too early.
Note that the content of the buildVersion module is imported into versionInfo, so versionInfo can continue to be used as normal where ever it is safe to do so. Use buildVersion directly for code that could be run before languageHandler is initialized.

Testing performed:

  • Run as source, switching language to French and ensuring the About dialog message is displayed in French. It did.
  • Tried installing over the top of another copy that is still running so that it deliberately fails and logs a message from the installer via remote logging. It did.
  • Closed the old copy and retried the install. It succeeded.
  • Run the installed copy, switching the language to French and ensuring the text in the About dialog was displayed in French. It was.

Known issues with pull request:

None

Change log entry:

Changes from rc1 to rc2:

  • The text in NVDA's About Dialog is again correctly shown in the user's configured language.

…ildVersion module. This allows importing of buildVersion in places where the version is needed before languageHandler is initialized. Specifically, this is now used in logHandler to stop versionInfo being accidentally imported too early.

@feerrenrut feerrenrut 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.

Looks good. Just a couple of suggestions for doc strings. I'll approve this and leave it up to you whether to take those suggestions on board.

Comment thread source/versionInfo.py
@@ -5,43 +5,9 @@
#See the file COPYING for more details.

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.

Perhaps a module level doc string to say "This module is for UI specific / translatable version information. The source of this information should come from the non-translated buildVersion module so that it can be used in parts of nvda that are executed pre-translation system initialisation."

Comment thread source/buildVersion.py
#Copyright (C) 2006-2017 NV Access Limited
#This file is covered by the GNU General Public License.
#See the file COPYING for more details.

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.

I would suggest some module level doc string to indicate that "this module is for non-localised version information. It should not contain UI strings or translatable strings. The versionInfo module is the correct place for that"

@michaelDCurran
michaelDCurran merged commit 07e74bd into rc Nov 22, 2017
@nvaccessAuto nvaccessAuto added this to the 2017.4 milestone Nov 22, 2017
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.

3 participants