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

Fix audio output device selection #17532

Merged
merged 7 commits into from
Dec 18, 2024
Merged

Fix audio output device selection #17532

merged 7 commits into from
Dec 18, 2024

Conversation

SaschaCowley
Copy link
Member

Link to issue number:

Fixes #17530

Summary of the issue:

With the switch to exclusively WASAPI, selecting among more than 2 output devices did not work properly.

Description of user facing changes

Selection of output devices should now be more reliable.

Description of development approach

Fixed up the check for whether the selected device is the default output device. Also removed some unneeded initialisation parameters and constants.

Testing strategy:

Manually tested switching between 3 output devices (plus default), with various devices set as the Windows and NVDA default.

Known issues with pull request:

Some legacy winmm code remains, but this will be removed once SAPI4 support has been fixed.

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 16, 2024 03:59
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Dec 16, 2024
user_docs/en/changes.md Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@SaschaCowley SaschaCowley merged commit bcccf14 into master Dec 18, 2024
5 checks passed
@SaschaCowley SaschaCowley deleted the fixAudioOutputSel branch December 18, 2024 06:29
@github-actions github-actions bot added this to the 2025.1 milestone Dec 18, 2024
@jcsteh
Copy link
Contributor

jcsteh commented Dec 18, 2024

While we're removing things, is it worth renaming WasapiWavePlayer to just WavePlayer as well, thus removing the redundant alias? The fact that it's called WasapiWavePlayer was an internal implementation detail necessitated by the coexistence of WinMM and WASAPI. In hindsight, I probably should have underscore prefixed it in the first place.

@SaschaCowley
Copy link
Member Author

@jcsteh yes, I think we should. I'm planning to do this in the new year.

@seanbudd
Copy link
Member

seanbudd commented Jan 6, 2025

@SaschaCowley - I think this also missed being announced on the API mailing list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default output device detection does not work correctly
3 participants