Skip to content

UX fixes for WASAPI GUI and doc#15478

Merged
seanbudd merged 2 commits into
nvaccess:betafrom
CyrilleB79:wasapi
Sep 20, 2023
Merged

UX fixes for WASAPI GUI and doc#15478
seanbudd merged 2 commits into
nvaccess:betafrom
CyrilleB79:wasapi

Conversation

@CyrilleB79

Copy link
Copy Markdown
Contributor
### Link to issue number:

Follow-up of #15472.

Summary of the issue:

If NVDA is started with WASAPI disabled in config and if you enable WASAPI, the new audio options to control NVDA sounds along with voice and volume of NVDA sound are available. That's confusing because modifying them will not have any effect until NVDA is restarted.

Description of user facing changes

Enabling or disabling audio options linked to WASAPI will be done looking at current state of WASAPI usage rather than looking at the state configured for next restart.

Also added an indication in the User Guide that these options can be unavailable so that the user does not look for them when they are greyed out.

Description of development approach

Rather than checking the config, check the player currently used to determine if the WASAPI related options need to be disabled or not.

Cc @jcsteh to confirm this check.

Testing strategy:

  • Manual test: disable and reenable WASAPI option in adv settings. Check audio options after having changed WASAPI option and after restart of NVDA.
  • Check User Guide generated by appVeyor.

Known issues with pull request:

None

Note

No change log needed.
Note: With the new process requiring change log to be integrated in the modifications rather than in the PR's description, we should pay attention to missing change log: has it been forgotten or is its omission intended (small fix, fix of unreleased code, etc.)

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.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 0245a4369f

@Adriani90

Copy link
Copy Markdown
Collaborator

Does disabling WASAPI still make sense at all, given the bugs that are introduced when it is disabled? Maybe this should be clarified. If there is no use case for disabled WASAPI anymore, then i suggest removing this setting completely and let only the related settings to the enabled WASAPI to appear in the GUI.

cc: @jcsteh

@CyrilleB79

Copy link
Copy Markdown
Contributor Author

Does disabling WASAPI still make sense at all, given the bugs that are introduced when it is disabled?

Which bugs? Could you be more specific? WASAPI is disabled by default in 2023.2 with no critical issue to my knowledge.

If there is no use case for disabled WASAPI anymore, then i suggest removing this setting completely and let only the related settings to the enabled WASAPI to appear in the GUI.

There are still add-ons that do not support WASAPI. One of them is Sound Splitter maintained by @XLTechie and it is very useful for me.
As an alternative, there is NVDA global commands extension by @paulber19 which embeds a quite similar sound splitting feature. But:

  • Although being quite popular, this add-on is not (yet) in the store (Sorry Paul to insist on it!)
  • This add-on is NVDA's Swiss Army knife with many embedded features and a huge codebase; personally since I am always hunting bugs while running NVDA, I am a bit reluctant to install it given the number of changes that it implements in NVDA.

@CyrilleB79 CyrilleB79 changed the base branch from master to beta September 20, 2023 12:48
@CyrilleB79 CyrilleB79 closed this Sep 20, 2023
@CyrilleB79 CyrilleB79 reopened this Sep 20, 2023
@Adriani90

Adriani90 commented Sep 20, 2023 via email

Copy link
Copy Markdown
Collaborator

@CyrilleB79

Copy link
Copy Markdown
Contributor Author

There are several bugs fixed by Wasapi, see the corespomding PR.

There are #11169 (Infrequent crash in Firefox), #10185 (scratch at end of line) and #11061 (voice crack in text with blank lines).

All these issues seem to be quite infrequent or causing only little inconveniences and impacting only few users. For many the benefit of sound splitting capability is much higher than the reported inconveniences of legacy audio system.

Addons should follow this technology.

I fully agree that at the end, add-ons should use WASAPI. And that in the first place, they should have asked NV Access to have the needed functions/methods in the API, rather than using private methods. But they haven't.
My request is not to keep legacy audio system forever in NVDA; I have heard and understood that maintaining the old audio system for a long time has no sense and would be a burden for NV Access. But the request is rather to give add-on authors more time to work on this. NVDA 2023.3 seems very near and WASAPI is enabled only since 2023.2. Why not wait at least for the next API compa breaking release when add-on authors will have to review their add-ons in any case?

My understanding is that the setting whether to use Wasapi or not, was added to the gui in order to have a testing phase.

IMO, when options are introduced by e feature flag, the following steps should be followed:

  1. Old options enabled, no new option implemented at all
  2. Old options enabled by default, new option implemented and available as experimental.
  3. New option enabled by default, old option still available as fallback/
  4. New option enabled and mandatory, old option not available anymore.

And a full release cycle should be guaranteed between each step so that non-beta users can confirm that there is no issue with each step.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit e1dcc4f0ab

@mwhapples

Copy link
Copy Markdown
Contributor

Does disabling WASAPI still make sense at all, given the bugs that are introduced when it is disabled? Maybe this should be clarified. If there is no use case for disabled WASAPI anymore, then i suggest removing this setting completely and let only the related settings to the enabled WASAPI to appear in the GUI.

cc: @jcsteh

My experience is that WASAPI enabled is worse and has bugs to make it unusable, so removing the ability to disable it is not an option for me. I have created issue #15483 to reflect the issues I am having with WASAPI being enabled. In short it makes any audio beeps not audible unless speech is going on and bits of speech go missing.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review September 20, 2023 20:00
@CyrilleB79 CyrilleB79 requested review from a team as code owners September 20, 2023 20:00
@CyrilleB79 CyrilleB79 requested review from Qchristensen and seanbudd and removed request for a team September 20, 2023 20:00
Comment thread source/nvwave.py
@seanbudd seanbudd added this to the 2023.3 milestone Sep 20, 2023
@seanbudd seanbudd merged commit dd1713a into nvaccess:beta Sep 20, 2023
@CyrilleB79 CyrilleB79 deleted the wasapi branch September 21, 2023 07:37
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.

5 participants