Avoid a situation where no NVDA modifier key is defined

[CyrilleB79]

Link to issue number:

Fixes #14527

May be considered a follow-up of #14233.

Summary of the issue:

When configuring NVDA Modifier Key for a non-default profile and for the default profile, we can end-up with no NVDA key defined at all in the non-default profile (see #14527 for detailed steps).

This is due to the fact that the config stores one item per NVDA key but that there is a dependency between these values to honor the requirement that at least one key should be defined as NVDA key. These values are checked when changing NVDA keys via the GUI. But the requirement may be missed when switching profiles and stacking their configurations.

There is also a second scenario allowing to have no NVDA key defined using only default profile:

• In keyboard settings enable only caps lock and disable both insert, then validate
• Open the NVDA startup dialog via Help menu, uncheck caps lock and validate.
  This scenario is quite unlikely but there is no check in NVDA startup dialog to prevent such situation.

Description of user facing changes

Situations where no NVDA key is defined will not be possible anymore.

Description of development approach

• Use only one config item to store the used NVDA modifier keys.
• Define a config flag for NVDA key; each value corresponds to one bit so that all the values can be bitwise or-ed and the reesulting value can be stored in the config as an integer.

During the dev, I have had to import key labels (via the config flag) early during NVDA startup. This has caused issues since pgettext was not yet defined at this moment. I have tried to fix this; but please double-check it because I do not know very well this part of the code and why gettext translations need to be initialized two times during NVDA startup.

Note: with this PR, keyboardHandler.SUPPORTED_NVDA_MODIFIER_KEY is not used anymore in NVDA's code. Should I deprecate it?

Testing strategy:

Manual tests:

• Check config upgrade in nvda.ini
• Modify NVDA modifier keys in keyboard parameters
• Enable or not caps lock as NVDA modifier in NVDA startup dialog
• Test scenario of Possibility to have no more NVDA key defined #14527
• Test 2nd scenario with startup dialog described in this PR.

Unit tests:

• New unit tests created to check various config upgrade situations

Known issues with pull request:

None

Change log entries:

Bug fixes
When configuring NVDA it will be ensured that at least one key is defined as NVDA key in any situation. (#14527)

Deprecation:
keyboardHandler.SUPPORTED_NVDA_MODIFIER_KEYS is deprecated with no direct replacement. Consider using the class config.configFlags.NVDAKey instead. (#14528)

API breaking changes:

• In the configuration, the following keys have been removed from the [keyboard] section: useCapsLockAsNVDAModifierKey, useNumpadInsertAsNVDAModifierKey, useExtendedInsertAsNVDAModifierKey. They are replaced by NVDAModifierKeys. (Avoid a situation where no NVDA modifier key is defined #14528)

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.

[AppVeyorBot]

• Unable to install NVDA prior to tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/4nub9r3kpjvej1sq/artifacts/output/nvda_snapshot_pr14528-27483,e3c1c5a5.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 23.4,
  TESTSETUP_START 0.0,
  FINISH_END 3.1

See test results for failed build of commit e3c1c5a506

[AppVeyorBot]

• Unable to install NVDA prior to tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ii15mg2al257r1ni/artifacts/output/nvda_snapshot_pr14528-27484,2f93e756.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 24.2,
  TESTSETUP_START 0.0,
  FINISH_END 3.1

See test results for failed build of commit 2f93e756b8

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ldj5cy4ifts8ssmk/artifacts/output/nvda_snapshot_pr14528-27489,7ac0f5ad.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 24.1,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 20.2,
  FINISH_END 0.1

See test results for failed build of commit 7ac0f5ad22

[CyrilleB79]

@seanbudd I have addressed all the review comments.

However I have raised a new question while addressing the review. Please answer before merging. Thanks.

[seanbudd]

Note: with this PR, keyboardHandler.SUPPORTED_NVDA_MODIFIER_KEY is not used anymore in NVDA's code. Should I deprecate it?

Yes, this is a good idea. Suggest NVDAKey as a replacement

[seanbudd]

Let me know if I missed any other questions

[CyrilleB79]

Let me know if I missed any other questions

Yes please, your feedback to this comment in nvda.pyw:

While working on the review in nvda_slave.pyw, I realize that the install method does not take any parameter there, wherease here in nvda.pyw, it takes True as parameter. This was already the case before this PR.

The help on this install method is as follows:

>>> help(trans.install)
Help on method install in module gettext:

install(names=None) method of gettext.GNUTranslations instance

And the documentation indicates:

install(names=None)
This method installs gettext() into the built-in namespace, binding it to _.
If the names parameter is given, it must be a sequence containing the names of functions you want to install in the builtins namespace in addition to _(). Supported names are 'gettext', 'ngettext', 'lgettext' and 'lngettext'.

This True parameter does not make sens to me. Is it a bug or something left over after a change in the signature of the install method?

Should I remove this parameter? If yes, should I do it here or in a separate PR (in case this causes unexpected issue and need to be reverted)?

[seanbudd]

I agree, remove the parameter in this PR.

[CyrilleB79]

I have addressed the parameter issue and implemented the deprecation. I have also updated the initial description's change log with the deprecation info.

The PR is ready to review again.

[seanbudd]

Thanks, LGTM