Fix up config upgrade steps from schema version 14 to 15#17642
Conversation
|
Added blocked/needs-info until we know more what's going on in #17637. |
|
@cary-rowen Pinging you as this is based on your work :) |
|
Thanks @SaschaCowley I haven't figured out the real cause of #17637. But sorry about that! |
CyrilleB79
left a comment
There was a problem hiding this comment.
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?
|
@CyrilleB79, |
) Fixes nvaccess#17637 Follow-up to nvaccess#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. Description of user facing changes The configuration upgrade should not corrupt the user's config. Description of development approach Instead of just `return`ing when we get a `ValueError` in `upgradeConfigFrom_14_to_15`, delete the offending config entry. Testing strategy: Tested by creating valid and invalid config strings, instantiating `ConfigObj`s 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.
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
Trueto a value ofTypingEcho.EDIT_CONTROLS, even though the behaviour ofTrueis the same as that ofTypingEcho.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 aValueErrorinupgradeConfigFrom_14_to_15, delete the offending config entry.Change the mapping for
TruetoTypingEcho.ALWAYSwhen 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 toupgradeConfigFrom_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:
@coderabbitai summary