speechManager: Ensure handling of skipped indexes

[michaelDCurran]

Link to issue number:

Fixes #10292

Summary of the issue:

Synths such as OneCore and sapi5 seem to fail to fire callbacks for bookmarks in speech that directly preceed another bookmark, with no text content in between.
This can happen if doing a sayAll which contains blank lines in the middle.
SpeechManager currently expects that each and every index will be received. But in this case, the index for the blank line is not recieved, and therefore speech stops.
This was not an issue with 2019.2 and below, presumably because we didn't care so much about indexes for each and every utterance.

Description of how this pull request fixes the issue:

A list of indexes currently sent to the synth is now tracked. I.e. the index value of all IndexCommand objects that are in the sequence that go to synth.speak go into a indexesSpeaking instance variable on SpeechManager.
_handleIndex now has an optional handleSkippedIndexes argument, which is True by default. If true, then it looks back through indexesSpeaking, and calls _handleIndex manually for any index lower than the current index.
_handleIndex also removes index from indexesSpeaking.
_reset also clears indexesSpeaking.

Testing performed:

With Onecore, sapi5 and espeak: did a say All on the following content:

1.
2.
3.
4.

5.
6.
7.
8.

Before the fix, NVDa would stop reading at 5.
After the fix, it reaches the end.

Known issues with pull request:

None known.

Change log entry:

None needed.

[AppVeyorBot]

• PR introduces Flake8 errors
• Build (for testing PR)

See test results for failed build of commit fa4b50d881

[AppVeyorBot]

• Unit test failure
• Build (for testing PR)

See test results for failed build of commit a33b34b527

[CyrilleB79]

This build fixes OneCore but not the eSpeak issue that I describe in #10612. Should I open a new issue or will this be investigated in this PR.

[michaelDCurran]

#10612 is about Excel. Perhaps you meant to say #10292? Please open a new issue specific for the eSpeak problem.  I'd like to specifically know if the eSpeak issue is only in 2019.3, or existed in 2019.2 as well.

[CyrilleB79]

On the contrary to what I stated in my previous comment, it seems that the eSpeak problem is also resolved by this PR. At least, I cannot reproduce it anymore with this PR's build.
Maybe I have stopped the sayAll by inadvertently pulling the mouse slightly during my first test.

[feerrenrut]

I think speech manager is in fairly dire need of unit tests.

[feerrenrut]

I'd prefer if this was not recursive, I think it only adds extra complexity, because this function now does several things and recursing the skipped indexes is just one part. I think that _pushNextSpeech should only be called once. Despite my initial feeling that _removeCompletedFomQueue should only be called once and it being less efficient than handling this in bulk, it looks like it necessary. This should be well documented. It seems a shame that we are searching through the sequence for these indexes already (in _removeCompletedFomQueue). Any way, removing the recursive approach you could achieve the same with:

	def _handleIndex(self, index):
		# A synth (such as OneCore) may skip indexes
		# If before another index, with no text content in between.
		# Therefore, collect all skipped indexes.
		handleIndexes = []
		for oldIndex in self._indexesSpeaking:  # no need to take a copy anymore
			if oldIndex < index:
				log.debugWarning("Handling skipped index %s" % oldIndex)
				handleIndexes.append(oldIndex)
		handleIndexes.append(index)

		# a more concise approach to create handleIndexes:
		self._indexesSpeaking.sort()  # Note: if we wish to maintain out of order indexes (which I don't think we should allow) use s = sorted()
		indexOfIndex = self._indexesSpeaking.index(index)
		handleIndexes = self._indexesSpeaking[:indexOfIndex]

		valid, endOfUtterance = False, False
		for i in handleIndexes:
			try:
				self._indexesSpeaking.remove(i)
			except ValueError:
				log.error("Unknown index %s" % i)
				break  # try the rest, this is a very unexpected path.
			if i != index:
				log.debugWarning("Handling skipped index %s" % i)

			# we must do the following for each index, any/all of them may be end of utterance, which must
			# trigger _pushNextSpeech
			_valid, _endOfUtterance = self._removeCompletedFromQueue(i)
			valid = valid or _valid
			endOfUtterance = endOfUtterance or _endOfUtterance
			
			if _valid:
				callbackCommand = self._indexesToCallbacks.pop(index, None)
				if callbackCommand:
					try:
						callbackCommand.run()
					except:
						log.exception("Error running speech callback")
		if endOfUtterance:
			# Even if we have many indexes, we should only push next speech once.
			self._pushNextSpeech(False)

[feerrenrut]

I'm a little concerned about the effect this will have on the timing of the callbacks. Or resulting callbacks being called that were intended to be associated with text that the synth decided not to speak. Though, thinking about this from oncore's side. If there are several indexes in sequence, with no speakable text in between, it would seem like a fairly logical optimization to only call the last one.

[michaelDCurran]

I committed your suggested rewrite, however, I still copied _indexesSpeaking and use log.debug rather than log.error when handling an exception when removing the item. Reason being that errors were constantly being rased when cancelling speech, as indexes were being removed from _indexesSpeaking in the main thread, yet _handleIndex is run from a background thread.

[feerrenrut]

as indexes were being removed from _indexesSpeaking in the main thread, yet _handleIndex is run from a background thread.

I think there must be something else going on, check

nvda/source/speech/manager.py

Line 331 in cbd8f24

queueHandler.queueFunction(queueHandler.eventQueue, self._handleIndex, index)

It would also be bad if these callbacks were called from another thread.

[feerrenrut]

This message is emitted later as well.

[feerrenrut]

I think it's best if this is merged, so it can get wider testing over the weekend. Since the plan is to release an RC on Monday. I will merge this and make another Beta shortly. It may warrant a followup, since there seems to be some assumptions that don't hold.

[zstanecic]

Hi reef, Will this be merged into master?

[feerrenrut]

It's first merged into beta, I'll merge beta into master shortly.

[michaelDCurran]

After investigating this further, it seems that this is not a threading issue. @feerrenrut was correct that everything should be executed on the main thread, and @jcsteh also confirmed to me this was the way it was designed.
However, what is actually happening (a lot of the time for sapi5) and rarely for eSpeak, is that once manager._reset is called (from cancelSpeech), a synth may still fire at least one index from the previous speech. In this pr, _indexesSpeaking is cleared on _reset, but then _handleIndex is called with an index that used to be in _indexesSpeaking and is now not (as it was cleared).
Nothing to do for 2019.3 as the error is currently detected and ignored. But eventually, _handleIndex probably should check this higher up. I.e. if index is not in _indexesSpeaking at all, then this is a roag index from previous speech.