Fix cyclic import context help#11958
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
lukaszgo1
previously approved these changes
Dec 21, 2020
lukaszgo1
left a comment
Contributor
There was a problem hiding this comment.
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!
Contributor
Author
|
I don't think these imports should be considered redundant. The For more (and better) descriptions of this see: |
This comment has been minimized.
This comment has been minimized.
One unused, should refer to full import path. Another missing import.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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