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

Change how enabled providers are saved in the config spec #10500

Merged
merged 7 commits into from
Nov 20, 2019
Merged

Change how enabled providers are saved in the config spec #10500

merged 7 commits into from
Nov 20, 2019

Conversation

LeonarddeR
Copy link
Collaborator

Link to issue number:

Follow up of #10082
Related to #10476

Summary of the issue:

Currently, active vision enhancement providers are saved in a list. Therefore, the state of active providers is saved on a list basis, not per provider. This means that, once a profile changes the list of active vision enhancement providers, the full list is saved in that profile, rather than just the provider for which the state was changed in that profile.

Description of how this pull request fixes the issue:

Save enabled/disabled state of the provider in the provider section itself, as an enabled boolean.

Testing performed:

  1. Tested tat activation/deactivation still works as expected.

  2. Tested the following steps:

    1. Enable highlighter and screen curtain
    2. Switch profile
    3. Disable screen curtain.
    4. Switch back to default profile
    5. Ensure screen curtain and highlighter are both enabled
    6. Disable highlighter
    7. Switch back to the custom profile
    8. Ensure that both providers are disabled in the profile. Disabled state of the highlighter is inherrited from the default config, rather than saved in the profile.

Known issues with pull request:

This does not solve the concern from #10476. I feel we should discuss this further before coming up with a solution.

Change log entry:

None

@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit 9ab111802a

source/vision/visionHandler.py Outdated Show resolved Hide resolved
source/visionEnhancementProviders/NVDAHighlighter.py Outdated Show resolved Hide resolved
@LeonarddeR
Copy link
Collaborator Author

So, this pr adds the enabled boolean to the config spec, but this entry could go out of sync with the settings in the settings storage. In the storage, all entries could be True even when enabled is False. If my theory is right, in this case when opening the vision dialog, the highlighter will be enabled as soon as you focus the vision category and will be disabled again if you cancel.

I wonder what would be best to get this in sync. I think the enabled boolean in the config should take precedence over all provider specific settings, so if enabled is False and follow focus/caret/browse mode are off, the gui should show as was everything disabled.

@feerrenrut
Copy link
Contributor

I think the enabled boolean in the config should take precedence over all provider specific settings

I'm fine with this. But only if the enabled boolean is false, then it should override the other settings on load. When it's true, overriding the other settings would defeat their purpose.

@LeonarddeR
Copy link
Collaborator Author

I think I've done as requested.

@feerrenrut
Copy link
Contributor

Thanks @LeonarddeR I had a play with this, setting the enabled config state using the python console and got the expected behavior.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Nov 20, 2019 via email

@feerrenrut feerrenrut merged commit 866aafb into nvaccess:master Nov 20, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Nov 20, 2019
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.

4 participants