Conversation
|
There are issues with the system tests to be investigated, but the fix has been manually confirmed. |
See test results for failed build of commit 20c633bcc5 |
See test results for failed build of commit f2ee2af9dc |
|
Note the failure from the about dialog test in #12957 (comment) |
| instances = list(cls._instances) | ||
| for instance in instances: | ||
| if instance and not instance.IsBeingDeleted() and instance.IsModal(): | ||
| instance.onOk(None) # Save and close |
There was a problem hiding this comment.
I'm uncomfortable that this will save the state of the checkboxes. The user may have fiddled with the checkboxes on this dialog, but not have pressed okay yet. Then when they exit, it will automaticaly comit those changes, if I am assuming right? We really need the same action as pressing escape... cancel... Though I do acknowledge that there is no cancel button on the dialog. Would this be possible?
There was a problem hiding this comment.
Closing this dialog without saving is also fine - it depends on what behaviour we prefer. Since there is no distinct "Save" vs "Cancel" button, it's hard to say what the default close action should be or what the user would expect.
Link to issue number:
Fixes #12907
Summary of the issue:
NVDA crashes if the welcome dialog is open, and was opened via the NVDA menu.
Description of how this pull request fixes the issue:
Track instances of the welcome dialog as they are created, close them all before the exit dialog is triggers an exit.
As the cause of this problem is complicated, and a proper fix is risky, this small patch avoids touching the core module.
Testing strategy:
Manual testing:
NVDA menu > HelpConfirm the other way to start a Welcome Dialog exits correctly.
System tests have been added which test the issue for quitting from keyboard via the Exit Dialog
Known issues with pull request:
The About Dialog still shares this problem, however it is harder to track instances of
wx.MessageBox, it will need to be subclassed. Note the failure from the about dialog test in #12957 (comment)This will be attempted in a separate PR.
Change log entries:
I think none needed - this is a rare bug fix that is a recent regression.
Code Review Checklist: