Stronger guarantee regarding Generators in speech#11040
Conversation
PRs #10878 and #10879 addressed some issues with Speech.speak being passed a Generator. Rather than create a list from it, ensure that the generator is not passed in the first place. The typing of several functions is no much more clear, in particular _flattenNestedSequences now no longer implicitly converts generators to lists. However, in all places it is used, generators are explicitly converted to lists. Logging of generator types has been added in several places, enabled via the advanced settings panel 'speech' debug logging category.
michaelDCurran
left a comment
There was a problem hiding this comment.
This looks okay. But I do think in future we should consider better standardization for the get*speech function names some how. some of them return speechSequences, some of them return generators:
getSpellingSpeech: Generator[SequenceItemT, None, None]
getObjectPropertiesSpeech: SpeechSequence
getObjectSpeech: unsure
getIndentationSpeech: SpeechSequence
getPreselectedTextSpeech: SpeechSequence
getTextInfoSpeech: Generator[SpeechSequence, None, bool]
getPropertiesSpeech: SpeechSequence
getControlFieldSpeech: SpeechSequence
getFormatFieldSpeech: SpeechSequence
getTableInfoSpeech: SpeechSequence
|
Before you merge, Please confirm you tested this as I had previously done in #10879. |
|
Re: names. I agree. Although arguably the typing info does that for those using an IDE. I completed the testing as per #10879
Perhaps this should be added as a system test? |
| reason=controlTypes.REASON_SAYALL, | ||
| useCache=state | ||
| ) | ||
| speechGen = speech.GeneratorWithReturn(speechGen) |
There was a problem hiding this comment.
This wasn't needed, since the return is never considered. Instead we look at the return from speakWithoutPauses to see if it spoke.
| ) | ||
| speechGen = speech.GeneratorWithReturn(speechGen) | ||
| seq = speech._flattenNestedSequences(speechGen) | ||
| seq = list(_flattenNestedSequences(speechGen)) |
There was a problem hiding this comment.
_flattenNestedSequences now returns a generator, so it can preserve generators.
| speak(sequence, priority=priority) | ||
|
|
||
|
|
||
| def _flattenNestedSequences(nestedSequences: Iterator[SpeechSequence]) -> Iterator[SequenceItemT]: |
There was a problem hiding this comment.
moved to speech.types
| return | ||
|
|
||
|
|
||
| class GeneratorWithReturn: |
There was a problem hiding this comment.
moved to speech.types
| priority: Optional[Spri] = None | ||
| ) -> bool: | ||
| speechSequences = getTextInfoSpeech( | ||
| speechGen = getTextInfoSpeech( |
There was a problem hiding this comment.
Renamed to avoid confusion with SpeechSequence type
| # Deprecation warning: In 2021.1 speakTextInfo will no longer send speech through speakWithoutPauses | ||
| # if reason is sayAll, as sayAllhandler does this manually now. | ||
| return _speakWithoutPauses.speakWithoutPauses(speechSequences) | ||
| log.error( |
There was a problem hiding this comment.
More explicit about deprecation. Perhaps we should start doing this rather than just a deprecation comment?
| if reason == controlTypes.REASON_SAYALL: | ||
| # Deprecation warning: In 2021.1 speakTextInfo will no longer send speech through speakWithoutPauses | ||
| # if reason is sayAll, as sayAllhandler does this manually now. | ||
| return _speakWithoutPauses.speakWithoutPauses(speechSequences) |
There was a problem hiding this comment.
This was a bug, speechSequences is a generator (now renamed to speechGen) but speakWithoutPauses does not accept a generator. It needs to be able to get the number of items in the sequence and slice it.
| if onlyInitialFields or any(isinstance(x, str) for x in speechSequence): | ||
| yield speechSequence | ||
| if not onlyInitialFields: | ||
| yield getSpellingSpeech( |
There was a problem hiding this comment.
This might have been a source of generators in speech. It certainly wasn't consistent with the return type defined on the function.
| implementation had several calls to speech, this approach replicates that. | ||
| """ | ||
| if speechSequence is not None: | ||
| logBadSequenceTypes(speechSequence) |
There was a problem hiding this comment.
Since getSpeechWithoutPauses can not accept an generator, also perform the check here.
|
I added some comments on the diff to explain the purpose of the changes in case it comes up in the future. |
Link to issue number:
From comment on #10879 (comment)
Summary of the issue:
The concern was that by suggesting that speech.speak may now accept a generator, the addon API had changed, potentially breaking addons.
PRs #10878 and #10879 addressed some issues with Speech.speak being passed a Generator.
Description of how this pull request fixes the issue:
Confirms that speech.speak does not accept a generator. Checked all locations that return generators for speech sequences ('getSpellingSpeech
andgetTextInfoSpeech`) and the functions that call them to ensure that speak isn't called with the generator.I added more typing information to functions to help narrow this down. Notably,
_flattenNestedSequenceswas implicitly creating a list. This has been fixed to return a generator which was the original intention, all usages have been checked to ensure they now explicitly convert to a list.An issue was found in the "say all" branch of
speakTextInfo, this has been resolved and the deprecation has been made more explicit.Logging of generator types has been added in several places, enabled via the advanced settings panel 'speech' debug logging category.
Testing performed:
Manual testing of speak spelling and say-all.
Known issues with pull request:
None
Change log entry:
None