Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FeatureFlag: fix changing from default value and value of default #14133

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Sep 13, 2022

Link to issue number:

None

Summary of the issue:

Feature flags use a default option that results in a default value.
For example, for a boolean feature flag could have three options: Default (Enabled), Enabled, Disabled.
When comparing feature flags __eq__ is overridden such that Default (Enabled) == Enabled.

NVDA checks if a value has changed before marking a profile as dirty so that it is written to disk when saving the configuration.
Due to __eq__ being overridden, NVDA does not write changes to disk if you change from the default value to the value of default.

STR:

  1. With NVDA running, open braille preferences
  2. Update "Interrupt Speech While scrolling" from Default (Enabled) to Enabled
  3. Save the settings and exit the settings dialog
  4. Re-open braille preferences, note the setting is still set to Default (Enabled)

Description of user facing changes

NVDA correctly updates feature flags when changing from the default value to the value of default.

Description of development approach

When checking if a config setting has changed, compare non-sections as strings, as this is how they are written to the config profile.

Testing strategy:

Manually test STR

Unit testing has been added for AggregatedSection

Known issues with pull request:

AggregatedSection should be moved in a separate PR to the new module.

Change log entries:

Bug fixes:

  • Fixed an issue where the config did not save correctly when changing between a "Default" option and the value of the "Default" option.

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.

@seanbudd seanbudd requested a review from a team as a code owner September 13, 2022 00:14
@seanbudd seanbudd requested a review from feerrenrut September 13, 2022 00:14
@seanbudd seanbudd self-assigned this Sep 13, 2022
@AppVeyorBot
Copy link

See test results for failed build of commit 77b915a162

@seanbudd seanbudd marked this pull request as draft September 15, 2022 07:59
@seanbudd seanbudd marked this pull request as ready for review September 20, 2022 04:01
@seanbudd seanbudd requested a review from feerrenrut September 20, 2022 05:08
@seanbudd seanbudd marked this pull request as draft September 20, 2022 05:46
@seanbudd seanbudd marked this pull request as ready for review September 21, 2022 05:39
@AppVeyorBot
Copy link

See test results for failed build of commit b2f3ec5b6c

source/config/__init__.py Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit a91446c26a

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, However, could you add unit tests?
In particular, given PR #14183, it would be good to confirm this behavior for other data types.

@seanbudd seanbudd marked this pull request as draft October 13, 2022 01:36
@seanbudd seanbudd force-pushed the fix-featureFlag-updating-val branch from 6ef7a57 to 154a90a Compare January 9, 2023 02:51
@seanbudd seanbudd marked this pull request as ready for review January 9, 2023 02:54
Comment on lines +1253 to 1260
if self._isSection(val) or self._isSection(curVal):
# If value is a section, continue to update
pass
elif str(val) == str(curVal):
# Check str comparison as this is what is written to the config.
# If the value is unchanged, do not update
# or mark the profile as dirty.
return
Copy link
Member Author

@seanbudd seanbudd Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this section is the only intended change of behaviour

@seanbudd seanbudd merged commit 816496f into master Jan 9, 2023
@seanbudd seanbudd deleted the fix-featureFlag-updating-val branch January 9, 2023 04:57
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Jan 9, 2023
seanbudd added a commit that referenced this pull request Jul 3, 2023
Fixes #14760
Fixup of #14133

Summary of the issue:
When saving a config spec, validation would be skipped if the string value of the data is unchanged.

This caused various issues including config values not being correctly converted to numbers from strings when validating.
This caused config profiles to fail to load or save correctly.

Description of user facing changes
Fix up of various bugs related to user config

Description of development approach
Perform special handling that was introduced in #14133 for feature flags only
seanbudd pushed a commit that referenced this pull request Sep 17, 2024
)

Fixes #17157
Fixup of #14133
Partially reverts #15074

Summary of the issue:
When saving configuration profiles, unchanged values are yet saved to a profile.

Description of user facing changes
Configuration profiles should become less polluted after this change.

Description of development approach
Restored the behavior of #14133, mostly reverting #15074. There was namely a False assumption in #15074.
@seanbudd wrote:

This caused various issues including config values not being correctly converted to numbers from strings when validating.

In fact, the validation was not the problem here. Instead, when __getitem__ was called with checkValidity set to False, the unvalidated value was yet saved in the cache. Therefore subsequent calls of __getitem__ would always return the unvalidated cached value that would always be of the string type.
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.

5 participants