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

seanbudd · 2021-04-27T01:24:40Z

Link to issue number:

Changes

Summary of the issue:

PR #12286 Introduced the following issues:

Logging via prints caused 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.
Prevented the addon handler from properly closing addons
Added unnecessary wait time for exiting NVDA to check whether to send WM_QUIT after WM_EXIT_NVDA fails.

PR #12286 addressed:

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:

This PR:

Creates core.triggerNVDAExit which terminates necessary modules and then closes all windows
Uses a parser error message if an new NVDA instance fails to end a running instances.
Uses an enum for ChangeWindowMessageFilter filters.

PR #12286 addressed it's issues by:

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.
Test these with the build binary for this branch as well as source
- 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
- Running latest alpha (leave it running) and start this build. This build should cause latest alpha 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 latest alpha which should cause this build to exit, and latest alpha should replace it as the running version. Confirm by checking about -> version

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

Change log entry:

None

Code Review Checklist:

lukaszgo1

Aside from one comment about raising WinError below this looks pretty nice. I am no longer able to reproduce crashes I've been experiencing with the previous implementation.

LeonarddeR

I have two concerns with the current code:

It introduces logic to the winUser module. winUser historically was meant to wrap user32.dll functions, now there's NVDA specific stuff leaking into it. I'd like to suggest moving this to another module, windowUtils for example, though a separate module in the gui could also be considered.
Please avoid registering the window message at the top level of a module, which will always register the message when the module is imported. This means it will try to register the message when linting, for example.

seanbudd · 2021-04-29T06:53:14Z

I have two concerns with the current code:

It introduces logic to the winUser module. winUser historically was meant to wrap user32.dll functions, now there's NVDA specific stuff leaking into it. I'd like to suggest moving this to another module, windowUtils for example, though a separate module in the gui could also be considered.

Please avoid registering the window message at the top level of a module, which will always register the message when the module is imported. This means it will try to register the message when linting, for example.

We've addressed this in favour of reverting using this custom windows message, as it's not necessary to fix the bug and this otherwise blocks the release.

AppVeyorBot · 2021-04-29T07:30:26Z

System test failure
Build (for testing PR)

See test results for failed build of commit 6bf2942c42

feerrenrut

Thanks @seanbudd

feerrenrut · 2021-05-06T09:24:17Z

+		preNVDAExit.unregister(handler)
+
+
+def closeAllWindows():


It seems like this should never be called directly, only via preNVDAExit Action. If so, please add a note to the doc string.

I also think this function name should be prefixed with an underscore, as a convention to signify their "protected" state. Typically anything that we don't want add-ons to use should be prefixed with an underscore (or in a class that is named that way).

seanbudd requested a review from a team as a code owner April 27, 2021 01:24

seanbudd requested a review from michaelDCurran April 27, 2021 01:24

seanbudd changed the title ~~Cleanup exit~~ refactor the exit of nvda and gui.terminate Apr 27, 2021

seanbudd force-pushed the cleanup-exit branch from fb02dc7 to 2784f18 Compare April 27, 2021 01:31

seanbudd requested a review from feerrenrut April 27, 2021 01:35

seanbudd self-assigned this Apr 27, 2021

seanbudd added this to the 2021.1 milestone Apr 27, 2021

lukaszgo1 reviewed Apr 27, 2021

View reviewed changes

Comment thread source/winUser.py Outdated

LeonarddeR suggested changes Apr 28, 2021

View reviewed changes

LeonarddeR approved these changes Apr 29, 2021

View reviewed changes

Comment thread source/nvda.pyw Outdated

michaelDCurran reviewed Apr 29, 2021

View reviewed changes

Comment thread source/gui/__init__.py

Comment thread source/core.py

lukaszgo1 reviewed Apr 29, 2021

View reviewed changes

Comment thread source/gui/__init__.py Outdated

feerrenrut reviewed Apr 30, 2021

View reviewed changes

Comment thread source/nvda.pyw Outdated

seanbudd marked this pull request as draft April 30, 2021 05:15

seanbudd force-pushed the cleanup-exit branch from 59509c2 to b644baf Compare May 3, 2021 03:45

use an enum for windows message filters

26ac314

seanbudd force-pushed the cleanup-exit branch from b644baf to d31f5a8 Compare May 3, 2021 03:48

use an extension point to handle NVDA exit safely

a71a145

seanbudd force-pushed the cleanup-exit branch from d31f5a8 to a71a145 Compare May 3, 2021 03:51

seanbudd marked this pull request as ready for review May 3, 2021 03:54

michaelDCurran approved these changes May 3, 2021

View reviewed changes

seanbudd requested a review from feerrenrut May 3, 2021 23:14

feerrenrut approved these changes May 6, 2021

View reviewed changes

fix docstring and variable name

4e39b30

seanbudd merged commit 9dc1f36 into master May 7, 2021

seanbudd deleted the cleanup-exit branch May 7, 2021 02:31

seanbudd mentioned this pull request May 7, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor the exit of nvda and gui.terminate#12342

refactor the exit of nvda and gui.terminate#12342
seanbudd merged 3 commits into
masterfrom
cleanup-exit

seanbudd commented Apr 27, 2021 •

edited

Loading

Uh oh!

lukaszgo1 left a comment

Uh oh!

Uh oh!

LeonarddeR left a comment

Uh oh!

seanbudd commented Apr 29, 2021

Uh oh!

Uh oh!

AppVeyorBot commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feerrenrut left a comment

Uh oh!

feerrenrut May 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

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

Uh oh!

Link to issue number:

Changes

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!

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Apr 29, 2021

Uh oh!

Uh oh!

AppVeyorBot commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut May 6, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seanbudd commented Apr 27, 2021 •

edited

Loading