refactor of sayAllHandler into speech by seanbudd · Pull Request #12251 · nvaccess/nvda

seanbudd · 2021-03-30T23:52:23Z

Link to issue number:

Follow up of #12228, which fixed #12225

Summary of the issue:

SpeechWithoutPauses is only used by sayAllHandler, but the code lies in speech\__init__.py. Due to code changes sayAllHandler needs to instantiate a SpeechWithoutPauses instance, and would either introduce a circular dependency or require a singleton to be created when the instance is needed.

Description of how this pull request fixes the issue:

sayAllHandler is moved to a new module - speech.sayAll.
SpeechWithoutPauses is moved to it's own module
speech\__init__.py has been moved to speech\speech.py so that speech.sayAll can import the necessary functions from speech.
sayAllHandler top level functions have been refactor into a class. An instance of SayAllHandler is initialised when NVDA is started in a consistent manner with other initialisations in the code base.

Testing strategy:

Ensure #12225 is still fixed - ensure that sayAll still no longer mixes content from different runs.

Ensure the API changes match the changelog entry

Known issues with pull request:

Linting has been disabled on speech.py and speechWithoutPauses.py to be reenabled (tracked by #12377)

Change log entry:

Developer Changes

Changes to existing changelog items:

- - `speakTextInfo` will no longer send speech through `speakWithoutPauses` if reason is `SAYALL`, as `sayAllhandler` does this manually now. (#12150)
+ - `speakTextInfo` will no longer send speech through `speakWithoutPauses` if reason is `SAYALL`, as `SayAllHandler` does this manually now. (#12150)

- - `speech.speakWithoutPauses` has been removed - please use `speech.SpeechWithoutPauses(speakFunc=speech.speak).speakWithoutPauses` instead. (#12195)
- - `speech.re_last_pause` has been removed - please use `speech.SpeechWithoutPauses.re_last_pause` instead. (#12195)
+ - `speech.speakWithoutPauses` has been removed - please use `speech.speechWithoutPauses.SpeechWithoutPauses(speakFunc=speech.speak).speakWithoutPauses` instead. (#12195, #12251)
+ - `speech.re_last_pause` has been removed - please use `speech.speechWithoutPauses.SpeechWithoutPauses.re_last_pause` instead. (#12195, #12251)

New developer changes:

sayAllHandler has been moved to speech.sayAll (refactor of sayAllHandler into speech #12251):
- speech.sayAll.SayAllHandler exposes the functions stop, isRunning, readObjects, readText, lastSayAllMode.
- SayAllHandler.stop also resets the SayAllHandler SpeechWithoutPauses instance.
- CURSOR_REVIEW and CURSOR_CARET has been replaced with CURSOR.REVIEW and CURSOR.CARET.

Code Review Checklist:

lukaszgo1 · 2021-03-31T10:04:54Z

Hi @seanbudd Thanks for this PR! I agree that code is much more readable now. Unfortunately in some rare cases with code from this PR NVDA can crash due to circular imports.
STR:

Download Report passwords 1.0 or for that matter any add-on which imports gui in the installTasks module.
Install it and then try to uninstall.

With the code from this PR NVDA crashes after restart with the following log:

ERROR - addonHandler.Addon.completeRemove (11:53:58.056) - MainThread (5644):
task 'onUninstall' on addon 'reportPasswords' failed
Traceback (most recent call last):
  File "addonHandler\__init__.py", line 330, in completeRemove
    self.runInstallTask("onUninstall")
  File "addonHandler\__init__.py", line 466, in runInstallTask
    self._installTasksModule=self.loadModule('installTasks')
  File "addonHandler\__init__.py", line 445, in loadModule
    return loader.load_module(fullname)
  File "C:\Python38\lib\pkgutil.py", line 285, in load_module
    mod = imp.load_module(fullname, self.file, self.filename, self.etc)
  File "C:\Python38\lib\imp.py", line 234, in load_module
    return load_source(name, filename, file)
  File "C:\Python38\lib\imp.py", line 171, in load_source
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 702, in _load
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "userConfig\addons\reportPasswords\installTasks.py", line 7, in <module>
    import gui
  File "gui\__init__.py", line 18, in <module>
    import ui
  File "ui.py", line 21, in <module>
    import speech
  File "speech\__init__.py", line 16, in <module>
    import api
  File "api.py", line 12, in <module>
    import review
  File "review.py", line 10, in <module>
    from NVDAObjects import NVDAObject, NVDAObjectTextInfo
  File "NVDAObjects\__init__.py", line 18, in <module>
    import eventHandler
  File "eventHandler.py", line 16, in <module>
    import treeInterceptorHandler
  File "treeInterceptorHandler.py", line 11, in <module>
    import documentBase
  File "documentBase.py", line 7, in <module>
    from scriptHandler import isScriptWaiting
  File "scriptHandler.py", line 13, in <module>
    import sayAllHandler
  File "sayAllHandler.py", line 30, in <module>
    _speechWithoutPausesInstance = speech.SpeechWithoutPauses(speakFunc=speech.speak)
AttributeError: partially initialized module 'speech' has no attribute 'SpeechWithoutPauses' (most likely due to a circular import)
DEBUG - addonHandler.Addon.completeRemove (11:53:58.096) - MainThread (5644):
removing addon reportPasswords from _disabledAddons/_blockedAddons
DEBUG - addonHandler._getAvailableAddonsFromPath (11:53:58.096) - MainThread (5644):
Listing add-ons from D:\my_repos\nvda\source\userConfig\addons
DEBUG - addonHandler._getAvailableAddonsFromPath (11:53:58.096) - MainThread (5644):
Loading add-on from D:\my_repos\nvda\source\userConfig\addons\charInfo
DEBUG - addonHandler._getAvailableAddonsFromPath (11:53:58.096) - MainThread (5644):
Found add-on charInfo - 1.4. Requires API: (2017, 3, 0). Last-tested API: (2019, 3, 0)
DEBUG - addonHandler._getAvailableAddonsFromPath (11:53:58.096) - MainThread (5644):
Loading add-on from D:\my_repos\nvda\source\userConfig\addons\columnsReview
DEBUG - addonHandler._getAvailableAddonsFromPath (11:53:58.096) - MainThread (5644):
Found add-on columnsReview - 3.0-20210115-dev. Requires API: (2017, 3, 0). Last-tested API: (2020, 4, 0)
DEBUG - core.main (11:53:58.096) - MainThread (5644):
Initializing appModule Handler
DEBUG - core.main (11:53:58.096) - MainThread (5644):
Initializing NVDAHelper
DEBUG - core.main (11:53:58.156) - MainThread (5644):
Initializing tones
DEBUG - core.main (11:53:58.166) - MainThread (5644):
Speech Dictionary processing
DEBUG - speechDictHandler.SpeechDict.load (11:53:58.166) - MainThread (5644):
Loading speech dictionary 'D:\my_repos\nvda\source\userConfig\speechDicts\default.dic'...
DEBUG - speechDictHandler.SpeechDict.load (11:53:58.166) - MainThread (5644):
file 'D:\my_repos\nvda\source\userConfig\speechDicts\default.dic' not found.
DEBUG - speechDictHandler.SpeechDict.load (11:53:58.166) - MainThread (5644):
Loading speech dictionary 'D:\my_repos\nvda\source\builtin.dic'...
DEBUG - speechDictHandler.SpeechDict.load (11:53:58.166) - MainThread (5644):
3 loaded records.
CRITICAL - __main__ (11:53:58.176) - MainThread (5644):
core failure
Traceback (most recent call last):
  File "nvda.pyw", line 264, in <module>
    core.main()
  File "core.py", line 303, in main
    import speech
  File "speech\__init__.py", line 16, in <module>
    import api
  File "api.py", line 12, in <module>
    import review
  File "review.py", line 10, in <module>
    from NVDAObjects import NVDAObject, NVDAObjectTextInfo
  File "NVDAObjects\__init__.py", line 18, in <module>
    import eventHandler
  File "eventHandler.py", line 16, in <module>
    import treeInterceptorHandler
  File "treeInterceptorHandler.py", line 11, in <module>
    import documentBase
  File "documentBase.py", line 7, in <module>
    from scriptHandler import isScriptWaiting
  File "scriptHandler.py", line 13, in <module>
    import sayAllHandler
  File "sayAllHandler.py", line 30, in <module>
    _speechWithoutPausesInstance = speech.SpeechWithoutPauses(speakFunc=speech.speak)
AttributeError: partially initialized module 'speech' has no attribute 'SpeechWithoutPauses' (most likely due to a circular import)

Could you please look into this?

AppVeyorBot · 2021-04-01T03:33:47Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 6d921fc3ee

seanbudd · 2021-04-06T06:48:40Z

a simple fix for this may be to import sayAllHandler before the chain that eventually reaches speech (eg in ui or gui)

lukaszgo1 · 2021-04-06T19:54:30Z

Why you've decided not to convert activesayall to a class after all?
According to my test importing sayAllHandler in gui or in ui does not fix this. The simplest fix would be to import speech and ui lazily in the gui module. Having said that this is not a first time we are trying to work around a crash caused by some add-on importing gui in the installTasks module which suggests to me that gui is in a dire need of a refactor. Perhaps we should focus on making gui's layout more flexible and then come back to this PR?

feerrenrut · 2021-04-07T05:41:02Z

We agree that restructuring code would eventually resolve these kinds of issues. However, we'd like to see if we can find a minimal fix in case we run into issues during the refactor.

AppVeyorBot · 2021-04-07T08:16:48Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit 5c9c8d6fb2

feerrenrut · 2021-04-07T09:04:29Z

 	speech.initialize()
+	from speech import sayAll
+	log.debug("Initializing sayAllHandler")
+	sayAll.SayAllHandler.initializeSpeechWithoutPauses()


I think if we are going this far, then rather than make SayAllHandler a singleton, just construct it in core (passing speech function in). Other module that need it can be initialised after this and have the SayAllHandler instance passed to them, or if that is difficult with there current design, they can fetch it (at function level, not module level) from core.

would it make more sense to do this in speech.initialize() so that they can fetch it from speech, not core?

I've done this in 01b8c5f - I don't think we need to pass the speech function in. Wouldn't it be somewhat misleading considering that SayAllHandler should always use speech.speak?

AppVeyorBot · 2021-04-07T23:26:58Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 1c5fc7ee72

seanbudd · 2021-04-07T23:31:36Z

I tried to start clean at master (@ 0efeee4 ) to make reviewing this easier. The commits from 4ebb450 are based clean off master

>>>git diff  0efeee4b7dbdd243a7b880cf08dcbef284b48896 4ebb450

diff --git a/source/speech/__init__.py b/source/speech/speech.py
similarity index 100%
rename from source/speech/__init__.py
rename to source/speech/speech.py

seanbudd · 2021-04-08T01:45:45Z

Fixing the lint as a result of file moves was quite involved. Here was my process

linting `source\speech\speech.py` due to the file move

in the nvda virtual environment run the following (see b198d8f):

pip install autopep8==1.5.2 
autopep8 --global-config tests\lint\flake8.ini source\speech\speech.py -i -a -a

convert spaces to tabs with IDE (VS code) (see b198d8f)
manually fix long comments (see 5532502)
manually fix hanging indents (see 5532502)
remove unused variables and check if they are used globally (see 986ce76)
manually fix translator comments (see 613b38a, eb02c71)

fixing the imports in `source\speech\init.py` so that compatibility doesn't break (see `db99485`)

using git bash, generate 2 files which can be copy and pasted into source\speech\__init__.py:

# for from .speech import * in speech\__init__.py
# functions
sed -nE "s/^def ([A-z_][A-z0-9_]*).*$/\t\1,/p" source/speech/speech.py > imports.txt
# other variables
sed -nE "s/^([A-z_][A-z0-9_]*) = .*$/\t\1,/p" source/speech/speech.py >> imports.txt
# sort
sort imports.txt -o imports.txt

# for __all__ in speech\__init__.py
# functions
sed -nE 's/^def ([A-z_][A-z0-9_]*).*$/\t"\1",/p' source/speech/speech.py > all.txt
# other variables
sed -nE 's/^([A-z_][A-z0-9_]*) = .*$/\t"\1",/p' source/speech/speech.py >> all.txt
# sort
sort all.txt -o all.txt

manually include the functions that were being imported from .types and .priorities

AppVeyorBot · 2021-04-08T01:47:56Z

Build (for testing PR)

See test results for failed build of commit 3ad35eb39b

AppVeyorBot · 2021-04-08T02:04:01Z

Build (for testing PR)

See test results for failed build of commit 88e9d8f5ab

AppVeyorBot · 2021-04-08T02:30:06Z

Build (for testing PR)

See test results for failed build of commit 37ff5d7eaa

feerrenrut · 2021-04-09T07:34:46Z

If this is just moved code, perhaps we should consider not fixing the lint warnings. Instead we could temporarily disable lint as a failure on this branch (so that we get the benefits of the other tests). For a large scale formatting change it would be best if it did the whole code base at once and then we could ignore that revision explicitly.

seanbudd · 2021-05-06T23:47:01Z

In that commit functions like getObjectSpeech are created in a new file source/speech/speechWithoutPauses.py despite never having anything to do with speechWithoutPauses.
You might need to take another look at it, I'm not sure the result is what you intended.

Yes, the intentions was to copy speech\__init__.py entirely over, so that the changes in speech\speechWithoutPauses.py in this commit are easier to track (just removing irrelevant code). It would be good to keep the entire blame history for the functions in speech\speechWithoutPauses.py but I don't think git can track speech\__init__.py being split into two files, and then recreated with other content. I don't mind changing this to just moving the required functionality over. 5d2e6c1#diff-b01881d490e0d24548bdc4e134c2b6f0192d6f417abe4081fbd47b7d3c5422a1

… to speech/sayAll.py

SpeechWithoutPauses is only used by sayAllHandler, but the code lies in speech\__init__.py. Due to code changes sayAllHandler needs to instantiate a SpeechWithoutPauses instance, and would either introduce a circular dependency or require a singleton to be created when the instance is needed. Description of how this pull request fixes the issue: - sayAllHandler is moved to a new module - speech.sayAll. - SpeechWithoutPauses is moved to it's own module - speech\__init__.py has been moved to speech\speech.py so that speech.sayAll can import the necessary functions from speech. - sayAllHandler top level functions have been refactor into a class. An instance of SayAllHandler is initialised when NVDA is started in a consistent manner with other initialisations in the code base.

feerrenrut

Looks good @seanbudd!

JulienCochuyt · 2021-05-10T16:20:11Z

Seems to have caused #12393

Btw, sorry if this has been discussed before, but why did you write speech.sayAll.SayAllHandler with an initial capital "S" instead of speech.sayAll.sayAllHandler? (The usual convention being to capitalize types and not other attributes.)

Seeing the class named _SayAllHandler rings the bell to the common pattern class _C / C = _C / c = C() which allowed in Python 2 for easier injection of derived classes, but I guess that's out of point here.

seanbudd · 2021-05-11T03:53:30Z

-	import sayAllHandler
-	sayAllHandler.stop()
-	sayAllHandler.getSpeechWithoutPauses().reset()
+	from speech import sayAll
+	sayAll.SayAllHandler.stop()


seanbudd · 2021-05-11T04:00:42Z

+	def stop(self):
+		'''
+		Stops any active objects reader and resets the SayAllHandler's SpeechWithoutPauses instance
+		'''
+		active = self._getActiveSayAll()
+		if active:
+			active.stop()
+			self.speechWithoutPausesInstance.reset()
+


See <nvaccess/nvda#12251>

The function header for sayAllHandler is incorrect, and the behaviour of cancelling sayAll speech changed in #12251

NVDA fails to uninstall addons which import gui due to a circular dependency going from speech.py -> sayAll.py While the refactor of speech/sayAll #12251 was designed to make fixing this problem simpler, it did not actually fix this issue. Description of how this pull request fixes the issue: makes sayAll no longer dependent on speech by injecting the dependencies on initialization removes unused imports moves imports to lazy load

This comment has been minimized.

Sign in to view

seanbudd marked this pull request as ready for review March 31, 2021 05:32

seanbudd marked this pull request as draft April 1, 2021 03:15

seanbudd commented Apr 1, 2021

View reviewed changes

Comment thread source/sayAll.py Outdated

seanbudd commented Apr 1, 2021

View reviewed changes

Comment thread source/sayAll.py Outdated

This comment has been minimized.

Sign in to view

seanbudd added this to the 2021.1 milestone Apr 6, 2021

seanbudd force-pushed the sayAllHandler-refactor branch from e689543 to 48e045f Compare April 7, 2021 08:05

feerrenrut reviewed Apr 7, 2021

View reviewed changes

seanbudd mentioned this pull request Apr 8, 2021

Fix a load of linting issues #12261

Closed

seanbudd force-pushed the sayAllHandler-refactor branch from 0df0f5b to eb02c71 Compare April 8, 2021 02:40

seanbudd changed the title ~~simplify cancelling sayAll speech~~ refactor of sayAllHandler into speech Apr 8, 2021

seanbudd marked this pull request as ready for review April 8, 2021 05:10

seanbudd requested review from feerrenrut and michaelDCurran April 8, 2021 06:50

seanbudd self-assigned this Apr 8, 2021

seanbudd force-pushed the sayAllHandler-refactor branch from 0efe737 to eb72dd7 Compare May 6, 2021 23:52

move speech/__init__.py to speech/speech.py and move sayAllHandler.py…

57f2791

… to speech/sayAll.py

seanbudd force-pushed the sayAllHandler-refactor branch from eb72dd7 to ac6fae8 Compare May 7, 2021 05:51

seanbudd mentioned this pull request May 7, 2021

Linting was disabled on speech.py #12377

Closed

move SpeechWithoutPauses out of speech.py

509b7fa

seanbudd force-pushed the sayAllHandler-refactor branch from ac6fae8 to 0c4dacb Compare May 9, 2021 23:55

seanbudd requested a review from feerrenrut May 10, 2021 03:10

seanbudd mentioned this pull request May 10, 2021

Merge alpha into beta #12389

Closed

seanbudd force-pushed the sayAllHandler-refactor branch from 0c4dacb to b95743d Compare May 10, 2021 06:32

feerrenrut approved these changes May 10, 2021

View reviewed changes

seanbudd merged commit b60b974 into master May 10, 2021

seanbudd deleted the sayAllHandler-refactor branch May 10, 2021 07:00

JulienCochuyt mentioned this pull request May 10, 2021

Resuming speech with shift no longer works #12393

Closed

lukaszgo1 mentioned this pull request May 10, 2021

Fix _RowsReader for NVDA 2021.1 ABuffEr/columnsReview#14

Merged

seanbudd mentioned this pull request May 11, 2021

improve state management in speech.py #12395

Merged

7 tasks

seanbudd commented May 11, 2021

View reviewed changes

seanbudd mentioned this pull request May 11, 2021

always reset speechWithoutPauses instance when sayAll cancelled #12396

Merged

7 tasks

seanbudd commented May 11, 2021

View reviewed changes

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request May 11, 2021

Fix due to refactor of sayAllHandler into speech

5a3efbc

See <nvaccess/nvda#12251>

lukaszgo1 mentioned this pull request May 11, 2021

NVDA crashes when uninstalling add-ons importing gui at the module level in installTasks. #12399

Closed

MarcoZehe mentioned this pull request May 11, 2021

No external synth drivers work in latest alpha version #12400

Closed

seanbudd added a commit that referenced this pull request May 12, 2021

always reset speechWithoutPauses instance when sayAll cancelled (#12396)

5d4e64f

The function header for sayAllHandler is incorrect, and the behaviour of cancelling sayAll speech changed in #12251

seanbudd mentioned this pull request May 12, 2021

move/remove imports to prevent circular dependencies #12404

Merged

7 tasks

tmthywynn8 mentioned this pull request Sep 16, 2021

Fix REASON_* constant compatibility blindpandas/Audio-Themes-NVDA-Add-on#12

Closed

Uh oh!

Conversation

seanbudd commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entry:

Code Review Checklist:

Uh oh!

This comment has been minimized.

This comment has been minimized.

lukaszgo1 commented Mar 31, 2021

Uh oh!

AppVeyorBot commented Apr 1, 2021

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

seanbudd commented Apr 6, 2021

Uh oh!

lukaszgo1 commented Apr 6, 2021

Uh oh!

feerrenrut commented Apr 7, 2021

Uh oh!

AppVeyorBot commented Apr 7, 2021

Uh oh!

feerrenrut Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Apr 7, 2021

Uh oh!

seanbudd commented Apr 7, 2021

Uh oh!

seanbudd commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

linting source\speech\speech.py due to the file move

fixing the imports in source\speech\__init__.py so that compatibility doesn't break (see db99485)

Uh oh!

AppVeyorBot commented Apr 8, 2021

Uh oh!

AppVeyorBot commented Apr 8, 2021

Uh oh!

AppVeyorBot commented Apr 8, 2021

Uh oh!

feerrenrut commented Apr 9, 2021

Uh oh!

seanbudd commented May 6, 2021

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

JulienCochuyt commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanbudd May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanbudd May 11, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seanbudd commented Mar 30, 2021 •

edited

Loading

seanbudd Apr 8, 2021 •

edited

Loading

seanbudd commented Apr 8, 2021 •

edited

Loading

linting `source\speech\speech.py` due to the file move

fixing the imports in `source\speech\init.py` so that compatibility doesn't break (see `db99485`)

JulienCochuyt commented May 10, 2021 •

edited

Loading

seanbudd May 11, 2021 •

edited

Loading