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

Remove winmm support from NVDA #17496

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Remove winmm support from NVDA #17496

wants to merge 28 commits into from

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Dec 10, 2024

Link to issue number:

Closes #16080
Closes #2067

Summary of the issue:

The support for WASAPI (and Windows' Core Audio APIs more broadly) in NVDA is now quite mature, and maintenance of our winmm support is becoming untenable.

Description of user facing changes:

The option to use WASAPI for audio output has been removed from NVDA's advanced settings. This has been on by default for some time, so it is unlikely to affect most users.

As the Mmdevice API does not have the concept of an ID to refer to the default output device, "Microsoft Sound Mapper" is no longer an option in the output device selection in Audio Settings. Users can instead choose "Default output device".

Description of development approach

Removed all references to winmm in nvwave.py. Rewrote the device enumeration to use the Mmdevice API, via pycaw. Slightly modified the device selection logic in the settings GUI in light of Mmdevice not having an equivalent to Microsoft Sound Mapper.

Testing strategy:

System and unit tests, as well as running NVDA, and changing output devices, including plugging and unplugging headphones while NVDA was running.

Known issues with pull request:

None

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.

@coderabbitai summary

@SaschaCowley SaschaCowley requested a review from a team as a code owner December 10, 2024 06:30
@SaschaCowley SaschaCowley linked an issue Dec 10, 2024 that may be closed by this pull request
@SaschaCowley
Copy link
Member Author

@coderabbitai please open an issue related to this one, for switching to using endpoint IDs rather than friendly names for identifying the selected output device in user configuration.

Copy link
Contributor

coderabbitai bot commented Dec 10, 2024

Tip

For best results, initiate chat on the files or code changes.

@SaschaCowley: I've opened a new GitHub issue as requested: #17497

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Open a new GitHub issue with the requested details
gh issue create \
  --title "Switch to using endpoint IDs for identifying selected output device in user configuration" \
  --body "As per [PR #17496](https://github.com/nvaccess/nvda/pull/17496) and [this comment](https://github.com/nvaccess/nvda/pull/17496#issuecomment), we need to switch to using endpoint IDs rather than friendly names for identifying the selected output device in user configuration. Requested by @SaschaCowley." \
  --assignee "SaschaCowley"

Length of output: 499


✏️ Learnings added
Learnt from: SaschaCowley
URL: https://github.com/nvaccess/nvda/pull/17496

Timestamp: 2024-12-10T06:33:03.209Z
Learning: When identifying selected output devices in NVDA's user configuration, use endpoint IDs rather than friendly names.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

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.

This PR seems incomplete:

  • Various audio features (e.g. sound split, mute other apps, etc.) only work if WASAPI is enabled:
    • in settings dialog, the check of WASAPI to enable or disable controls for such settings should be removed (see (nvwave.usingWasapiWavePlayer calls)
    • in the User Guide, the documentation of these settings mention WASAPI requirement; this should be removed
  • In config spec, WASAPI should be removed from audio section, and this removal should be documented in API-breaking changes. Or in case you want to keep it as a no-effect parameter, you should at least document it in the change log and remove all of its usage in NVDA's code.
  • I also wonder if the removal of AdvancedPanelControls.wasapiComboBox should be documented in the API-breaking changes.

@SaschaCowley SaschaCowley marked this pull request as draft December 10, 2024 22:18
@SaschaCowley SaschaCowley marked this pull request as ready for review December 11, 2024 03:42
@SaschaCowley
Copy link
Member Author

@CyrilleB79 thanks for the thorough notes. I believe I've addressed all of your points now.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @SaschaCowley - mostly looks good to me!

source/audio/appsVolume.py Show resolved Hide resolved
source/nvwave.py Outdated Show resolved Hide resolved
source/nvwave.py Outdated Show resolved Hide resolved
source/nvwave.py Outdated Show resolved Hide resolved
source/nvwave.py Outdated Show resolved Hide resolved
source/nvwave.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants