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
Speech manager no longer sends synths utterances containing only param change and index commands. #11651
Conversation
…ly contain synth param commands and indexCommands. This was causing some synths to ignore the index.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Since this change is probably too risky to put into the beta. I'm thinking that it might be best to merge #11586 into beta, so the main symptom is resolved in 2020.3 then revert it in this PR when merging to master for 2020.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add a test specifically for the case that this solves:
- to help document that it is an explicit decision on behavior.
- to catch regressions if / when this is refactored.
Potentially this could be done with tests for _processSpeechSequence
or ensureEndUtterance
directly rather than the whole SpeechManager
class.
…tedIndex directly.
…ntaining param change and index commands is no longer emmitted after an utterance that contains param change commands and ends in an EndUtterance command E.g. speaking a character.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks for adding the extra test.
A couple of minor things, but overall I think this is ready to go!
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
An alternative to pr #11586
Link to issue number:
Fixes #11168
Summary of the issue:
NvDA's speech manager splits speech sequences on endUtterance commands, and ensures that each utterance ends in an index command. However, it was possible that this would result in a speech sequence being sent to the synth that only contained param change commands and a final lindex. This is not only a redundant sequence, but some synths such as SAPI5 Ivona, could ignore the index completely, as the sequence did not actually contain any speech. And therefore, this would cause NVDA to not receive the final index callback and therefore not speak any further queued speech. For example in issue #11168 typing a letter in a list: NVDA would speak (spell) that character, but then fail to announce the newly highlighted list item.
Description of how this pull request fixes the issue:
Improved the logic in _processSpeechSequence and its ensureEndUtterance inner function, so that replaying of param change commands after the end of one utterance only occurs if there is actually more speech. And, therefore another index won't pointlessly be added after the param changes.
The test_4_profiles speech manager unit test had to be significantly rewritten though as this changed the numbering of indexes, as that test seemed to expect the redundant param change and index commands (specifically between two configProfileSwitch commands with no actual speech).
Testing performed:
Performed the stesps in issue #11168 while using an Australian SAPI5 Ivona demo voice from https://www.harposoftware.com/
Before the test NvDA would not say anything after speaking the typed character in the list. After, it also correctly spoke the list item as well.
Known issues with pull request:
Although this directly addresses the problems with Ivona and therefore fixes issue #11168 , it does not fix issue #10901 which is somewhat worse and deals with much older software.
Change log entry:
Bug fixes: