refactor the exit of nvda and gui.terminate

[seanbudd]

Link to issue number:

Closes #12238

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.

Testing strategy:

• Ensure NVDA system tray icons are cleaned on any exit.
• Ensure NVDA exits properly on restart and when starting the process subsequently. Check nvda-old.log when restarting NVDA.
• Ensure NVDA can exit and restart correctly without errors or crashes. Existing system tests exist for this process.
• Running 2020.4 (leave it running) and start this build. This build should cause 2020.4 to exit, and this build should replace it as the running version. Confirm by checking about -> version
• Running this build (leave it running) and start 2020.4 which should cause this build to exit, and 2020.4 should replace it as the running version. Confirm by checking about -> version

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

Change log entry:

Developer changes

-  do not exit NVDA by sending a `WM_QUIT` message to the process. Instead send `winUser.WM_EXIT_NVDA` to the window handle (found by `winUser.FindWindow('wxWindowClassNR', 'NVDA')`)

Code Review Checklist:

• Pull Request description is up to date.
• Unit tests.
• System (end to end) tests.
• Manual tests.
• User Documentation.
• Change log entry.
• Context sensitive help for GUI changes.

[AppVeyorBot]

• PR introduces Flake8 errors
• Build (for testing PR)

See test results for failed build of commit 91d0012f0d

[AppVeyorBot]

• System test failure
• Build (for testing PR)

See test results for failed build of commit 074e5b2a59

[feerrenrut]

Can you also test:

• Running 2020.4 (leave it running) and start this build. This build should cause 2020.4 to exit, and this build should replace it as the running version. Confirm by checking about -> version
• Running this build (leave it running) and start 2020.4 which should cause this build to exit, and 2020.4 should replace it as the running version. Confirm by checking about -> version

[seanbudd]

@feerrenrut - these test cases work (as in the correct version of NVDA runs) but NVDA won't exit safely, I've noted this in the Known issues with this PR section.

[seanbudd]

will this backwards compatibility for upgrading/downgrading need to stay forever? I've marked it for 2022.1 deprecation but it would still be an issue when upgrading from 2020.4

[feerrenrut]

but NVDA won't exit safely, I've noted this in the issues with this PR section.

That's fine. I don't think there is any way around that.

will this backwards compatibility for upgrading/downgrading need to stay forever?

I think it should stay for a long time. I wouldn't remove it for 2022.1.
Going forward, newer version (eg 2021.1 and later) won't have the limitation from the "known issues" section, right? If so, please clarify.

[seanbudd]

I think it should stay for a long time. I wouldn't remove it for 2022.1.

How should I mark the code? Or should the deprecation notes be removed entirely?

Going forward, newer version (eg 2021.1 and later) won't have the limitation from the "known issues" section, right? If so, please clarify.

Correct, how does the new message look?

[michaelDCurran]

Is quitting a newer copy of NVDA by version <= 2020.4 (with WM_QUIT) any worse now than it used to be? Granted it probably never entirely worked, but has the movement of code now caused less things to be cleaned up?
It should be said though that I do agree with the changes and like the fact we have moved away from WM_QUIT.

[AppVeyorBot]

• Unit test failure
• Build (for testing PR)

See test results for failed build of commit 982119d5ed

[seanbudd]

Is quitting a newer copy of NVDA by version <= 2020.4 (with WM_QUIT) any worse now than it used to be? Granted it probably never entirely worked, but has the movement of code now caused less things to be cleaned up?
It should be said though that I do agree with the changes and like the fact we have moved away from WM_QUIT.

No, as WM_QUIT causes the MainLoop to exit without calling safeAppExit. I've added some code to call safeAppExit in gui.terminate if it hasn't been called yet 9f561ca .

[AppVeyorBot]

• System test failure
• Build (for testing PR)

See test results for failed build of commit 89fe6664e5

[AppVeyorBot]

• PR introduces Flake8 errors
• Build (for testing PR)

See test results for failed build of commit 89fe6664e5

Added significant changes

[michaelDCurran]

In safeAppExit: I think we should replace all x.Destroy() and wx.CallAfter(x.Destroy) calls with app.ScheduleForDestruction(x).
app being gotten from wx.GetApp()
ScheduleForDestruction, given any wx object (so I'm assuming windows, menus, icons etc), will schedule that object for destruction in the next event loop cycle, or if not in the event loop (UsesEventLoop() is False such as after exiting the MainLoop) then ScheduleForDestruction will destroy the objects in-place.
So, this should handle SafeAppExit being called either inside the event loop such as for a clean exit with wm_exit_nvda, or outside of the event loop, such as with wm_quit.
I have not tested this though, just going by the docs and WXWidgets source code. But I specifically looked into this as we have calls to wx.CallAfter in safeAppExit, but it may be called outside of the event loop in gui.terminate.

[seanbudd]

@michaelDCurran - I've incorporated that change and retested. I noticed that the WM_QUIT signal was not being sent properly (based off my misunderstanding of how WM_QUIT worked from earlier). This should be fixed now.

[michaelDCurran]

The print calls in nvda.pyw (which happen when upgrading from an older snapshot build, or any older release) are causing a wx error dialog to appear in binary builds. This is confusing, but is also seeming to freeze up the Windows shell, or at very least, NVDA is struggling to read much after this occurs.
Purely for the error dialog with the print calls alone, I think we must revert this pr for now and investigate further.

[lukaszgo1]

I'm getting a lot of crashes when restarting binary versions of NVDA after this PR was merged with either [WinError 1400] Invalid window handle or [WinError 5] Access denied

[seanbudd]

1. Revert PR
2. Reopen this PR and:
   • Add add-on handler clean up to safeAppExit
   • Remove print statements
   • Change the exit process by renaming window with the version number, check if that is there to decide whether to send WM_QUIT or WM_EXIT_NVDA
   • test the binaries