Re-add audio device combo box to synth select dialog #15486

codeofdusk · 2023-09-20T22:46:01Z

Link to issue number:
Partially reverts #15472.

Summary of the issue:

The "output device" option has been removed from the NVDA+Control+s dialog. This breaks muscle memory for long-time NVDA users and makes it very hard to blindly (without audible speech or connected Braille) change sound devices in the event of unexpected device switching.

Description of how this pull request fixes the issue:

Selectively reverts the change in #15472 that removes the option from the synth selection dialog.

Testing strategy:

Verified that the "output device" option in both dialogs works as expected

Known issues with pull request:

There is a small amount of code duplication.
The same dialog appears when clicking the "change..." button in voice settings and when invoking the gesture, but it doesn't make as much sense for the "audio device" option to appear in the first scenario since it's just as easy to find under audio settings. In a follow-up, consider making the option appear only when accessing the synth change dialog via gesture.

Change log entry:

In PR

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 · 2023-09-20T23:34:28Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/87qdqbaffn6qr108/artifacts/output/nvda_snapshot_pr15486-29266,32f11a2d.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.0,
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 32f11a2d16

codeofdusk · 2023-09-20T23:42:41Z

Some lint errors (especially re string formatting) appear to be false-positives, and this was compliant as originally present.

cary-rowen · 2023-09-21T00:29:36Z

This breaks muscle memory for long-time NVDA users and makes it very hard to blindly (without audible speech or connected Braille) change sound devices in the event of unexpected device switching.

Regarding the second concern you mentioned, I think implementing #8801 may be a long-term solution.

As for what you mentioned about destroying muscle memory, I think it is not the most important, because it will not lead to any terrible consequences.

codeofdusk · 2023-09-21T02:06:59Z

Regarding the second concern you mentioned, I think implementing #8801 may be a long-term solution.

Yes, though likely not in 2023.3. Worth considering for 2024.1!

cary-rowen · 2023-09-21T02:09:14Z

#15472 was also a change introduced during the beta cycle. What does NV Access think about this?
cc @seanbudd

source/gui/settingsDialogs.py

seanbudd · 2023-09-21T03:30:23Z

Regarding the second concern you mentioned, I think implementing #8801 may be a long-term solution.

Yes, though likely not in 2023.3. Worth considering for 2024.1!

agreed, the beta period is for completing work and fixing regressions not new features.
#15472 is the completion of WASAPI work that now appears stable and has had alpha and limited beta testing.

seanbudd · 2023-09-21T04:57:11Z

source/gui/settingsDialogs.py

@@ -2566,6 +2572,38 @@ def _synthWarningDialog(newSynth: str):
 	)


+def _addAudioCombos(panel, sHelper):


can this please get type hints?
Perhaps this could be a static method of AudioPanel, so that when the deprecated code is removed the function still makes sense as a helper function

any thoughts on making this a static method? it will greatly simplify the diff and also simplify reverting in 2024.1

source/gui/settingsDialogs.py

user_docs/en/changes.t2t

Adriani90 · 2023-09-21T05:04:31Z

I think if the audio settings category will have a shortcut as well or am I wrong?
To be honnest, if the user guide and the change log document properly that the audio settings category contains this setting, then there is no reason why long term users should not get used to it.
Long term nvda users will certainly be able to adapt this change in the day to day use as long as the documentation is clear about this.change.
In my view this setting suits better in the audio settings category but maybe I don‘t understand completely the issue here.

LeonarddeR · 2023-09-21T05:57:34Z

I'm not sure about this proposal. I really like a separate settings category, as long as it has a dedicated shortcut to open. In that case, audio output device would be the first item in the dialog and it would instantly gain focus, which is an improvement over the current sequence. That definitely needs to be added regardless of what happens with this pr.
Furthermore, since I introduced RD Access, I always found it a bit weird that the audio device combobox was situated in the synthesizer selection dialog since a synthesizer doesn't necessarily need an audio device locally.

However, I'm also slightly concerned that removing the audio device selection from the synthesizer dialog causes unexpected behavioral changes in case where NVDA is somehow stuck to a nonexistent sound device. In the past, changing synth would also reinitialize audio in this case since the selected device would automatically fall back to sound mapper, but now, this no longer happens.

CyrilleB79 · 2023-09-21T07:28:35Z

Agree with @LeonarddeR and @Adriani90: A shortcut to open audio settings could be added for the audio panel and the audio device would be the first item, directly focused. @codeofdusk, could you explain if and why this solution is not satisfying?

