-
-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor of sayAllHandler into speech #12251
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
With the code from this PR NVDA crashes after restart with the following log:
Could you please look into this? |
See test results for failed build of commit 6d921fc3ee |
This comment has been minimized.
This comment has been minimized.
a simple fix for this may be to import |
Why you've decided not to convert activesayall to a class after all? |
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. |
e689543
to
48e045f
Compare
See test results for failed build of commit 5c9c8d6fb2 |
source/core.py
Outdated
@@ -303,6 +303,9 @@ def main(): | |||
import speech | |||
log.debug("Initializing speech") | |||
speech.initialize() | |||
from speech import sayAll | |||
log.debug("Initializing sayAllHandler") | |||
sayAll.SayAllHandler.initializeSpeechWithoutPauses() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make more sense to do this in speech.initialize()
so that they can fetch it from speech, not core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
See test results for failed build of commit 1c5fc7ee72 |
I tried to start clean at master (@ 0efeee4 ) to make reviewing this easier. The commits from
|
Fixing the lint as a result of file moves was quite involved. Here was my process linting
|
0df0f5b
to
eb02c71
Compare
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. |
Yes, the intentions was to copy |
0efe737
to
eb72dd7
Compare
… 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.
eb72dd7
to
ac6fae8
Compare
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.
ac6fae8
to
0c4dacb
Compare
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.
0c4dacb
to
b95743d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @seanbudd!
Seems to have caused #12393 Btw, sorry if this has been discussed before, but why did you write Seeing the class named |
source/speech/speech.py
Outdated
import sayAllHandler | ||
sayAllHandler.stop() | ||
sayAllHandler.getSpeechWithoutPauses().reset() | ||
from speech import sayAll | ||
sayAll.SayAllHandler.stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #12396
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() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #12396
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
Link to issue number:
Follow up of #12228, which fixed #12225
Summary of the issue:
SpeechWithoutPauses
is only used bysayAllHandler
, but the code lies inspeech\__init__.py
. Due to code changessayAllHandler
needs to instantiate aSpeechWithoutPauses
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 modulespeech\__init__.py
has been moved tospeech\speech.py
so thatspeech.sayAll
can import the necessary functions fromspeech
.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
andspeechWithoutPauses.py
to be reenabled (tracked by #12377)Change log entry:
Developer Changes
Changes to existing changelog items:
New developer changes:
sayAllHandler
has been moved tospeech.sayAll
(refactor of sayAllHandler into speech #12251):speech.sayAll.SayAllHandler
exposes the functionsstop
,isRunning
,readObjects
,readText
,lastSayAllMode
.SayAllHandler.stop
also resets theSayAllHandler
SpeechWithoutPauses
instance.CURSOR_REVIEW
andCURSOR_CARET
has been replaced withCURSOR.REVIEW
andCURSOR.CARET
.Code Review Checklist: