Refactor more speech functions #10593
Merged
Conversation
This file contains 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
This comment has been minimized.
This comment has been minimized.
|
@zstanecic : These changes should not affect synthDrivers at all.
|
This comment has been minimized.
This comment has been minimized.
|
@zstanecic and @Brian1Gaff This PR does not break backwards compatibility. The description has been updated to make that clear. |
feerrenrut
force-pushed
the
refactorMoreSpeechFunctions
branch
from
400554c
to
a48f1e4
Compare
LeonarddeR
approved these changes
Dec 9, 2019
There was a problem hiding this comment.
This looks good to me, however note that I only had to had time to look at the code briefly, I haven't tested this. I therefore recommend a look from @michaelDCurran before merging this.
michaelDCurran
requested changes
Dec 9, 2019
There was a problem hiding this comment.
- Would it be possible to make a new type annotation for 'reason'? All through this code you have reason: str. It would be great if this could be its own type.
- with all the speakSelection functions: it would be good if they could take speech sequences, rather than just strings. Then speakSelectionChange can pass in a sequence from getTextInfoSpeech rather than just TextInfo.text. It also means that some of the code in speakSelectionChange can be replaced with a call to getSpellingSpeech. However, these changes would mean we can't have the text included with the message as part of one translatable string. Though I don't believe this is a bad thing.
See test results for failed build of commit 4ee5ba34ef |
feerrenrut
force-pushed
the
refactorMoreSpeechFunctions
branch
from
29e56b8
to
193be74
Compare
feerrenrut
force-pushed
the
refactorMoreSpeechFunctions
branch
from
5602078
to
582cb55
Compare
|
Is it just me, or have the changes to |
I think it's just you 😉. It is hidden from the diff initially because it is "large". You can jump to it, or other files by pressing 't' and typing |
michaelDCurran
approved these changes
Jan 29, 2020
…Info Clarify that the sequence is modified in place, nothing is spoken.
For now deprecate (but maintain) the old name.
For easier testing / refactoring.
Use instance methods directly.
Make speakWithoutPauses an instance method - To make it more testable. - Keep backwards compat; speakWithoutPauses name is added at module level. Make getSpeechWithoutPauses a generator with return value. - Speech can happen as it is generated, with the same number of calls to speak as previous implementation. - Returns the same boolean values as previous implementation. - To capture these return values, use GeneratorWithReturn class. Add unit test for speechWithoutPauses - To ensure behavior has not changed between implementations
- Speech can happen as it is generated, with the same number of calls to speak as previous implementation. - Returns the same boolean values as previous implementation. - To capture these return values, use GeneratorWithReturn class.
Simplify the speakObject function by extracting this calculation into a helper.
The new method '_objectSpeech_calculateAllowedProps()', was extracted from 'speakObject()' and needs formatting to pass linting checks.
Must flatten the results from getTextInfoSpeech generator. Rename _speakPlaceholderIfEmpty -> _getPlaceholderSpeechIfTextEmpty
|
I'm going to rebase and merge this PR (to preserve the history of the refactor). I'll push after the rebase so that the commits in this PR / branch match. |
feerrenrut
force-pushed
the
refactorMoreSpeechFunctions
branch
from
582cb55
to
9a884eb
Compare
feerrenrut
added a commit
that referenced
this pull request
Jan 29, 2020
Split several functions into two. One speakX acts as before, but relies on a new getXspeech which returns a speech sequence. This PR maintains backwards compatibility. This branch is being merged to preseve history of the refactor. Merge remote-tracking branch 'origin/refactorMoreSpeechFunctions'
feerrenrut
added a commit
that referenced
this pull request
Jan 29, 2020
seanbudd
added
the
deprecated/2021.1
This was referenced Mar 5, 2021
7 tasks
seanbudd
pushed a commit
that referenced
this pull request
Mar 22, 2021
Removes code provided for backwards compatibility in PR #10593 ### Summary of the issue: PR #10593 introduced `speech.speakWithoutPauses ` as an alias for `speech._speakWithoutPauses.speakWithoutPauses` and `speech.re_last_pause` as an alias for `speech._speakWithoutPauses.re_last_pause` for backwards compatibility. Since 2021.1 is going to be backwards compatibility breaking release it makes sense to remove these. ### Description of how this pull request fixes the issue: These aliases are removed and their usages are replaces with `speech.SpeechWithoutPauses(speakFunc=speech.speak).speakWithoutPauses` and `speech.SpeechWithoutPauses.re_last_pause` respectively.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Link to issue number:
None
Summary of the issue:
Returning sequences from these functions allows us greater flexibility for affecting all speech that is caused by a call to speakObject.
Description of how this pull request fixes the issue:
Split several functions into a speakX and getXspeech
This PR maintains backwards compatibility
Testing performed:
Ran NVDA from source.
Known issues with pull request:
Change log entry:
Section: New features, Changes, Bug fixes