Skip to content

Pull out wx to top level import, improve typing for core#13701

Closed
seanbudd wants to merge 4 commits into
masterfrom
warnIfCallLaterBeforeReady
Closed

Pull out wx to top level import, improve typing for core#13701
seanbudd wants to merge 4 commits into
masterfrom
warnIfCallLaterBeforeReady

Conversation

@seanbudd
Copy link
Copy Markdown
Member

@seanbudd seanbudd commented May 17, 2022

Link to issue number:

Follow up of #13699

Summary of the issue:

NVDA core is dependent on wx in many implicit cases.

Description of how this pull request fixes the issue:

Make the wx dependency explicit.
Add additional linting and typing for work done in #13699.

Future work is needed to make core no longer dependent on wx.

Testing strategy:

Test starting NVDA with a built launcher and running from source

Known issues with pull request:

Change log entries:

No user facing changes

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd seanbudd requested a review from a team as a code owner May 17, 2022 00:14
@Brian1Gaff

This comment was marked as off-topic.

@seanbudd seanbudd closed this May 18, 2022
@seanbudd seanbudd reopened this May 18, 2022
@seanbudd seanbudd marked this pull request as draft May 18, 2022 01:13
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 1bff20152c

@seanbudd seanbudd changed the title Prevent callLater when NVDA not initialized Pull out wx to top level import, improve typing for core May 19, 2022
@seanbudd seanbudd marked this pull request as ready for review May 19, 2022 03:27
@lukaszgo1
Copy link
Copy Markdown
Contributor

How you've determined that not importing wxlazily is safe?

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented May 19, 2022

How you've determined that not importing wxlazily is safe?

I've run the launcher and run from source. I'd be incredibly surprised if it wasn't safe. Our main reason for deferred imports is circular dependencies.
This is an external dependency, and as such shouldn't execute code that depends on NVDA when being imported.

@seanbudd
Copy link
Copy Markdown
Member Author

Another reason for deferred imports is one dependency may need another dependency initialized first. wx doesn't rely on any NVDA modules to be initialized first.

@feerrenrut
Copy link
Copy Markdown
Contributor

Another thing to consider here, where some other module needs to import core before wx is imported. A reason for this may be the time it takes to import wx.

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented May 20, 2022

running from source, with the following code at the start of core.py

"""NVDA core"""
import time
s = time.time()
import wx
print(time.time()-s)

Time spent importing:
0.037993 (seconds)

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented May 20, 2022

This is an increase to the sum of the other imports: 0.00799

@seanbudd seanbudd marked this pull request as draft May 23, 2022 00:38
@LeonarddeR
Copy link
Copy Markdown
Collaborator

Will it also be considered to improve the code style of core, i.e. by taking the class definitions of App, CorePump and MessageWindow out of the main function?

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented Jun 10, 2022

Another thing to consider here, where some other module needs to import core before wx is imported. A reason for this may be the time it takes to import wx.

I believe imports are cached, and wx must be imported in core.main, before NVDA starts.

For the python console, this is already imported in the namespace: https://www.nvaccess.org/files/nvda/documentation/developerGuide.html#toc47

@seanbudd seanbudd removed the request for review from michaelDCurran June 10, 2022 08:57
@seanbudd seanbudd marked this pull request as ready for review June 10, 2022 08:57
@michaelDCurran
Copy link
Copy Markdown
Member

michaelDCurran commented Jun 10, 2022 via email

@XLTechie

This comment was marked as resolved.

@feerrenrut
Copy link
Copy Markdown
Contributor

I believe imports are cached, and wx must be imported in core.main, before NVDA starts.

When NVDA starts, multiple NVDA processes are triggered (due to the way we use Ease of Access), this is unrelated, probably unwanted behavior. However, in those cases, NVDA quite quickly determines that another NVDA process is already running, then exits.

Moving the import of wx may impact this.

For the python console, this is already imported in the namespace:

Sorry, I don't follow this.

@seanbudd
Copy link
Copy Markdown
Member Author

Another approach can be started to add typing to core, without moving the wx import.

@seanbudd seanbudd closed this Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants