Disable WASAPI by default by seanbudd · Pull Request #15172 · nvaccess/nvda

seanbudd · 2023-07-21T05:19:12Z

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.

AppVeyorBot · 2023-07-21T06:04:47Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/egjqgxo021u52bwm/artifacts/output/nvda_snapshot_pr15172-28713,d58596b5.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 19.5,
FINISH_END 0.1

See test results for failed build of commit d58596b5ea

CyrilleB79

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

AppVeyorBot · 2023-07-21T06:57:54Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/75un58yrn1cnuuci/artifacts/output/nvda_snapshot_pr15172-28714,d58596b5.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 19.5,
FINISH_END 0.2

See test results for failed build of commit d58596b5ea

AppVeyorBot · 2023-07-24T00:55:58Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/5p7x630vjm4tpher/artifacts/output/nvda_snapshot_pr15172-28719,030d4876.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.0,
FINISH_END 0.1

See test results for failed build of commit 030d4876e0

AppVeyorBot · 2023-07-24T05:07:53Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/pb4840w1268bmoea/artifacts/output/nvda_snapshot_pr15172-28726,eddb1f2c.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 18.8,
FINISH_END 0.1

See test results for failed build of commit eddb1f2c4f

Qchristensen

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

CyrilleB79 · 2023-07-24T13:04:12Z

@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.

Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>

XLTechie · 2023-07-25T01:21:05Z

Shouldn't we keep the note in the user guide, but encourage people to test it or some such? How _do_ we encourage testing of these disabled now, but intended to be enabled after testing, advanced options?

seanbudd · 2023-07-25T01:54:01Z

@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 · 2023-07-25T01:55:32Z

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).

Disable WASAPI by default

0c86e42

seanbudd requested a review from a team as a code owner July 21, 2023 05:19

seanbudd requested a review from michaelDCurran July 21, 2023 05:19

CyrilleB79 reviewed Jul 21, 2023

View reviewed changes

Comment thread source/config/configSpec.py Outdated

seanbudd added 2 commits July 24, 2023 09:42

address review comments

44f805f

Prompt NVDA to restart on WASAPI change

720b187

seanbudd requested a review from a team as a code owner July 24, 2023 00:06

seanbudd requested a review from Qchristensen July 24, 2023 00:06

seanbudd changed the base branch from master to beta July 24, 2023 02:47

seanbudd added this to the 2023.2 milestone Jul 24, 2023

fix lint

60fd288

Qchristensen previously approved these changes Jul 24, 2023

View reviewed changes

CyrilleB79 reviewed Jul 24, 2023

View reviewed changes

Comment thread user_docs/en/userGuide.t2t

Comment thread user_docs/en/userGuide.t2t Outdated

seanbudd dismissed Qchristensen’s stale review via 42cb3c2 July 24, 2023 23:36

Apply suggestions from code review

42cb3c2

Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>

seanbudd marked this pull request as draft July 24, 2023 23:37

seanbudd added 2 commits July 25, 2023 14:23

Merge remote-tracking branch 'origin/beta' into disableWASAPI

6c28907

revert restart dialog behaviour

a46f173

seanbudd marked this pull request as ready for review July 25, 2023 04:31

michaelDCurran approved these changes Jul 25, 2023

View reviewed changes

seanbudd merged commit a86d1cb into beta Jul 25, 2023

seanbudd deleted the disableWASAPI branch July 25, 2023 05:24

This was referenced Jul 25, 2023

nvWave: Use Windows Audio Session API #11615

Closed

Python 3 versions of NVDA produce a scratch in the speech when finishing the end of a line #10185

Closed

Texts with multiple line spacings are voiced with NVDA + down arrow and voices crack #11061

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Disable WASAPI by default#15172

Disable WASAPI by default#15172
seanbudd merged 7 commits into
betafrom
disableWASAPI

seanbudd commented Jul 21, 2023 •

edited

Loading

Uh oh!

AppVeyorBot commented Jul 21, 2023

Uh oh!

CyrilleB79 left a comment

Uh oh!

Uh oh!

AppVeyorBot commented Jul 21, 2023

Uh oh!

AppVeyorBot commented Jul 24, 2023

Uh oh!

AppVeyorBot commented Jul 24, 2023

Uh oh!

Qchristensen left a comment

Uh oh!

CyrilleB79 commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

XLTechie commented Jul 25, 2023 via email

Uh oh!

seanbudd commented Jul 25, 2023

Uh oh!

seanbudd commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

seanbudd commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jul 21, 2023

Uh oh!

CyrilleB79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AppVeyorBot commented Jul 21, 2023

Uh oh!

AppVeyorBot commented Jul 24, 2023

Uh oh!

AppVeyorBot commented Jul 24, 2023

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

XLTechie commented Jul 25, 2023 via email

Uh oh!

seanbudd commented Jul 25, 2023

Uh oh!

seanbudd commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seanbudd commented Jul 21, 2023 •

edited

Loading