FeatureFlag: fix changing from default value and value of default by seanbudd · Pull Request #14133 · nvaccess/nvda

seanbudd · 2022-09-13T00:14:20Z

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:

With NVDA running, open braille preferences
Update "Interrupt Speech While scrolling" from Default (Enabled) to Enabled
Save the settings and exit the settings dialog
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.

AppVeyorBot · 2022-09-13T00:58:46Z

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/58pqp2iiqxhx18k4/artifacts/output/nvda_snapshot_pr14133-26534,77b915a1.exe

See test results for failed build of commit 77b915a162

AppVeyorBot · 2022-09-21T06:58:23Z

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/6bb19s4nsdue9xal/artifacts/output/nvda_snapshot_pr14133-26645,b2f3ec5b.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 22.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.2,
TEST_START 0.0,
TEST_END 13.5,
FINISH_END 0.2

See test results for failed build of commit b2f3ec5b6c

AppVeyorBot · 2022-10-03T03:00:24Z

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/q1j585gxipi7x53u/artifacts/output/nvda_snapshot_pr14133-26719,a91446c2.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.7,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 20.9,
TESTSETUP_START 0.0,
TESTSETUP_END 0.2,
TEST_START 0.0,
TEST_END 13.1,
FINISH_END 0.1

See test results for failed build of commit a91446c26a

feerrenrut

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 · 2023-01-09T02:54:55Z

+			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


Note: this section is the only intended change of behaviour

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

) 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.

seanbudd requested a review from a team as a code owner September 13, 2022 00:14

seanbudd requested a review from feerrenrut September 13, 2022 00:14

seanbudd self-assigned this Sep 13, 2022

feerrenrut reviewed Sep 14, 2022

View reviewed changes

Comment thread source/config/__init__.py Outdated

seanbudd marked this pull request as draft September 15, 2022 07:59

seanbudd marked this pull request as ready for review September 20, 2022 04:01

seanbudd requested a review from feerrenrut September 20, 2022 05:08

seanbudd marked this pull request as draft September 20, 2022 05:46

seanbudd marked this pull request as ready for review September 21, 2022 05:39

seanbudd commented Sep 26, 2022

View reviewed changes

Comment thread source/config/__init__.py Outdated

feerrenrut reviewed Oct 3, 2022

View reviewed changes

seanbudd marked this pull request as draft October 13, 2022 01:36

Create failing test as base on master

154a90a

seanbudd force-pushed the fix-featureFlag-updating-val branch from 6ef7a57 to 154a90a Compare January 9, 2023 02:51

Fix bug with FeatureFlags, add typing to aggregated section

17ee132

seanbudd marked this pull request as ready for review January 9, 2023 02:54

seanbudd requested a review from michaelDCurran January 9, 2023 02:54

seanbudd commented Jan 9, 2023

View reviewed changes

michaelDCurran approved these changes Jan 9, 2023

View reviewed changes

update changes

fc56536

seanbudd merged commit 816496f into master Jan 9, 2023

seanbudd deleted the fix-featureFlag-updating-val branch January 9, 2023 04:57

nvaccessAuto added this to the 2023.1 milestone Jan 9, 2023

CyrilleB79 mentioned this pull request Mar 31, 2023

error when switching to another synthesizer when the change was previously made in a profile #14760

Closed

seanbudd mentioned this pull request Jun 29, 2023

Fix issue with saving config and FeatureFlags #15074

Merged

6 tasks

LeonarddeR mentioned this pull request Sep 11, 2024

Config profiles quickly get polluted with untouched settings #17157

Closed

LeonarddeR mentioned this pull request Sep 11, 2024

Fix config profiles quickly get polluted with untouched settings #17158

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

FeatureFlag: fix changing from default value and value of default#14133
seanbudd merged 3 commits into
masterfrom
fix-featureFlag-updating-val

seanbudd commented Sep 13, 2022 •

edited

Loading

Uh oh!

AppVeyorBot commented Sep 13, 2022

Uh oh!

Uh oh!

AppVeyorBot commented Sep 21, 2022

Uh oh!

Uh oh!

AppVeyorBot commented Oct 3, 2022

Uh oh!

feerrenrut left a comment

Uh oh!

seanbudd Jan 9, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

seanbudd commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Sep 13, 2022

Uh oh!

Uh oh!

AppVeyorBot commented Sep 21, 2022

Uh oh!

Uh oh!

AppVeyorBot commented Oct 3, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seanbudd commented Sep 13, 2022 •

edited

Loading

seanbudd Jan 9, 2023 •

edited

Loading