Exit NVDA safely by closing all top level windows by seanbudd · Pull Request #12183 · nvaccess/nvda

seanbudd · 2021-03-16T06:46:19Z

Link to issue number:

Summary of the issue:

Silent installs of NVDA were causing a crash at the end of the install process.

Upon further investigation it was found NVDA closes the wxApp by exiting the MainLoop. This is not recommended and the behaviour of closing the TopWindow is recommended.

Description of how this pull request fixes the issue:

Adds nvda_crash.dmp from the install process to AppVeyor artifacts for easier testing in future.

Create gui.safeAppExit that

Closes all open dialogs and windows
Forcibly close the main frame window, which will finally exit the app

From wxWidget docs

Call this to explicitly exit the main message (event) loop.

You should normally exit the main loop (and the application) by deleting the top window.

This function simply calls wxEvtLoopBase::Exit() on the active loop

Testing strategy:

Check the various ways of closing and restarting NVDA:

through the installation processes
through the exit dialog
exiting the process as a developer

Confirm the crash dump exists nvda_crash.dmp on this AppVeyor build and the build fails (https://ci.appveyor.com/project/NVAccess/nvda/builds/38282501)

Known issues with pull request:

None

Change log entry:

Changes

- NVDA will exit even with windows still open, the exit process now closes all NVDA windows and dialogs (#1740)

Code Review Checklist:

This reverts commit 80e41ed.

This reverts commit a469c4d.

AppVeyorBot · 2021-03-23T01:54:19Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit bd474b6be6

feerrenrut · 2021-03-23T09:01:38Z

I think it probably still makes sense to have a helper function for shutting down the application. It allows us to more easily change the shutdown procedure and to track errors such as being called repetitively.

The docs advise using wx.window.close rather than destroy. Destroy is called by close, but is the "more forceful" method.

seanbudd · 2021-03-24T03:20:48Z

I think it probably still makes sense to have a helper function for shutting down the application. It allows us to more easily change the shutdown procedure and to track errors such as being called repetitively.

I agree, I have moved the function yet again to be top level in gui. The destruction of the mainFrame would class the desctruction of the systray icon, which would destroy the systray menu. This process missed other dialog windows such as the WelcomeDialog which would prevent an exit.

The docs advise using wx.window.close rather than destroy. Destroy is called by close, but is the "more forceful" method.

We bind the ExitDialog to EVT_CLOSE so we can't use mainFrame.Close to close the window. (https://github.com/nvaccess/nvda/blob/fc6b73f/source/gui/__init__.py#L55)

https://stackoverflow.com/a/10837130

feerrenrut · 2021-03-25T05:37:16Z

We bind the ExitDialog to EVT_CLOSE so we can't use mainFrame.Close to close the window. >(https://github.com/nvaccess/nvda/blob/fc6b73f/source/gui/__init__.py#L55)

This is binding to the event on the "main frame". These events "bubble upwards" to parent controls if not handled or a handler calls evt.skip(). So yes, I guess they would get there. Close is used because it allows windows to veto, eg if they want to ask to save or similar. If we are confident that none of the locations we are calling from should make use of this, then destroy is fine. Actually, it is probable scope creep to consider it. Maybe just add a line to the docstring that this intentionally doesn't use close, and implementation for a veto-able shutdown procedure will need to be done from scratch and consider how MainFrame.onExitCommand(self, evt) interacts.

Summary of the issue: When restarting NVDA, WM_QUIT is posted as an event to the window, forcibly exiting the app. This leaves objects such as the system tray icon left behind. Additionally, changes introduced in #12183 - caused the braille viewer to be closed without saving state properly - lost code that destroyed the system tray and menu in some instances - made most of gui.terminate no longer necessary/redundant Description of how this pull request fixes the issue: - A windows event winUser.WM_EXIT_NVDA is registered that triggers safeAppExit and can be called across instances of NVDA. - move the safe destruction of the brailleviewer to safeAppExit so that it is exited properly before destruction - reintroduce the destruction of the system tray icon and menu, and remove the icon manually. - ensured safeAppExit is not called from gui.terminate if it has been called elsewhere to terminate the app. WM_QUIT is the other way to exit the MainLoop other than safeAppExit - removed restarting the MainLoop in gui.terminate to process pending events as this doesn't work. Known issues with pull request: - When starting a new instance of NVDA with an existing instance running, where one is version <2020.4, NVDA will not exit safely. Instead, the running NVDA copy will terminate directly using the behaviour of 2020.4. This is because WM_EXIT_NVDA won't be registered on the older instance. - Issues with terminating NVDA across instances cannot be logged properly as the loghandler hasn't been initialized

Summary of the issue: Changes introduced in #12183 - caused the braille viewer to be closed without saving state properly - lost code that destroyed the system tray and menu in some instances - made most of gui.terminate no longer necessary/redundant Description of how this pull request fixes the issue: - Creates `core.triggerNVDAExit` which terminates necessary modules safely and then closes all windows - Destroys the system tray icon and menu - Uses a parser error message if a new NVDA instance fails to end a running instance. - Uses an enum for ChangeWindowMessageFilter filters. Known issues with pull request: WM_QUIT will not exit the app safely (called from a new NVDA instance) when a dialog such as WelcomeDialog is still open

seanbudd added this to the 2021.1 milestone Mar 16, 2021

seanbudd self-assigned this Mar 16, 2021

This comment has been minimized.

Sign in to view

seanbudd force-pushed the fix-12153 branch 2 times, most recently from 2c8d29c to 79c989b Compare March 16, 2021 07:07

This comment has been minimized.

Sign in to view

seanbudd added 4 commits March 18, 2021 13:06

push crash dump as appveyor artifact

1ccdf8b

create safeAppExit

c605479

remove unnecessary Destroy call

d57d486

check if crashDump exists before pushing

bf9447e

seanbudd force-pushed the fix-12153 branch from d71c5e8 to bf9447e Compare March 18, 2021 02:22

seanbudd changed the title ~~WIP - Exit properly on Appveyor~~ Exit properly on silent install Mar 18, 2021

seanbudd added 2 commits March 18, 2021 14:22

revert d57d486

49eb9e7

improve handling of welcomedialog closing

684b9de

seanbudd force-pushed the fix-12153 branch from 60dda98 to 684b9de Compare March 18, 2021 03:30

seanbudd added 2 commits March 18, 2021 14:35

revert to previous behaviour to trigger appveyor crash dump

80e41ed

Revert "revert to previous behaviour to trigger appveyor crash dump"

0e628c4

This reverts commit 80e41ed.

This comment has been minimized.

Sign in to view

seanbudd marked this pull request as ready for review March 18, 2021 04:15

seanbudd added 3 commits March 18, 2021 15:44

add appveyor fail when crash dump on install process

cbf39f6

revert to previous behaviour to trigger appveyor crash dump

a469c4d

Revert "revert to previous behaviour to trigger appveyor crash dump"

79aea67

This reverts commit a469c4d.

This comment has been minimized.

Sign in to view

seanbudd requested review from feerrenrut and michaelDCurran March 18, 2021 05:04

move mainFrame exiting service to mainFrame, better logging

8f577c2

seanbudd marked this pull request as draft March 23, 2021 01:16

destroy the mainFrame to exit the App

d7aea3b

fix lint

439d7c0

seanbudd marked this pull request as ready for review March 23, 2021 03:01

seanbudd added 3 commits March 24, 2021 12:06

Merge branch 'master' into fix-12153

5475681

minimize diff

1fc0981

handle all top level window desctruction from safe app exit

c915591

use close behaviour where possible

9eb32c7

feerrenrut reviewed Mar 24, 2021

View reviewed changes

Comment thread source/gui/__init__.py

feerrenrut reviewed Mar 24, 2021

View reviewed changes

Comment thread source/gui/__init__.py

improved logging

f5eba29

seanbudd linked an issue Mar 25, 2021 that may be closed by this pull request

Can't quit NVDA while modal dialog is open #1740

Closed

seanbudd changed the title ~~Exit properly on silent install~~ Exit NVDA safely by closing all top level windows Mar 25, 2021

seanbudd mentioned this pull request Mar 25, 2021

Can't quit NVDA while modal dialog is open #1740

Closed

feerrenrut approved these changes Mar 25, 2021

View reviewed changes

michaelDCurran approved these changes Mar 25, 2021

View reviewed changes

seanbudd added 2 commits March 25, 2021 18:40

update changes t2t

7786252

Merge remote-tracking branch 'origin/master' into fix-12153

1564328

seanbudd merged commit 8665526 into master Mar 25, 2021

seanbudd deleted the fix-12153 branch March 25, 2021 08:05

seanbudd mentioned this pull request Mar 29, 2021

revert changes to the destruction of systray from the mainframe #12243

Closed

7 tasks

seanbudd mentioned this pull request Apr 12, 2021

refactor the exit of nvda and gui.terminate #12286

Merged

7 tasks

seanbudd mentioned this pull request Apr 27, 2021

refactor the exit of nvda and gui.terminate #12342

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Exit NVDA safely by closing all top level windows#12183

Exit NVDA safely by closing all top level windows#12183
seanbudd merged 29 commits into
masterfrom
fix-12153

seanbudd commented Mar 16, 2021 •

edited

Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

AppVeyorBot commented Mar 23, 2021

Uh oh!

feerrenrut commented Mar 23, 2021

Uh oh!

seanbudd commented Mar 24, 2021

Uh oh!

Uh oh!

Uh oh!

feerrenrut commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

seanbudd commented Mar 16, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

AppVeyorBot commented Mar 23, 2021

Uh oh!

feerrenrut commented Mar 23, 2021

Uh oh!

seanbudd commented Mar 24, 2021

Uh oh!

Uh oh!

Uh oh!

feerrenrut commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seanbudd commented Mar 16, 2021 •

edited

Loading