Speech manager no longer sends synths utterances containing only param change and index commands. #11651
Merged
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.
Contributor
|
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 |
feerrenrut
suggested changes
Sep 21, 2020
feerrenrut
left a comment
Contributor
There was a problem hiding this comment.
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.
feerrenrut
previously approved these changes
Sep 22, 2020
feerrenrut
left a comment
Contributor
There was a problem hiding this comment.
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>
This was referenced Sep 29, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: