Navigation Menu

Skip to content

Commit

Permalink
Add feature flag for vBuf load on busy state (PR #13859)
Browse files Browse the repository at this point in the history
See base PR #13851
Adds support for multi value feature flags from PR #13871

### Summary:
NVDA increasingly makes use of feature flags to expose experimental changes to users.
There are already several approaches already in use.
It is easy to miss use-cases for changing default behavior, potentially overriding an advanced users preferred config.

Some feature flags require more than just a Boolean enabled/disabled outcome (in addition to 'default'), E.G.:
- AllowUiaInMSWord [WHEN_NECESSARY, WHERE_SUITABLE, ALWAYS]
- AllowUiaInChromium [WHEN_NECESSARY, YES, NO]

Several approaches have been tried:
- Store an int in the config that match with enum values, as per (AllowUiaInMSWord and AllowUiaInChromium)
- Use "options" in config (several string values). Literal strings in code to compare refer to these.

These approaches can hide errors:
- Looking at the config, what are the valid values when an integer is used.
- Using string "options":
  - What are the valid options?
  - How do you find all usages?
  - Change the strategy for default?
- For both, when looking at the config or configSpec, how can the behavior of default be determined?

### Description of user facing changes
The target user of this change are developers.
This introduces constructs and utilities to streamline the approach to adding feature flags.

Adding a new feature flag:
- Most feature flags can use the BoolFlag.
- If more values are required, define a new enum class in featureFlagEnums.py (include a DEFAULT member) E.G.: MyNewFeature
- Add the entry to the configSpec.py E.G.
  shouldUseMyNewFeature = featureFlag(optionsEnumClass="MyNewFeature", behaviorOfDefault="ON_WEEKENDS")
- Add a GUI option for it, using FeatureFlagCombo
- Wrap the code for the feature with:
   config.conf["section"]["shouldUseMyNewFeature "] == MyNewFeature.ON_WEEKENDS

### Development approach
Introduction of a generic BoolFlag type (enabled/disabled) which needs to account for 3 states:
- The current NVDA default (IE user has no preference)
- User has a preference of enabled
- User has a preference of disabled

To facilitate this, one of the three values (default, enabled, disabled) is stored in the config.
Spec/config transformations are provided to streamline the support.
A featureFlag type in the spec is automatically converted to a FeatureFlag class instance.
The FeatureFlag class abstracts the consideration of "default", allowing the FeatureFlag to treated as a boolean within internal NVDA logic.

A utility GUI control is added which presents the FeatureFlag option to the user, and allows developers to give translated names to the logical values (enabled, disabled), the name for default incorporates the behaviorOfDefault (and is also translated).

Note:
- The configSpec explicitly defines what the behavior of default is.
- The enum class that backs the config is defined, allows for type / value checking.
- Existing flags have not been upgraded, these would require upgrade code for the config, this introduces unnecessary complexity.

### Known issues:
This approach bypasses the handling for defaults provided by configObj.
It was considered whether the default not written on read behavior (of the configObj library) to facilitate the third (default) state.
However:
- This would require explicit code to test for the existence of the key to know if the value returned by `conf["section"]["myKey"]` was just the default or an explicit setting.
- It also requires careful diligence to ensure that the value is not ever written accidentally.
- Code to delete the key is required whenever attempting to return to the default.
- The way validation checks are created in `configObj` makes transforming the type (to provide an abstraction) difficult. It is not possible to get access to the `default` without a similar approach used in this PR. If adopting the complexity of a spec and config transform step, then it is preferable to have explicit values for default. Explicit values make the config easier to inspect and make the approach more robust.
  • Loading branch information
feerrenrut committed Jul 12, 2022
1 parent 1bcd5d4 commit e86094a
Show file tree
Hide file tree
Showing 11 changed files with 981 additions and 4 deletions.
144 changes: 144 additions & 0 deletions devDocs/featureFlags.md
@@ -0,0 +1,144 @@
# Feature Flags

NVDA makes judicious use of feature flags to enable and disable features that are in early development.
The following are provided to streamline the creation of new feature flags:
- A config spec type
- A GUI control type

## Background
When providing a feature flag it is important to understand the importance of providing a "default" state.
A boolean feature, must have 3 states selectable by the user:
- `True`
- `False`
- `Default` (NVDA developer recommendation)

This allows a choice between the following use-cases to be made at any point in time:
- **Explicitly opt-in** to the feature, regardless of the default behavior.
An early adopter may choose to do this to test the feature and provide feedback.
- **Explicitly opt-out** of the feature, regardless of the default behavior.
A user may find the pre-existing behavior acceptable, and wants the maximum delay to adopt the new feature.
They may be prioritising stability, or anticipating this feature flag receives a permanent home in NVDA settings.
- **Explicitly choose the default** (NVDA developer recommended) behavior.
Noting, that in this case it is important that the user must be able to select one of the other options first,
and return to the default behavior at any point in time.

This should be possible while still allowing developers to change the behaviour
of the default option.
The development process might require that initially NVDA is released with
the feature disabled by default.
In this case only testers, or the most curious users are expected to opt-in temporarily.
As the feature improves, bugs are fixed, edge cases are handled, and the UX is improved,
developers may wish to change the behavior of the default option to enable the feature.
This change shouldn't affect those who have already explicitly opted out of the feature.
Only those who maybe haven't tried the feature, because they were using the prior default behaviour, or
those who have tried and found the feature to be unstable and decided they would wait for it to become
stable.

## Feature Flag Enum
To aid static typing in NVDA, `enum` classes are used.
`BoolFlag` is provided, the majority of feature flags are expected to use this.
However, if more values are required (E.G. `AllowUiaInMSWord` has options `WHEN_NECESSARY`, `WHERE_SUITABLE`, `ALWAYS`, in addition to `DEFAULT`), then a new `enum` class can be defined.
Adding the enum class to the `featureFlagEnums.py` file will automatically expose it for use in the config spec (see the next section).
Example new `enum` class:

```python

class AllowUiaInMSWordFlag(DisplayStringEnum):
"""Feature flag for UIA in MS Word.
The explicit DEFAULT option allows developers to differentiate between a value set that happens to be
the current default, and a value that has been returned to the "default" explicitly.
"""

@property
def _displayStringLabels(self):
""" These labels will be used in the GUI when displaying the options.
"""
# To prevent duplication, self.DEFAULT is not included here.
return {
# Translators: Label for an option in NVDA settings.
self.WHEN_NECESSARY: _("Only when necessary"),
# Translators: Label for an option in NVDA settings.
self.WHERE_SUITABLE: _("Where suitable"),
# Translators: Label for an option in NVDA settings.
self.ALWAYS: _("ALWAYS"),
}

DEFAULT = enum.auto()
WHEN_NECESSARY = enum.auto()
WHERE_SUITABLE = enum.auto()
ALWAYS = enum.auto()
```

## Config Spec
In `configSpec.py` specify the new config key, ideally in the category that is most relevant to the feature.
Placing it in a category rather than a catch-all feature flags category, allows for the option to become
permanent without having to write config upgrade code to move it from section to another.

```ini
[virtualBuffers]
newOptionForUsers = featureFlag(optionsEnum="BoolFlag", behaviourOfDefault="disabled")
anotherOptionForUsers = featureFlag(optionsEnum="AllowUiaInMSWordFlag", behaviourOfDefault="WHERE_SUITABLE")
```

The `featureFlag` type is a custom spec type.
It will produce a `config.FeatureFlag` class instance when the key is accessed.
```python
newFlagValue: config.FeatureFlag = config.conf["virtualBuffers"]["newOptionForUsers"]

# BoolFlag converts to bool automatically, taking into account 'behaviorOfDefault'
if newFlagValue:
print("The new option is enabled")

anotherFlagValue: config.FeatureFlag = config.conf["virtualBuffers"]["anotherOptionForUsers"]

# Other "optionsEnum" types can compare with the value, the 'behaviorOfDefault' is taken into account.
if flagValue == AllowUiaInMSWordFlag.ALWAYS:
print("Another option is enabled")
```

## GUI
A control (`gui.nvdaControls.FeatureFlagCombo`) is provided to simplify exposing the feature flag to the user.

### Usage:
Note the comments in the example:
- `creation`
- `is default`
- `reset to default value`
- `save GUI value to config`


```python
import collections
from gui import nvdaControls, guiHelper
import config
import wx

sHelper = guiHelper.BoxSizerHelper(self, sizer=wx.BoxSizer(wx.HORIZONTAL))

# Translators: Explanation for the group name
label = _("Virtual Buffers")
vbufSizer = wx.StaticBoxSizer(wx.VERTICAL, self, label=label)
vbufGroup = guiHelper.BoxSizerHelper(vbufSizer, sizer=vbufSizer)
sHelper.addItem(vbufGroup)

# creation
self.newOptionForUsersCombo: nvdaControls.FeatureFlagCombo = vbufGroup.addLabeledControl(
labelText=_(
# Translators: Explanation of what the control does and where it is used.
"New option for users"
),
wxCtrlClass=nvdaControls.FeatureFlagCombo,
keyPath=["virtualBuffers", "newOptionForUsers"], # The path of keys, see config spec.
conf=config.conf, # The configObj instance, allows getting / setting the value
)
...
# is default
# Checking if the user has a saved (non-default) value
self.loadChromeVbufWhenBusyCombo.isValueConfigSpecDefault()
...
# reset to default value:
self.loadChromeVbufWhenBusyCombo.resetToConfigSpecDefault()
...
# save GUI value to config:
self.loadChromeVbufWhenBusyCombo.saveCurrentValueToConf()
```
4 changes: 4 additions & 0 deletions requirements.txt
Expand Up @@ -17,6 +17,10 @@ py2exe==0.11.1.0
sphinx==3.4.1
sphinx_rtd_theme

# Requirements for type checking.
# typing_extensions is incorporated in py3.8+, also available via mypy
typing_extensions==4.3.0

# Requirements for automated linting
flake8 ~= 3.7.7
flake8-tabs == 2.1.0
Expand Down
2 changes: 1 addition & 1 deletion source/NVDAObjects/IAccessible/chromium.py
Expand Up @@ -90,7 +90,7 @@ class Document(ia2Web.Document):

def _get_treeInterceptorClass(self) -> typing.Type["TreeInterceptor"]:
shouldLoadVBufOnBusyFeatureFlag = bool(
config.conf["virtualBuffers"]["loadChromiumVbufOnBusyState"]
config.conf["virtualBuffers"]["loadChromiumVBufOnBusyState"]
)
vBufUnavailableStates = { # if any of these are in states, don't return ChromeVBuf
controlTypes.State.EDITABLE,
Expand Down
23 changes: 22 additions & 1 deletion source/config/__init__.py
Expand Up @@ -34,6 +34,10 @@
import extensionPoints
from . import profileUpgrader
from .configSpec import confspec
from .featureFlag import (
_transformSpec_AddFeatureFlagDefault,
_validateConfig_featureFlag,
)
from typing import Any, Dict, List, Optional, Set

#: True if NVDA is running as a Windows Store Desktop Bridge application
Expand Down Expand Up @@ -496,6 +500,20 @@ def addConfigDirsToPythonPackagePath(module, subdir=None):
pathList.extend(module.__path__)
module.__path__=pathList


def _transformSpec(spec: ConfigObj):
"""To make the spec less verbose, transform the spec:
- Add default="default" to all featureFlag items. This is required so that the key can be read,
even if it is missing from the config.
"""
spec.configspec = spec
spec.validate(
Validator({
"featureFlag": _transformSpec_AddFeatureFlagDefault,
}), preserve_errors=True,
)


class ConfigManager(object):
"""Manages and provides access to configuration.
In addition to the base configuration, there can be multiple active configuration profiles.
Expand All @@ -517,13 +535,16 @@ class ConfigManager(object):

def __init__(self):
self.spec = confspec
_transformSpec(self.spec)
#: All loaded profiles by name.
self._profileCache: Optional[Dict[Optional[str], ConfigObj]] = {}
#: The active profiles.
self.profiles: List[ConfigObj] = []
#: Whether profile triggers are enabled (read-only).
self.profileTriggersEnabled: bool = True
self.validator: Validator = Validator()
self.validator: Validator = Validator({
"_featureFlag": _validateConfig_featureFlag
})
self.rootSection: Optional[AggregatedSection] = None
self._shouldHandleProfileSwitch: bool = True
self._pendingHandleProfileSwitch: bool = False
Expand Down
2 changes: 1 addition & 1 deletion source/config/configSpec.py
Expand Up @@ -174,7 +174,7 @@
trapNonCommandGestures = boolean(default=true)
enableOnPageLoad = boolean(default=true)
autoFocusFocusableElements = boolean(default=False)
loadChromiumVbufOnBusyState = boolean(default=True)
loadChromiumVBufOnBusyState = featureFlag(optionsEnum="BoolFlag", behaviorOfDefault="enabled")
[touch]
enabled = boolean(default=true)
Expand Down

0 comments on commit e86094a

Please sign in to comment.