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

Settings Ring: Lazily load available setting values the first time they're needed #14704

Merged
merged 2 commits into from Mar 20, 2023

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Mar 7, 2023

Link to issue number:

None

Summary of the issue:

A settings ring entry for a String setting caches its possible values when creating the StringSynthSetting object. This can be costly if it takes some time for the possible values to return, for example when they're coming from a remote system in my remote desktop add-on.

Description of user facing changes

None.

Description of development approach

Load and cache the available settings values the first time they're fetched. This shouldn't have any noticeable impact for most synths.that

Testing strategy:

Tested with NVDA Remote Desktop add-on that initial setup of the remote synth is quicker, whereas the first selection of a settings ring entry is somewhat slower.

Known issues with pull request:

None

Change log entries:

For Developers

  • The NVDA Synth Settings Ring now caches available setting values the first time they're needed, rather than when loading the synthesizer.

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.

@LeonarddeR LeonarddeR marked this pull request as ready for review March 15, 2023 12:38
@LeonarddeR LeonarddeR requested a review from a team as a code owner March 15, 2023 12:38
@michaelDCurran michaelDCurran merged commit 1a47427 into nvaccess:master Mar 20, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Mar 20, 2023
LeonarddeR added a commit to LeonarddeR/nvda that referenced this pull request Mar 25, 2023
michaelDCurran pushed a commit that referenced this pull request Mar 27, 2023
In #14704, we introduced lazy loading of some settings ring settings. However, this resulted in error sounds being played when moving in the list of OneCore voices.

Description of user facing changes
Fix of the regression.

Description of development approach
The max value of a synth setting is based on the number of available settings. This is now dynamically fetched.
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.

None yet

3 participants