Various fix-up of keep audio awake feature by CyrilleB79 · Pull Request #16147 · nvaccess/nvda

CyrilleB79 · 2024-02-08T11:16:42Z

This PR contains various fix-ups of PR #16099, some less important than others. Cc @mltony

Link to issue number:

Fix-up of #16099

Summary of the issue:

Error when disabling WASAPI and reopening advanced settings:

ERROR - unhandled exception (11:14:54.349) - MainThread (6028):
Traceback (most recent call last):
  File "gui\settingsDialogs.pyc", line 4517, in onCategoryChange
  File "gui\settingsDialogs.pyc", line 693, in onCategoryChange
  File "gui\settingsDialogs.pyc", line 675, in _doCategoryChange
  File "gui\settingsDialogs.pyc", line 603, in _getCategoryPanel
  File "gui\settingsDialogs.pyc", line 362, in __init__
  File "gui\settingsDialogs.pyc", line 372, in _buildGui
  File "gui\settingsDialogs.pyc", line 3514, in makeSettings
  File "gui\settingsDialogs.pyc", line 3259, in __init__
  File "gui\settingsDialogs.pyc", line 3352, in _onWasapiChange
  File "gui\guiHelper.pyc", line 232, in Enable
TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag'
ERROR - unhandled exception (11:14:54.361) - MainThread (6028):
Traceback (most recent call last):
  File "gui\guiHelper.pyc", line 218, in _onEnableChanged
TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag'

Oversight in AdvancedPanelControls.haveConfigDefaultsBeenRestored in settingsDialogs.py.
In settingsDialogs.py, rename variable to be consistent with the GUI ("silence" -> "keep audio awake")
Documentation (User Guide and Change log) updates

Description of user facing changes

The option is enabled or disabled depending on the real current WASAPI state, not the state defined in the configuration for the next restart of NVDA.
The "keep audio awake" option is moved to audio settings
The documentation does not mention "playing silence" at all anymore

Description of development approach

For point 1: use real WASAPI state instead of what is in the config for the next restart, as it was already done in other audio settings depending on WASAPI.

Testing strategy:

Checked that the error reported in point 1 does not occur anymore.

Checked that the feature still works, using the debug config parameter whiteNoiseVolume.

Note: I am not able to directly test the feature to keep audio awake, since I have no impacted device (e.g. Bluetooth headset).

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.

…ng WASAPI

CyrilleB79 · 2024-02-08T16:41:00Z

Note: there are two points following the merge of #16099 that are still being discussed:

Play silence to keep audio device awake #16099 (comment): should we use "speech sequence", "spoken phrase", "utterance" or something else be used in the documentation? cc @XLTechie )
Play silence to keep audio device awake #16148: should the "keep audio awake duration" option be moved to audio settings panel? (cc @haruncetinkaya306)

If we reach an agreement on one or both of this points, I can also include them in this PR.

mltony · 2024-02-08T22:55:44Z

Looks good to me!

seanbudd · 2024-02-08T23:07:41Z

should we use "speech sequence", "spoken phrase", "utterance" or something else be used in the documentation

Why not just: "This edit box specifies how long NVDA plays silence after speech ends"

seanbudd · 2024-02-09T00:02:19Z

I think it would be good to move the setting to the audio panel in this PR

cary-rowen · 2024-02-09T00:35:15Z

I think it would be good to move the setting to the audio panel in this PR

The default value is sufficient for me and even most users, it is not a setting that needs to be changed frequently, so I think it is appropriate to put it in the advanced settings.

mltony · 2024-02-09T00:41:05Z

I agree with @cary-rowen - most users won't need to change this value, so I'd think it belongs on the advanced panel.

XLTechie · 2024-02-09T04:37:50Z

The default value is sufficient for me and even most users, it is not a setting that needs to be changed frequently, so I think it is appropriate to put it in the advanced settings.

It is not a matter of whether the default is sufficient. It is a matter of where users would expect to find it. IMO, that's in audio. There is nothing overly advanced about this setting. Plus, though I haven't tried the latest build, I think the only way to turn this feature on/off, is to change this setting, is it not? If that is so, even more reason to keep it out of Advanced and put it in Audio. It advertises the feature, and what can be done with it. Some users are afraid of anything in Advanced, as anyone inclined to be afraid of it likely should be.

XLTechie · 2024-02-09T04:39:42Z

Why not just: "This edit box specifies how long NVDA plays silence after speech ends"

Yes, exactly the kind of thing I was targeting. Explains it with no technical terminology at all. CC @Adriani90

CyrilleB79 · 2024-02-09T09:17:28Z

I have addressed review comments:

improved the documentation
moved the parameter to audio settings

To see the documentation improvements before the paragraph was moved, do:
git diff master 1825f0bfeef3e0ecd7a163c4fd13f3057ae0c89b -- Copie""$ user_docs/en/userGuide.t2t
or better, if master has moved in the meantime:
git diff 49d5e581d499768d3858443ce25d2ccba4319630 1825f0bfeef3e0ecd7a163c4fd13f3057ae0c89b -- Copie""$ user_docs/en/userGuide.t2t

