Fix up config upgrade steps from schema version 14 to 15

[SaschaCowley]

Link to issue number:

Fixes #17637(?)
Follow-up to #17505

Summary of the issue:

The config upgrade steps for moving from version 14 to 15 of the config schema may leave the user's config in an invalid state if either of ["keyboard"]["speakTypedCharacters"] or ["keyboard"]["speakTypedWords"] is not a boolean.
This is because the upgrade steps, as implemented, take no action if those values are not bools.
For most upgrade steps, this approach is perfectly valid.
However, version 15 of the schema expects these values to be integers, so validation of the upgraded config will fail.

These config upgrade steps also upgrade a value of True to a value of TypingEcho.EDIT_CONTROLS, even though the behaviour of True is the same as that of TypingEcho.ALWAYS.

Description of user facing changes

The configuration upgrade should not corrupt the user's config.

If the user has changed the behaviour of either of these options, the behaviour will not change for them.

Description of development approach

Instead of just returning when we get a ValueError in upgradeConfigFrom_14_to_15, delete the offending config entry.

Change the mapping for True to TypingEcho.ALWAYS when converting from an existing boolean to a new integer.

Testing strategy:

Tested by creating valid and invalid config strings, instantiating ConfigObjs with them, and feeding those to upgradeConfigFrom_14_to_15.

Known issues with pull request:

A user may lose their setting for "Speak typed characters" or "Speak typed words", if configobj doesn't recognise it as a boolean.
Theoretically this shouldn't be possible, as the config wouldn't have validated under any of the prior schema versions.
Nevertheless this seems to have happened, and it is better to lose (at most) 2 settings than all settings.

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

@coderabbitai summary

[SaschaCowley]

Added blocked/needs-info until we know more what's going on in #17637.

[SaschaCowley]

@cary-rowen Pinging you as this is based on your work :)

[cary-rowen]

Thanks @SaschaCowley I haven't figured out the real cause of #17637. But sorry about that!

[CyrilleB79]

Here is my feedback.

In addition, I notice that upgradeConfigFrom_12_to_13 uses the same pattern. It has probably not caused any issue since it has already made its way in a stable release.

Since the new upgrade function had probably been copied from upgradeConfigFrom_12_to_13, would it be worth to also modify it so that we do not copy this pattern anymore?

[SaschaCowley]

@CyrilleB79, upgradeConfigFrom_12_to_13 does not use the same pattern. This class of issue relies on the config path remaining the same, but the type changing. If a setting is being created at a new config path from one at an old config path, and the value at the old config path is not recognised by the upgrade step , this can be ignored, as the old path will not be in the config schema, so its type will not matter when NVDA validates the config.