IMO the fact that people are used to NVDA+control+S then tab to blindly change their audio device is not a good reason to revert. Things may be changed provided that they are well documented in User Guide and in the Change log; an article in In-process could also be worth it if needed (Cc @Qchristensen)

However, I'm also slightly concerned that removing the audio device selection from the synthesizer dialog causes unexpected behavioral changes in case where NVDA is somehow stuck to a nonexistent sound device. In the past, changing synth would also reinitialize audio in this case since the selected device would automatically fall back to sound mapper, but now, this no longer happens.

@LeonarddeR could you please be more specific: use case description in detail and step to recover from non-existing sound device situation?

LeonarddeR · 2023-09-21T08:26:59Z

@LeonarddeR could you please be more specific: use case description in detail and step to recover from non-existing sound device situation?

I must admit that this is slightly hypothetical, and I need to test it to gain some proof, but up until now, synthesizer and audio device selection have been that intertwined that there is likely a interdependence that we might be missing. It is not without reason that the audio device is part of the speech category in the config spec.
I'm not saying we shouldn't move the audio device to a dedicated audio setting. However given we're already at beta 2, introducing a change like this in beta 3 concerns me. I'd rather see this ripen in Alpha for a while.

codeofdusk · 2023-09-21T20:17:57Z

@codeofdusk, could you explain if and why this solution is not satisfying?

The solution is satisfying, but given the length of time for which we've had audio devices in NVDA+Control+s (well over 10 years) announcing a change like this (especially given the possibility of unforseen interdependence pointed out by @LeonarddeR) with very little warning presents a poor user experience. Target the change for 2024.1 and use the intervening time to warn users/think of a good UX for this.

Adriani90 · 2023-09-21T22:26:18Z

Target the change for 2024.1 and use the intervening time to warn users/think of a good UX for this.

This sounds like a reasonable request.

seanbudd · 2023-09-22T04:46:54Z

I must admit that this is slightly hypothetical, and I need to test it to gain some proof, but up until now, synthesizer and audio device selection have been that intertwined that there is likely a interdependence that we might be missing. It is not without reason that the audio device is part of the speech category in the config spec.

@LeonarddeR, please check the code changes in #15472.
We perform the same reinitialization of the synthesizer and tones when changing the audio output device from the audio panel. As long as the onOk/onSave handling matches correctly there shouldn't be an issue.

LeonarddeR · 2023-09-22T05:40:48Z

I think I'm convinced now. I just tried the following:

Set audio device to a specific soundcard
Disconnect that soundcard.
Select another synth.

The new synth happily ignores the configured nonexistent device and chooses the sound mapper. This is great.

source/gui/settingsDialogs.py

…om/codeofdusk/nvda into readd-audio-device-to-synth-dialog

AppVeyorBot · 2023-09-22T08:36:24Z

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/9t92xxj1ha7xu41n/artifacts/output/nvda_snapshot_pr15486-29327,52966abc.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 25.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.5,
FINISH_END 0.1

See test results for failed build of commit 52966abcc1

AppVeyorBot · 2023-09-22T19:31:51Z

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/tppu7dwlaivk8tpj/artifacts/output/nvda_snapshot_pr15486-29329,af7e5827.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
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 21.7,
FINISH_END 0.1

See test results for failed build of commit af7e582700

seanbudd · 2023-09-24T23:55:55Z

@codeofdusk - there is no need to bump CI. If the failures are unrelated they can be ignored

…-synth-dialog

…s#15486)" This reverts commit e4ae771.