The following changes have been done in documentation:

replace "speech sequence" by "after speech end" (see Various fix-up of keep audio awake feature #16147 (comment)).
Removed completely the notion of playing silence, which is of no use for the end user. (in line with Play silence to keep audio device awake #16099 (comment))
For unavailability without WASAPI, mention the whole feature reather than only the control, which is not a slider actually. When there is WASAPI off, not only the control is unavailable, but the whole feature.

Regarding the move of the option to audio settings:

Added a shortcut key. My opinion is having shortcut keys for any control can always be a plus for some people, at least in all panels except advanced settings.

Brian1Gaff · 2024-02-09T09:34:33Z

If it can work at the very start of a spoken item after a long gap, then I'd agree. Luckily my main computer has no latency sound turn off as its a desktop, but have encountered these effects on Bluetooth devices and some laptops, and there that program Silencio seemed to be the best approach. Brian

…

-- ***@***.*** Sent via blueyonder.(Virgin media) Please address personal E-mail to:- ***@***.***, putting 'Brian Gaff' in the display name field. ----- Original Message ----- From: mltony To: nvaccess/nvda Cc: Subscribed Sent: Friday, February 09, 2024 12:41 AM Subject: Re: [nvaccess/nvda] Various fix-up of keep audio awake feature (PR #16147) I agree with @cary-rowen - most users won't need to change this value, so I'd think it belongs on the advanced panel. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

XLTechie

The following suggestions are mainly to improve the reading of this in my opinion as an English speaker and editor. @Qchristensen might also have some thoughts.

XLTechie · 2024-02-09T10:08:07Z

+			initial=config.conf["audio"]["keepAudioAwakeTimeSeconds"]
+		)
+		self.bindHelpEvent("KeepAudioAwakeDuration", self.audioAwakeDurationEdit)
+		self.audioAwakeDurationEdit.Enable(nvwave.usingWasapiWavePlayer())


If this is going to be "audioAwakeTimeSeconds" in config, why not have it the same in all of these variables and the anchor?
Even "AudioAwakeTime" would be fine (and shorter) IMO.

XLTechie · 2024-02-09T10:20:52Z

 This setting only takes effect when "Volume of NVDA sounds follows voice volume" is disabled.
 This option is not available if you have started NVDA with [WASAPI disabled for audio output #WASAPI] in Advanced Settings.

+==== Duration of keeping audio device awake ====[KeepAudioAwakeDuration]


Personally, in English, I find this title slightly awkward.
Here is a suggested alternative. Note that I didn't change the anchor name, as that is referenced else ware, and should be changed holistically if it is changed.
I'm not in love with this alternative either, but it may be somewhat better.

Suggested change

==== Duration of keeping audio device awake ====[KeepAudioAwakeDuration]

==== Keep audio device awake time ====[KeepAudioAwakeDuration]

I think I would just go with Time to keep audio device awake

agreed with Sascha here

Spelling fixes and wording improvements Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>

CyrilleB79 · 2024-02-09T12:13:54Z

Thanks @XLTechie for your valuable feedback about English spelling / grammar, as well as other wording improvements.

Regarding the name of the option, I'd say that we need to agree on the label of the control in the GUI. Then I'll derive the following:

name of the paragraph in the User Guide = same as the GUI option, just omitting the part between parenthesis, i.e. "(sec)"
variable names: something shorter with the most significant words of the option's name
anchor: idem

I'll wait for other feedback (@seanbudd, @Qchristensen and community).

I agree that having a suitable name in the GUI and in the user guide can participate to the success of a new feature.

SaschaCowley · 2024-02-14T00:05:48Z

 This setting only takes effect when "Volume of NVDA sounds follows voice volume" is disabled.
 This option is not available if you have started NVDA with [WASAPI disabled for audio output #WASAPI] in Advanced Settings.

+==== Duration of keeping audio device awake ====[KeepAudioAwakeDuration]


I think I would just go with Time to keep audio device awake

SaschaCowley · 2024-02-14T00:08:18Z

+This can happen due to audio devices (especially Bluetooth and wireless devices) entering stand-by mode.
+This might also be helpful in other use cases, such as when running NVDA inside a virtual machine (e.g. Citrix Virtual Desktop), or on certain laptops.
+
+Lower values may allow audio to be cut-off more often, as a device may enter stand-by mode too soon, causing the start of the following speech to be clipped.


The various instances of "stand-by" should be replaced with "standby".

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>

AppVeyorBot · 2024-02-20T09:55:31Z

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/q4b7gfny09htjfe9/artifacts/output/nvda_snapshot_pr16147-31117,8ccf634a.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 29.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 2.3,
FINISH_END 0.4

See test results for failed build of commit 8ccf634a3d

Fix-up of nvaccess#16099 Fixes nvaccess#16148 Summary of the issue: Error when disabling WASAPI and reopening advanced settings: ERROR - unhandled exception (11:14:54.349) - MainThread (6028): Traceback (most recent call last): File "gui\settingsDialogs.pyc", line 4517, in onCategoryChange File "gui\settingsDialogs.pyc", line 693, in onCategoryChange File "gui\settingsDialogs.pyc", line 675, in _doCategoryChange File "gui\settingsDialogs.pyc", line 603, in _getCategoryPanel File "gui\settingsDialogs.pyc", line 362, in __init__ File "gui\settingsDialogs.pyc", line 372, in _buildGui File "gui\settingsDialogs.pyc", line 3514, in makeSettings File "gui\settingsDialogs.pyc", line 3259, in __init__ File "gui\settingsDialogs.pyc", line 3352, in _onWasapiChange File "gui\guiHelper.pyc", line 232, in Enable TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag' ERROR - unhandled exception (11:14:54.361) - MainThread (6028): Traceback (most recent call last): File "gui\guiHelper.pyc", line 218, in _onEnableChanged TypeError: Window.Enable(): argument 1 has unexpected type 'FeatureFlag' Oversight in AdvancedPanelControls.haveConfigDefaultsBeenRestored in settingsDialogs.py. In settingsDialogs.py, rename variable to be consistent with the GUI ("silence" -> "keep audio awake") Documentation (User Guide and Change log) updates Description of user facing changes The option is enabled or disabled depending on the real current WASAPI state, not the state defined in the configuration for the next restart of NVDA. The "keep audio awake" option is moved to audio settings The documentation does not mention "playing silence" at all anymore Description of development approach For point 1: use real WASAPI state instead of what is in the config for the next restart, as it was already done in other audio settings depending on WASAPI.

CyrilleB79 added 3 commits February 8, 2024 11:53

Fix-up nvaccess#16099 - no more error in Settings dialog when disabli…

0599190

…ng WASAPI

Rename variables according to GUI

a3f154c

Documentation improvements

2a6f2c0

CyrilleB79 marked this pull request as ready for review February 8, 2024 16:41

CyrilleB79 requested review from a team as code owners February 8, 2024 16:41

CyrilleB79 requested review from Qchristensen and SaschaCowley February 8, 2024 16:41

CyrilleB79 mentioned this pull request Feb 8, 2024

Play silence to keep audio device awake #16148

Closed

seanbudd marked this pull request as draft February 9, 2024 00:01

CyrilleB79 added 3 commits February 9, 2024 08:40

Merge branch 'master' into fixUpPlaySilence

4dce63e

Improve documentation

1825f0b

Move param to audio settings

40fa108

CyrilleB79 marked this pull request as ready for review February 9, 2024 09:23

XLTechie suggested changes Feb 9, 2024

View reviewed changes

Apply suggestions from code review

d4255aa

Spelling fixes and wording improvements Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Feb 12, 2024

SaschaCowley reviewed Feb 14, 2024

View reviewed changes

seanbudd marked this pull request as draft February 20, 2024 05:35

CyrilleB79 and others added 3 commits February 20, 2024 09:50

Apply suggestions from code review

83283a9

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>

Addressing review comments

90a89b2

Merge branch 'master' into fixUpPlaySilence

a346a5f

Linting

3eef916

CyrilleB79 marked this pull request as ready for review February 20, 2024 21:37

seanbudd approved these changes Feb 21, 2024

View reviewed changes

seanbudd merged commit f178c1d into nvaccess:master Feb 21, 2024

nvaccessAuto added this to the 2024.2 milestone Feb 21, 2024

CyrilleB79 deleted the fixUpPlaySilence branch February 22, 2024 10:16

Adriani90 mentioned this pull request May 16, 2024

Unstable audio connection when using a Bluetooth headset with NVDA #8017

Closed

	==== Duration of keeping audio device awake ====[KeepAudioAwakeDuration]
	==== Keep audio device awake time ====[KeepAudioAwakeDuration]

Uh oh!

Conversation

CyrilleB79 commented Feb 8, 2024 • 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:

Code Review Checklist:

Uh oh!

CyrilleB79 commented Feb 8, 2024

Uh oh!

mltony commented Feb 8, 2024

Uh oh!

seanbudd commented Feb 8, 2024

Uh oh!

seanbudd commented Feb 9, 2024

Uh oh!

cary-rowen commented Feb 9, 2024

Uh oh!

mltony commented Feb 9, 2024

Uh oh!

XLTechie commented Feb 9, 2024 via email

Uh oh!

XLTechie commented Feb 9, 2024 via email

Uh oh!

CyrilleB79 commented Feb 9, 2024

Uh oh!

Brian1Gaff commented Feb 9, 2024 via email

Uh oh!

XLTechie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

XLTechie Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

XLTechie Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

SaschaCowley Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrilleB79 commented Feb 9, 2024

Uh oh!

Uh oh!

Uh oh!

SaschaCowley Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

SaschaCowley Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

CyrilleB79 commented Feb 8, 2024 •

edited

Loading