Skip to content

Fix say all error when on Desktop#10723

Merged
feerrenrut merged 6 commits into
nvaccess:masterfrom
clementb49:try-I9947
Jul 31, 2020
Merged

Fix say all error when on Desktop#10723
feerrenrut merged 6 commits into
nvaccess:masterfrom
clementb49:try-I9947

Conversation

@clementb49

@clementb49 clementb49 commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Link to issue number:

fixes #9947

Summary of the issue:

If say all was invoked in non-text, NVDA raise an error.

Description of how this pull request fixes the issue:

This PR check that the _TextReader object has an attribute reader

Testing performed:

  1. Focus the desktop.
  2. Invoke caret say all
    Check that produce no error.

Known issues with pull request:

None

Change log entry:

None

@clementb49

clementb49 commented Jan 27, 2020

Copy link
Copy Markdown
Contributor Author

CC @LeonarddeR

@clementb49 clementb49 force-pushed the try-I9947 branch 4 times, most recently from fa424c9 to c8bf59c Compare January 27, 2020 17:17
@feerrenrut

feerrenrut commented Jan 30, 2020

Copy link
Copy Markdown
Contributor

Can you please resolve conflicts, and then request a review from me.

@clementb49 clementb49 requested review from feerrenrut and jcsteh and removed request for jcsteh January 31, 2020 21:34
feerrenrut
feerrenrut suggested changes Feb 3, 2020
View reviewed changes
Comment thread source/sayAllHandler.py Outdated
@feerrenrut feerrenrut changed the title fixe say all in non-text Fix say all error when on Desktop Feb 3, 2020
@clementb49 clementb49 requested a review from feerrenrut February 3, 2020 11:34
@feerrenrut

feerrenrut commented Feb 3, 2020

Copy link
Copy Markdown
Contributor

Can you mention why you chose not to re-raise the exception?

With this current approach there is still self.speakTextInfoState and self.numBufferedLines (line 118) that are not set, leaving the instance invalid and susceptible to similar issues in the future.

@clementb49

clementb49 commented Feb 5, 2020

Copy link
Copy Markdown
Contributor Author

For you, what is the exception which should be raise?

@feerrenrut

feerrenrut commented Feb 5, 2020

Copy link
Copy Markdown
Contributor

I think it should raise NotImplementedError and embed the information from the inner exception.

@clementb49

clementb49 commented Feb 5, 2020

Copy link
Copy Markdown
Contributor Author

I don't understand what I supose to do.
If I raise an 'NotImplementedError' in the except block of the init method, it makes no sense.

@feerrenrut

feerrenrut commented Feb 5, 2020

Copy link
Copy Markdown
Contributor

I'm suggesting that you raise the exception in the init method, and where is used you catch that exception and return early. The class is used in only one place (inside method def readText(cursor) Line 76 and is a private class (it starts with an underscore) so it should not be used by addons.

The benefit of this is that a half made instance isn't created.

	def __init__(self, cursor):
		self.cursor = cursor
		self.trigger = SayAllProfileTrigger()
		self.trigger.enter()
		# Start at the cursor.
		if cursor == CURSOR_CARET:
			try:
				self.reader = api.getCaretObject().makeTextInfo(textInfos.POSITION_CARET)
			except (NotImplementedError, RuntimeError) as e:
				raise NotImplementedError("Unable to make text info") from e  # Wrap the exception so we can easily add logging to see what happened.
		else:
			self.reader = api.getReviewPosition()

Then in def readText(cursor):

def readText(cursor):
	global lastSayAllMode, _activeSayAll
	lastSayAllMode=cursor
	try:
		reader = _TextReader(cursor)
	except NotImplementedError as e:
		log.debugWarning("Unable to make reader", exc_info=True)
		return
	_activeSayAll = weakref.ref(reader)
	reader.nextLine()

@AppVeyorBot

AppVeyorBot commented Feb 7, 2020

Copy link
Copy Markdown

See test results for failed build of commit 978b2da2da

@feerrenrut feerrenrut added the bug label Apr 8, 2020
@lukaszgo1

lukaszgo1 commented May 3, 2020

Copy link
Copy Markdown
Contributor

@clementb49 Are you planning to work further on this? It looks like only remaining think to be done is addressing a Linter error.

feerrenrut
feerrenrut approved these changes Jul 30, 2020
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs a change log entry, I don't believe this is a user visible error in a release, though certainly annoying for alpha users.

@feerrenrut feerrenrut merged commit 87eee7f into nvaccess:master Jul 31, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.3 milestone Jul 31, 2020
@Adriani90

Adriani90 commented Aug 2, 2020

Copy link
Copy Markdown
Collaborator

There is still an error when pressing nvda+a on desktop (keyboard is laptop layout):

IO - inputCore.InputManager.executeGesture (16:31:52.864) - winInputHook (14656):
Input: kb(laptop):NVDA+a
DEBUG - config.ConfigManager._triggerProfileEnter (16:31:52.881) - MainThread (13592):
Activating triggered profile Sayall
DEBUG - synthDriverHandler.SynthDriver.loadSettings (16:31:52.890) - MainThread (13592):
Loaded changed settings for SynthDriver ibmeci
DEBUGWARNING - sayAllHandler.readText (16:31:52.894) - MainThread (13592):
Unable to make reader
Traceback (most recent call last):
  File "sayAllHandler.pyc", line 116, in __init__
  File "documentBase.pyc", line 24, in makeTextInfo
  File "textInfos\offsets.pyc", line 470, in __init__
  File "textInfos\offsets.pyc", line 249, in _getCaretOffset
NotImplementedError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "sayAllHandler.pyc", line 80, in readText
  File "sayAllHandler.pyc", line 118, in __init__
NotImplementedError: Unable to make TextInfo: 
ERROR - stderr (16:31:52.895) - MainThread (13592):
Exception ignored in:
ERROR - stderr (16:31:52.950) - MainThread (13592):
<function _TextReader.__del__ at 0x038A1978>
ERROR - stderr (16:31:53.027) - MainThread (13592):
Traceback (most recent call last):
ERROR - stderr (16:31:53.096) - MainThread (13592):
  File "sayAllHandler.pyc", line 252, in __del__
ERROR - stderr (16:31:53.148) - MainThread (13592):
  File "sayAllHandler.pyc", line 245, in stop
ERROR - stderr (16:31:53.202) - MainThread (13592):
AttributeError
ERROR - stderr (16:31:53.255) - MainThread (13592):
:
ERROR - stderr (16:31:53.321) - MainThread (13592):
'_TextReader' object has no attribute 'reader'

This causes following user impact:

  • When creating a sayall profile which is deactivated, after pressing NVDA+a the error occurs and the sayall profile is applied. However, looking in the profiles dialog, the sayall profile is still deactivated although the settings from the sayall profile are currently applied. Very strange..

@Adriani90

Adriani90 commented Aug 2, 2020

Copy link
Copy Markdown
Collaborator

The only way to turn back to the settings before pressing nvda+a on desktop is to restart NVDA. Even activating and deactivating profiles does not work. The settings are not applied unless you restart NVDA.

@Adriani90 Adriani90 mentioned this pull request Aug 2, 2020
Closed
@lukaszgo1 lukaszgo1 mentioned this pull request Aug 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

2020.3

Development

Successfully merging this pull request may close these issues.

Say all on Desktop raises an error

6 participants

@clementb49 @feerrenrut @AppVeyorBot @lukaszgo1 @Adriani90 @nvaccessAuto