…4.1 (#15512) #15486 was intended only for the 2023.3 release. Remove it in 2024.1. Closes #15516. This reverts commit e4ae771.

codeofdusk requested a review from a team as a code owner September 20, 2023 22:46

codeofdusk requested a review from seanbudd September 20, 2023 22:46

codeofdusk changed the base branch from master to beta September 20, 2023 22:46

codeofdusk mentioned this pull request Sep 21, 2023

Create audio panel for new sound settings #15472

Merged

6 tasks

seanbudd requested changes Sep 21, 2023

View reviewed changes

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved

seanbudd marked this pull request as draft September 21, 2023 03:28

codeofdusk force-pushed the readd-audio-device-to-synth-dialog branch from 44f18c0 to b85837d Compare September 21, 2023 04:43

Re-add audio combo boxes to synth dialog

4aef139

codeofdusk force-pushed the readd-audio-device-to-synth-dialog branch from b85837d to 4aef139 Compare September 21, 2023 04:46

codeofdusk marked this pull request as ready for review September 21, 2023 04:53

seanbudd reviewed Sep 21, 2023

View reviewed changes

Review actions

e451724

seanbudd added this to the 2023.3 milestone Sep 22, 2023

codeofdusk added 2 commits September 21, 2023 22:47

Review actions

5ae8ac0

Merge branch 'beta' into readd-audio-device-to-synth-dialog

eb16cd6

seanbudd previously approved these changes Sep 22, 2023

View reviewed changes

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved

remove comment

6088352

seanbudd dismissed their stale review via 6088352 September 22, 2023 06:40

codeofdusk added 2 commits September 21, 2023 23:42

Actually mark static, review action

de2589d

Merge branch 'readd-audio-device-to-synth-dialog' of https://github.c…

bde651c

…om/codeofdusk/nvda into readd-audio-device-to-synth-dialog

Re-run CI

43ddcb8

Again?

92269a2

seanbudd added 2 commits September 25, 2023 15:14

Merge remote-tracking branch 'origin/beta' into readd-audio-device-to…

2405dc1

…-synth-dialog

fixup

7204b7b

seanbudd approved these changes Sep 25, 2023

View reviewed changes

seanbudd merged commit e4ae771 into nvaccess:beta Sep 25, 2023
1 check was pending

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Sep 25, 2023

Revert "Re-add audio device combo box to synth select dialog (nvacces…

a59cceb

…s#15486)" This reverts commit e4ae771.

codeofdusk mentioned this pull request Sep 25, 2023

Revert "Re-add audio device combo box to synth select dialog" for 2024.1 #15512

Merged

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Sep 25, 2023

Revert "Re-add audio device combo box to synth select dialog (nvacces…

48aa313

…s#15486)" This reverts commit e4ae771.

CyrilleB79 mentioned this pull request Sep 25, 2023

synthesizer selection in the latest alpha fails #15516

Closed

seanbudd pushed a commit that referenced this pull request Sep 26, 2023

Revert "Re-add audio device combo box to synth select dialog" for 202…

96985d6

…4.1 (#15512) #15486 was intended only for the 2023.3 release. Remove it in 2024.1. Closes #15516. This reverts commit e4ae771.

CyrilleB79 mentioned this pull request Nov 9, 2023

Update what's new for PR #15512 #15760

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-add audio device combo box to synth select dialog #15486

Re-add audio device combo box to synth select dialog #15486

codeofdusk commented Sep 20, 2023 •

edited

AppVeyorBot commented Sep 20, 2023

codeofdusk commented Sep 20, 2023

cary-rowen commented Sep 21, 2023

codeofdusk commented Sep 21, 2023

cary-rowen commented Sep 21, 2023

seanbudd commented Sep 21, 2023

seanbudd Sep 21, 2023

seanbudd Sep 22, 2023

Adriani90 commented Sep 21, 2023

LeonarddeR commented Sep 21, 2023

CyrilleB79 commented Sep 21, 2023

LeonarddeR commented Sep 21, 2023

codeofdusk commented Sep 21, 2023

Adriani90 commented Sep 21, 2023

seanbudd commented Sep 22, 2023

LeonarddeR commented Sep 22, 2023

AppVeyorBot commented Sep 22, 2023

AppVeyorBot commented Sep 22, 2023

seanbudd commented Sep 24, 2023

		@@ -2566,6 +2572,38 @@ def _synthWarningDialog(newSynth: str):
		)


		def _addAudioCombos(panel, sHelper):

Re-add audio device combo box to synth select dialog #15486

Re-add audio device combo box to synth select dialog #15486

Conversation

codeofdusk commented Sep 20, 2023 • edited

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entry:

Code Review Checklist:

AppVeyorBot commented Sep 20, 2023

codeofdusk commented Sep 20, 2023

cary-rowen commented Sep 21, 2023

codeofdusk commented Sep 21, 2023

cary-rowen commented Sep 21, 2023

seanbudd commented Sep 21, 2023

seanbudd Sep 21, 2023

Choose a reason for hiding this comment

seanbudd Sep 22, 2023

Choose a reason for hiding this comment

Adriani90 commented Sep 21, 2023

LeonarddeR commented Sep 21, 2023

CyrilleB79 commented Sep 21, 2023

LeonarddeR commented Sep 21, 2023

codeofdusk commented Sep 21, 2023

Adriani90 commented Sep 21, 2023

seanbudd commented Sep 22, 2023

LeonarddeR commented Sep 22, 2023

AppVeyorBot commented Sep 22, 2023

AppVeyorBot commented Sep 22, 2023

seanbudd commented Sep 24, 2023

codeofdusk commented Sep 20, 2023 •

edited