Persist screen curtain state when changed through settings dialog

[SaschaCowley]

Link to issue number:

Fixes 19783

Summary of the issue:

When modified through the settings dialog, the state of the screen curtain is not persisted to the config.

Description of user facing changes:

The screen curtain state is now persisted to the config when changed through the settings dialog.

Description of developer facing changes:

None

Description of development approach:

Added a _screenCurtainCheckboxChanged bool to the privacy and security panel. Initially set to False, it is set to True when the user successfully interacts with the checkbox (that is, uses it to enable or disable the screen curtain). It is never set back to False. In onSave, store the state of the checkbox as the value of config.conf["screenCurtain"]["enabled"] if and only if this new flag is True`.

Testing strategy:

Tested altering the screen curtain state from the Privacy and Security settings panel as follows:

Screen curtain	config.conf["screenCurtain"]["enabled"]	Screen is	Open privasy and security and...	config.conf["screenCurtain"]["enabled"]	Screen is	Then...	config.conf["screenCurtain"]["enabled"]	Screen is
Disabled	False	Visible	Do nothing	False	Visible	Press Cancel	False	Visible
Disabled	False	Visible	Do nothing	False	Visible	Press OK	False	Visible
Disabled	False	Visible	Check to enable	False	Obscured	Press Cancel	False	Visible
Disabled	False	Visible	Check to enable	False	Obscured	Press OK	True	Obscured
Disabled	False	Visible	Check then uncheck to enable then disable	False	Visible	Press Cancel	False	Visible
Disabled	False	Visible	Check then uncheck to enable then disable	False	Visible	Press OK	False	Visible
Temporary	False	Obscured	Do nothing	False	Obscured	Press Cancel	False	Obscured
Temporary	False	Obscured	Do nothing	False	Obscured	Press OK	False	Obscured
Temporary	False	Obscured	Uncheck to disable	False	Visible	Press Cancel	False	Obscured
Temporary	False	Obscured	Uncheck to disable	False	Visible	Press OK	False	Visible
Temporary	False	Obscured	Uncheck then check to disable then enable	False	Obscured	Press Cancel	False	Obscured
Temporary	False	Obscured	Uncheck then check to disable then enable	False	Obscured	Press OK	True	Obscured
Enabled	True	Obscured	Do nothing	True	Obscured	Press Cancel	True	Obscured
Enabled	True	Obscured	Do nothing	True	Obscured	Press OK	True	Obscured
Enabled	True	Obscured	Uncheck to disable	True	Visible	Press Cancel	True	Obscured
Enabled	True	Obscured	Uncheck to disable	True	Visible	Press OK	False	Visible
Enabled	True	Obscured	Uncheck then check to disable then enable	True	Obscured	Press Cancel	True	Obscured
Enabled	True	Obscured	Uncheck then check to disable then enable	True	Obscured	Press OK	True	Obscured

Known issues with pull request:

It's possibly slightly unintuitive that starting from a temporary screen curtain, if one unchecks then checks the screen curtain checkbox and saves settings, the screen curtain will subsequently be enabled, not temporary.

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

[CyrilleB79]

I guess though that you reintroduce #19765 for screen curtain. Have you checked this.
If it is confirmed, you should at least mention it in known issues, and better, fix it if possible.

[SaschaCowley]

@CyrilleB79

I guess though that you reintroduce #19765 for screen curtain. Have you checked this. If it is confirmed, you should at least mention it in known issues, and better, fix it if possible.

Not to the best of my knowledge. The logic that @seanbudd introduced with #19652 handles making sure the state is restored on cancel. See the test table for my test findings.

[CyrilleB79]

@SaschaCowley, sorry, I have just tested and can confirm that you reintroduce #19765 for applied to screen curtain case, i.e.:

1. Start with default config with SC disabled (both in config and actually)
2. Open Privacy and security settings
3. Check "Make screen black" checkbox; as a result, the SC is activated.
4. Save config to disk with NVDA+control+c
5. Press escape to dismiss the dialog; as a result, the SC is disabled.
6. Press NVDA+control+r to restore the config from the disk

Actual result:
At step 6. the SC is enabled

Expected result:

• At step 3, the config should not be modified, since changes made in settings dialogs should only be changed in config when OK is pressed.
• At step 5., escape is pressed, not "OK". So no modification made in the settings dialog should have impacted the config.
• So at step 6. the SC should not be enabled.

The issue with what is currently done is that information is saved in the config while playing in the settings dialog and then this information is cancelled from config in case you press escape. This leads to corner case such as #19765 . Instead, it would be preferrable to enable SC only temporarily when you check the checkbox in the settings dialog; and you can then enable it in config only when "OK" is pressed to validate.

stale

[SaschaCowley]

@CyrilleB79 said

sorry, I have just tested and can confirm that you reintroduce #19765 for applied to screen curtain case

Thanks for catching that. I believe I have fixed it now.