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

Disable WASAPI by default #15172

Merged
merged 7 commits into from
Jul 25, 2023
Merged

Disable WASAPI by default #15172

merged 7 commits into from
Jul 25, 2023

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jul 21, 2023

Link to issue number:

Changes to handle #15150
Follow up to #14697, #14896, #15038, #15097, #15145

Summary of the issue:

WASAPI usage is known to cause intermittent crashing in #15150.
Generally, WASAPI code has not been proven to be stable.
Due to this, it should not be enabled by default in 2023.2.
WASAPI can be re-enabled by default once it is proven to be stable.

Description of user facing changes

Disable WASAPI by default, preventing intermittent crashing #15150

Description of development approach

Turn the WASAPI checkbox into a feature flag, so that it can easily be re-enabled in future.

Testing strategy:

Manual testing
Upgrading the profile: Test starting NVDA with the WASAPI config value set to "True/False" instead of a "enabled/disabled/default".

Test the various controls related to WASAPI - ensure they are saved, applied and respected correctly.

Known issues with pull request:

None

Change log entries:

Refer to diff

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.

@seanbudd seanbudd requested a review from a team as a code owner July 21, 2023 05:19
@seanbudd seanbudd requested a review from michaelDCurran July 21, 2023 05:19
@AppVeyorBot
Copy link

See test results for failed build of commit d58596b5ea

Copy link
Collaborator

@CyrilleB79 CyrilleB79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General remark: the User Guide has not been updated; it still indicates that wasapi is enabled by default.

source/config/configSpec.py Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit d58596b5ea

@seanbudd seanbudd requested a review from a team as a code owner July 24, 2023 00:06
@seanbudd seanbudd requested a review from Qchristensen July 24, 2023 00:06
@AppVeyorBot
Copy link

See test results for failed build of commit 030d4876e0

@seanbudd seanbudd changed the base branch from master to beta July 24, 2023 02:47
@seanbudd seanbudd added this to the 2023.2 milestone Jul 24, 2023
@AppVeyorBot
Copy link

See test results for failed build of commit eddb1f2c4f

Qchristensen
Qchristensen previously approved these changes Jul 24, 2023
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Userguide change is good. Some users will be disappointed, but the option is still there in advanced until the issues are ironed out.

@CyrilleB79
Copy link
Collaborator

@seanbudd, I am a bit concerned with the restart dialog for WASAPI option.

If we change both the language in General Settings and the WASAPI combobox in Advanced settings:

  • If we choose to restart later for language, we get a second dialog for WASAPI; this seems an unpleasant UX IMO.
  • If we choose to restart now when asked for language, may it cause GUI issue. E.g. is there another dialog for WASAPI below? Is this a problem? ((just asking...)

Also, at least one other advanced option requires restart (although not documented in the GUI as reported in #13505): "Registration for UI Automation events and property changes". It would make sense to have it managed the same way.

IMO, WASAPI and Registration of UIA events and prop changes are "real" advanced options. Specifying "(requires restart)" in the label should be enough for an advanced user using these options and the dialog is not required. If a beginner has to modify these options, he/she should do it under instructions of a more advanced user. So my proposal is to remove the restart dialog for advanced options provided "(requires restart)" is correctly specified in the option's label.

At last, it's worth noting that the new restart dialog would also be impacted by #10288.

user_docs/en/userGuide.t2t Show resolved Hide resolved
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
@seanbudd seanbudd marked this pull request as draft July 24, 2023 23:37
@XLTechie
Copy link
Collaborator

XLTechie commented Jul 25, 2023 via email

@seanbudd
Copy link
Member Author

@XLTechie - we can re-enable this by default for 2023.3 once this is merged.
This feature is buggy - I think we should be keeping it primarily to alpha testing.
However, we may encourage testers to opt-in to test it in beta/rc/2023.2 via our typical comms channels.

@seanbudd
Copy link
Member Author

The feature is in advanced settings because we consider it to be experimental and don't encourage wider testing of it (e.g. users who won't report issues or can't recover well from a crash/freeze).

@seanbudd seanbudd marked this pull request as ready for review July 25, 2023 04:31
@seanbudd seanbudd merged commit a86d1cb into beta Jul 25, 2023
@seanbudd seanbudd deleted the disableWASAPI branch July 25, 2023 05:24
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.

6 participants