Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cyclic import context help #11958

Merged
merged 4 commits into from
Dec 22, 2020
Merged

Fix cyclic import context help #11958

merged 4 commits into from
Dec 22, 2020

Conversation

feerrenrut
Copy link
Contributor

Link to issue number:

#11950

Summary of the issue:

When installing add-ons which imported gui in their InstallTasks module NVDA failed to restart properly.

This was caused by the fact that when gui was imported it also imported contextHelp.ContextHelpMixin and contextHelp in turn also imports gui causing circular import.

Description of how this pull request fixes the issue:

Alternative fix to #11951
Breaks the circular import in contextHelp by making imports of gui, ui, and queueHandler dynamic imports.

This exposes another problem, the order of imports mattered. Several modules imported by gui, depended on ContextHelpMixin (imported name) in the gui module. This is fixed by referring directly to gui.contextHelp.ContextHelpMixin rather than gui.ContextHelpMixin in all those files. To prevent this from happening again (or being used by add-ons) it has been imported as _ContextHelpMixin.

Testing performed:

Tested as per #11950

Known issues with pull request:

Dependencies and layout of code in gui package needs attention. This would be an API breaking change, as such it can happen in 2021.1

Change log entry:

None

Alternative to #11951
@AppVeyorBot

This comment has been minimized.

lukaszgo1
lukaszgo1 previously approved these changes Dec 21, 2020
Copy link
Contributor

@lukaszgo1 lukaszgo1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from failing Linter checks and some cosmetic changes which I've requested below this looks fine to me and resolves this issue on my side. Nice work!

source/NVDAObjects/window/excel.py Show resolved Hide resolved
source/cursorManager.py Show resolved Hide resolved
source/gui/__init__.py Outdated Show resolved Hide resolved
source/updateCheck.py Show resolved Hide resolved
@feerrenrut
Copy link
Contributor Author

I don't think these imports should be considered redundant. The gui package now no longer explicitly exposes that name, if no other module imported ContextHelp it wouldn't be found in the gui package. Depending on this is depending on an implementation detail. Instead I think it is better to be explicit.

For more (and better) descriptions of this see:

@AppVeyorBot

This comment has been minimized.

One unused, should refer to full import path. Another missing import.
@feerrenrut feerrenrut merged commit 6e161eb into beta Dec 22, 2020
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Dec 22, 2020
@feerrenrut feerrenrut deleted the fixCyclicImport-contextHelp branch December 22, 2020 06:02
@feerrenrut feerrenrut modified the milestones: 2021.1, 2020.4 Dec 22, 2020
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.

4 participants