Only close seika connection if initialized by seanbudd · Pull Request #13684 · nvaccess/nvda

seanbudd · 2022-05-11T06:01:26Z

Link to issue number:

Closes part of #13679

Summary of the issue:

It appears that when resetting a config, the seika notetaker driver can be terminated before it has been initialized.

Description of how this pull request fixes the issue:

Only terminate the notetaker driver if it has been initialized.

Testing strategy:

@cary-rowen to test a build of this PR

Known issues with pull request:

Another issue is highlighted in #13679. While terminating/initializing during a config change, the braille.handler is attempted to be used when handling events on NVDA objects.
This is a more complex problem and needs a braille device to reproduce and test.

Change log entries:

Fixes an unreleased regression

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

cary-rowen · 2022-05-11T07:45:44Z

I tested this and still reproduce #13679

cary-rowen · 2022-05-11T07:46:12Z

log.txt

AppVeyorBot · 2022-05-12T00:10:51Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/xa2u4s7l4qpktxfa/artifacts/output/nvda_snapshot_pr13684-25391,72ce6ecf.exe

See test results for failed build of commit 72ce6ecf1f

cary-rowen · 2022-05-12T03:02:50Z

Hi, @seanbudd
I tested the latest build and NVDA can catch this error:

ERROR - brailleDisplayDrivers.seikantk.BrailleDisplayDriver.terminate (10:50:45.576) - MainThread (10400):
Seika Notetaker driver not initialized

There seems to be another Error and I'm not quite sure how it happened:

RuntimeError: wrapped C/C++ object of type BoxSizer has been deleted
ERROR - unhandled exception (10:50:45.697) - MainThread (10400):
Traceback (most recent call last):
  File "wx\core.pyc", line 3407, in <lambda>
  File "gui\settingsDialogs.pyc", line 1457, in refreshGui

But my configuration can be reverse as expected.

Below is the full log:
log.txt

seanbudd · 2022-05-12T03:33:17Z

Thanks @cary-rowen - I will change the log level of that message but this is as expected.

XLTechie · 2022-05-12T03:34:30Z

Rowen wrote:

There seems to be another Error and I'm not quite sure how it happened: RuntimeError: wrapped C/C++ object of type BoxSizer has been deleted

I have also been seeing that on config resets lately. I suspect it is being caused by an add-on not (properly) cleaning up its config panel in terminate(), but haven't gone through the hassle of figuring out which one yet, if it is one and not core. Unfortunately it doesn't happen with every reset, so has been difficult to pin down. Next time please post a debug log.

seanbudd · 2022-05-12T03:38:48Z

Rowen wrote:
There seems to be another Error and I'm not quite sure how it happened: RuntimeError: wrapped C/C++ object of type BoxSizer has been deleted
I have also been seeing that on config resets lately. I suspect it is being caused by an add-on not (properly) cleaning up its config panel in terminate(), but haven't gone through the hassle of figuring out which one yet, if it is one and not core. Unfortunately it doesn't happen with every reset, so has been difficult to pin down. Next time please post a debug log.

This is a problem with NVDA core - I have noticed this without add-ons installed.

cary-rowen · 2022-05-12T04:52:00Z

Hi, @seanbudd

I tested it again and it seems to be working fine so far, Thanks for your efforts.
Please remember to put it on the 2022.1 milestone.

Thanks

seanbudd · 2022-05-12T05:36:24Z

Hi, @seanbudd

I tested it again and it seems to be working fine so far, Thanks for your efforts. Please remember to put it on the 2022.1 milestone.

Thanks

Please refer to this comment I've added to the issue
#13679 (comment)

feerrenrut · 2022-05-12T07:37:44Z


 class BrailleDisplayDriver(braille.BrailleDisplayDriver):
-	_dev: hwIo.IoBase
+	_dev: Optional[hwIo.IoBase] = None


Should this actually be made an instance variable?

this has been changed in a146544

feerrenrut · 2022-05-12T07:39:19Z

+		except Exception as e:
 			# The display driver seems to be failing, but we're terminating anyway, so just ignore it.
-			pass
+			log.debugWarning(f"Display driver {self} is failing while terminating. {e}")


Shouldn't this be logged at error level?

I agree, this has been changed in 941823b

seanbudd · 2022-05-16T03:12:01Z

@cary-rowen - if we end up doing a 2022.1rc2 due to #13694, I think the fix for this issue should be included as well.

cary-rowen · 2022-05-16T03:14:43Z

@seanbudd
I agree, I'll take a look too #13694

Only close seika connection if initialized

8cb44b8

fix typing for uninitialized seikantk

4b398d4

seanbudd marked this pull request as ready for review May 12, 2022 03:33

seanbudd requested a review from a team as a code owner May 12, 2022 03:33

seanbudd requested a review from feerrenrut May 12, 2022 03:33

warn at lower level

250ff18

seanbudd self-assigned this May 12, 2022

feerrenrut reviewed May 12, 2022

View reviewed changes

seanbudd added 2 commits May 16, 2022 14:02

make _dev an instance variable

a146544

log at error level

941823b

seanbudd changed the base branch from master to beta May 16, 2022 04:06

seanbudd requested a review from feerrenrut May 16, 2022 04:15

remove accidental import

b7241e8

feerrenrut approved these changes May 16, 2022

View reviewed changes

seanbudd merged commit 7b488cc into beta May 16, 2022

seanbudd deleted the revertconfigFixes branch May 16, 2022 06:04

nvaccessAuto added this to the 2022.2 milestone May 16, 2022

seanbudd modified the milestones: 2022.2, 2022.1 May 16, 2022

seanbudd mentioned this pull request May 19, 2022

Merge beta to rc for 2022.1rc2 #13709

Merged

seanbudd linked an issue May 19, 2022 that may be closed by this pull request

Revert configuration gives error #13679

Closed

Uh oh!

Conversation

seanbudd commented May 11, 2022 • 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 entries:

Code Review Checklist:

Uh oh!

cary-rowen commented May 11, 2022

Uh oh!

cary-rowen commented May 11, 2022

Uh oh!

AppVeyorBot commented May 12, 2022

Uh oh!

cary-rowen commented May 12, 2022

Uh oh!

seanbudd commented May 12, 2022

Uh oh!

XLTechie commented May 12, 2022 via email

Uh oh!

seanbudd commented May 12, 2022

Uh oh!

cary-rowen commented May 12, 2022

Uh oh!

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

Uh oh!

Uh oh!

feerrenrut May 12, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd May 16, 2022

Choose a reason for hiding this comment

Uh oh!

feerrenrut May 12, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd May 16, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd commented May 16, 2022

Uh oh!

cary-rowen commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seanbudd commented May 11, 2022 •

edited

Loading

seanbudd commented May 12, 2022 •

edited

Loading