refactor the exit of nvda and gui.terminate by seanbudd · Pull Request #12286 · nvaccess/nvda

seanbudd · 2021-04-12T00:23:56Z

Link to issue number:

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:

AppVeyorBot · 2021-04-12T00:34:42Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 91d0012f0d

AppVeyorBot · 2021-04-13T07:56:54Z

System test failure
Build (for testing PR)

See test results for failed build of commit 074e5b2a59

feerrenrut · 2021-04-13T09:21:34Z

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 · 2021-04-14T01:30:52Z

@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 · 2021-04-14T01:41:27Z

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 · 2021-04-14T03:37:32Z

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 · 2021-04-14T04:02:58Z

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 · 2021-04-21T01:48:59Z

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.

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

…cleanup-exit

AppVeyorBot · 2021-04-21T04:20:00Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit 982119d5ed

seanbudd · 2021-04-21T05:03:59Z

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 · 2021-04-21T05:15:12Z

System test failure
Build (for testing PR)

See test results for failed build of commit 89fe6664e5

AppVeyorBot · 2021-04-21T05:52:47Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 89fe6664e5

Added significant changes

michaelDCurran · 2021-04-21T06:43:17Z

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 · 2021-04-22T03:14:56Z

@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 · 2021-04-22T21:45:14Z

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 · 2021-04-22T21:59:02Z

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

This reverts commit 60c0b76.

seanbudd · 2021-04-23T02:34:30Z

Revert PR
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

This reverts commit 60c0b76.

refactor the exit of nvda

244ef55

fix lint

81b07c6

seanbudd added this to the 2021.1 milestone Apr 12, 2021

seanbudd requested a review from feerrenrut April 12, 2021 01:23

seanbudd added 2 commits April 13, 2021 11:22

remove systray icon before destroying

61cc63d

safely exit NVDA across instances

299bb81

seanbudd self-assigned this Apr 13, 2021

fix log strings

c34b8b3

seanbudd added the deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release label Apr 13, 2021

revert removal of register of WM_QUIT

6af0a88

seanbudd mentioned this pull request Apr 15, 2021

Improve logging of test for panel destruction #12291

Merged

7 tasks

feerrenrut reviewed Apr 16, 2021

View reviewed changes

Comment thread source/core.py

Comment thread source/gui/__init__.py Outdated

Comment thread source/nvda.pyw Outdated

Comment thread source/nvda.pyw Outdated

Comment thread source/winUser.py Outdated

seanbudd added 2 commits April 20, 2021 09:52

Merge branch 'master' into cleanup-exit

b24e0e6

incorporate review feedback

6a23a0a

seanbudd removed the deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release label Apr 20, 2021

feerrenrut previously approved these changes Apr 21, 2021

View reviewed changes

Comment thread source/winUser.py Outdated

Use enum for constants

3ccf9c5

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

seanbudd requested a review from a team as a code owner April 21, 2021 04:05

seanbudd added 2 commits April 21, 2021 14:05

Merge branch 'master' into cleanup-exit

390bf7c

Merge branch 'cleanup-exit' of https://github.com/nvaccess/nvda into …

d6afbdb

…cleanup-exit

use enum

c894163

call safeAppExit if it is not called in terminate

9f561ca

seanbudd added 4 commits April 22, 2021 12:18

Merge branch 'master' into cleanup-exit

7e12aa5

remove duplicated WM exit codes

c2479d9

use ScheduleForDestruction

5d9ed77

ensure WM_QUIT is sent on WM_EXIT_NVDA failure

badc252

michaelDCurran approved these changes Apr 22, 2021

View reviewed changes

update changes

fdf7a5e

seanbudd merged commit 60c0b76 into master Apr 22, 2021

seanbudd deleted the cleanup-exit branch April 22, 2021 06:06

seanbudd mentioned this pull request Apr 22, 2021

NVDA leaves a phantom icon in the systray the first time it's restarted after a system boot #11553

Open

lukaszgo1 mentioned this pull request Apr 22, 2021

gui.safeAppExit is called too early making it impossible for add-ons to cleanup their own GUI #12325

Closed

seanbudd added a commit that referenced this pull request Apr 22, 2021

Revert "refactor the exit of nvda and gui.terminate (#12286)"

b78a060

This reverts commit 60c0b76.

seanbudd mentioned this pull request Apr 22, 2021

Revert "refactor the exit of nvda and gui.terminate" #12326

Merged

michaelDCurran pushed a commit that referenced this pull request Apr 23, 2021

Revert "refactor the exit of nvda and gui.terminate (#12286)" (#12326)

d9afe35

This reverts commit 60c0b76.

seanbudd mentioned this pull request Apr 27, 2021

refactor the exit of nvda and gui.terminate #12342

Merged

7 tasks

Uh oh!

Conversation

seanbudd commented Apr 12, 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!

AppVeyorBot commented Apr 12, 2021

Uh oh!

AppVeyorBot commented Apr 13, 2021

Uh oh!

feerrenrut commented Apr 13, 2021

Uh oh!

seanbudd commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanbudd commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Apr 14, 2021

Uh oh!

seanbudd commented Apr 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelDCurran commented Apr 21, 2021

Uh oh!

Uh oh!

AppVeyorBot commented Apr 21, 2021

Uh oh!

seanbudd commented Apr 21, 2021

Uh oh!

AppVeyorBot commented Apr 21, 2021

Uh oh!

AppVeyorBot commented Apr 21, 2021

Uh oh!

michaelDCurran commented Apr 21, 2021

Uh oh!

seanbudd commented Apr 22, 2021

Uh oh!

michaelDCurran commented Apr 22, 2021

Uh oh!

lukaszgo1 commented Apr 22, 2021

Uh oh!

seanbudd commented Apr 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seanbudd commented Apr 12, 2021 •

edited

Loading

seanbudd commented Apr 14, 2021 •

edited

Loading

seanbudd commented Apr 14, 2021 •

edited

Loading