Skip to content

Decouple contextHelp from the GUI module to avoid circular imports when installing add--ons#11951

Closed
lukaszgo1 wants to merge 6 commits into
nvaccess:betafrom
lukaszgo1:I11950
Closed

Decouple contextHelp from the GUI module to avoid circular imports when installing add--ons#11951
lukaszgo1 wants to merge 6 commits into
nvaccess:betafrom
lukaszgo1:I11950

Conversation

@lukaszgo1

@lukaszgo1 lukaszgo1 commented Dec 19, 2020

Copy link
Copy Markdown
Contributor

This is opened against beta, since this PR fixes regression introduced during 2020.4 dev cycle

Link to issue number:

Fixes #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:

I've decoupled gui from the ContextHelp. Specific changes:

  • All places which were previously using gui.ContextHelpMixin are now using it from gui.contextHelp.ContextHelpMixin and main gui module no longer imports ContextHelpMixin at all. This necessitated moving UsageStatsDialog,, LauncherDialog, and WelcomeDialog to a separate file in the gui package.
  • gui.contextHelp no longer needs to import gui to do that it has been necessary to move getDocFilePath to a separate documentationUtils module - to preserve backwards compatibility it is imported into the main gui module.

Testing performed:

  • Tried installing problematic add-on package from Crash on startup when installing add-ons importing gui at installTasks module level #11950 - installation succeeded both when installing it to a clean config and when upgrading with it - without this fix NVDA failed to start when upgrading.
  • Ensured that WelcomeDialog and LauncherDialog still works correctly.
  • Ensured that context help can still be invoked with f1 in NVDA's settings dialog
  • Ensured that user guide can be opened from the help menu.

Known issues with pull request:

Since this is a large change it would be great to merge it into beta as soon as possible to give it plenty of testing.

Change log entry:

None needed - this regression is not in a release yet.

@lukaszgo1 lukaszgo1 mentioned this pull request Dec 19, 2020
Closed
@lukaszgo1

lukaszgo1 commented Dec 19, 2020

Copy link
Copy Markdown
Contributor Author

I've no idea why AppVeyor fails to report status for this one - when looking at it on AppVeyor's website build is successful.

@AppVeyorBot

AppVeyorBot commented Dec 19, 2020

Copy link
Copy Markdown

See test results for failed build of commit b0b47f77bb

@lukaszgo1

lukaszgo1 commented Dec 19, 2020

Copy link
Copy Markdown
Contributor Author

cc @michaelDCurran @feerrenrut

feerrenrut
feerrenrut suggested changes Dec 21, 2020
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be good to move the classes out of gui/__init__ however, is it necessary for this PR? It looks like the import cycle is broken by moving getDocFilePath out of gui, and dynamically importing it in contextHelp (no longer importing gui)

Moving the classes (and not importing them back to expose them as gui.X) is technically an API break. This isn't a problem for getDocFilePath because it is imported back into Gui. That said, it probably isn't necessary to move that either?

Comment thread source/gui/__init__.py
import os
import sys
import threading
import codecs

@feerrenrut feerrenrut Dec 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may be unused here, but it is possible some add-on may depend on it. It should be marked as 'noqa' with the appropriate code to disable error from flake 8 for unused includes. A comment should also be added that this is deprecated and will be removed in 2021.1

@lukaszgo1

lukaszgo1 commented Dec 21, 2020

Copy link
Copy Markdown
Contributor Author

@feerrenrut wrote:

I think it would be good to move the classes out of gui/__init__ however, is it necessary for this PR? It looks like the import cycle is broken by moving getDocFilePath out of gui, and dynamically importing it in contextHelp (no longer importing gui)

It unfortunately is since contextHelp imports ui which in turns imports gui again causing circular imports.

Moving the classes (and not importing them back to expose them as gui.X) is technically an API break.

I don't think any add-on should modify these classes. If you're really worried about This I can define a module level __getattr__ which would lazily import them if needed.

feerrenrut added a commit that referenced this pull request Dec 21, 2020
@feerrenrut
fixes #11950
59830e7 
Alternative to #11951
@feerrenrut feerrenrut mentioned this pull request Dec 21, 2020
Merged
@feerrenrut

feerrenrut commented Dec 21, 2020

Copy link
Copy Markdown
Contributor

I have created an alternative approach which I think resolves the issue. Would you please take a look? #11958

@feerrenrut

feerrenrut commented Dec 22, 2020

Copy link
Copy Markdown
Contributor

Since #11958 has been merged I will close this. As I said before, I'd be happy for the various gui classes to be reorganised in the 2021.1 release.

@feerrenrut feerrenrut closed this Dec 22, 2020
@lukaszgo1 lukaszgo1 mentioned this pull request Feb 26, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lukaszgo1 @AppVeyorBot @feerrenrut