When installing add-ons in add-on store make sure that restart dialog is shown only when the install is done #15852
Conversation
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
… is shown only when the install is done
|
cc @beqabeqa473 You may want to give this code a try. |
5 tasks
seanbudd
pushed a commit
that referenced
this pull request
Dec 27, 2023
…tween add-ons (#15967) Fixes regression from PR #14481. Discovered due to intermittent failures occurring when installing add-ons, when working on #15852 and #15937 Summary of the issue: When installing and uninstalling add-ons bundling install tasks, installation either fails or behaves unexpectedly in the following scenarios: 1: User installs an add-on User removes it and restarts NVDA User tries to install the same add-on In this scenario when installing the install tasks included in the version of add-on which was just removed are used, which is unexpected. Note that this worked well before #14481, not sure why pkgutil.ImpImporter behaves better (I haven't investigated, as it is deprecated). 2: User installs an add-on, which imports its sub module in install tasks using loadModule and also imports a sub module in the same way inside global plugin / app module during normal operation User upgrades the add-on to a different version In this scenario after upgrade the add-on module from the uninstalled version is used during normal operation of NVDA. 3: User installs an add-on which imports one of its modules in install tasks by manipulating sys.path, for real life examples see Instant Translate and Mouse Wheel Scrolling User uninstalls this add-on, restarts NVDA, and tries to install the new version In this scenario installation fails since the imported module from the old version is used. A slight variation of this scenario is to try to install both Instant Translate and Mouse Wheel Scrolling the add-on installed last would fail to be installed, since they both bundle module named donate_dialog but with different functions Description of user facing changes Add-ons are successfully installed in all scenarios described above. Description of development approach All these problems exist because when importing modules in Python if module with the same name exists in sys.modules it is just used without importing it again. To ensure modules are always re-imported when needed, the following has been done: When importing modules we no longer use add-on name to create a name space package in which the module would be imported, the last segment of the add-on path is used instead. This ensures that the package for add-on which is installed would just be its name, and for add-on which is pending install it would end with the pending install suffix. This fixes issue 1. loadModule memorizes all modules it imported for a given add-on. After installation / removal is done they're removed from sys.modules. This fixes issue 2. To remove cached modules imported with the standard import command, we store names of all modules imported before installation / removal, perform it, create list of newly imported modules and remove all newly added modules which belong to the add-on from the cache. This fixes issue 3.
Nael-Sayegh
pushed a commit
to Nael-Sayegh/nvda
that referenced
this pull request
Feb 15, 2024
…tween add-ons (nvaccess#15967) Fixes regression from PR nvaccess#14481. Discovered due to intermittent failures occurring when installing add-ons, when working on nvaccess#15852 and nvaccess#15937 Summary of the issue: When installing and uninstalling add-ons bundling install tasks, installation either fails or behaves unexpectedly in the following scenarios: 1: User installs an add-on User removes it and restarts NVDA User tries to install the same add-on In this scenario when installing the install tasks included in the version of add-on which was just removed are used, which is unexpected. Note that this worked well before nvaccess#14481, not sure why pkgutil.ImpImporter behaves better (I haven't investigated, as it is deprecated). 2: User installs an add-on, which imports its sub module in install tasks using loadModule and also imports a sub module in the same way inside global plugin / app module during normal operation User upgrades the add-on to a different version In this scenario after upgrade the add-on module from the uninstalled version is used during normal operation of NVDA. 3: User installs an add-on which imports one of its modules in install tasks by manipulating sys.path, for real life examples see Instant Translate and Mouse Wheel Scrolling User uninstalls this add-on, restarts NVDA, and tries to install the new version In this scenario installation fails since the imported module from the old version is used. A slight variation of this scenario is to try to install both Instant Translate and Mouse Wheel Scrolling the add-on installed last would fail to be installed, since they both bundle module named donate_dialog but with different functions Description of user facing changes Add-ons are successfully installed in all scenarios described above. Description of development approach All these problems exist because when importing modules in Python if module with the same name exists in sys.modules it is just used without importing it again. To ensure modules are always re-imported when needed, the following has been done: When importing modules we no longer use add-on name to create a name space package in which the module would be imported, the last segment of the add-on path is used instead. This ensures that the package for add-on which is installed would just be its name, and for add-on which is pending install it would end with the pending install suffix. This fixes issue 1. loadModule memorizes all modules it imported for a given add-on. After installation / removal is done they're removed from sys.modules. This fixes issue 2. To remove cached modules imported with the standard import command, we store names of all modules imported before installation / removal, perform it, create list of newly imported modules and remove all newly added modules which belong to the add-on from the cache. This fixes issue 3.
SaschaCowley
pushed a commit
to SaschaCowley/nvda
that referenced
this pull request
Feb 27, 2024
…tween add-ons (nvaccess#15967) Fixes regression from PR nvaccess#14481. Discovered due to intermittent failures occurring when installing add-ons, when working on nvaccess#15852 and nvaccess#15937 Summary of the issue: When installing and uninstalling add-ons bundling install tasks, installation either fails or behaves unexpectedly in the following scenarios: 1: User installs an add-on User removes it and restarts NVDA User tries to install the same add-on In this scenario when installing the install tasks included in the version of add-on which was just removed are used, which is unexpected. Note that this worked well before nvaccess#14481, not sure why pkgutil.ImpImporter behaves better (I haven't investigated, as it is deprecated). 2: User installs an add-on, which imports its sub module in install tasks using loadModule and also imports a sub module in the same way inside global plugin / app module during normal operation User upgrades the add-on to a different version In this scenario after upgrade the add-on module from the uninstalled version is used during normal operation of NVDA. 3: User installs an add-on which imports one of its modules in install tasks by manipulating sys.path, for real life examples see Instant Translate and Mouse Wheel Scrolling User uninstalls this add-on, restarts NVDA, and tries to install the new version In this scenario installation fails since the imported module from the old version is used. A slight variation of this scenario is to try to install both Instant Translate and Mouse Wheel Scrolling the add-on installed last would fail to be installed, since they both bundle module named donate_dialog but with different functions Description of user facing changes Add-ons are successfully installed in all scenarios described above. Description of development approach All these problems exist because when importing modules in Python if module with the same name exists in sys.modules it is just used without importing it again. To ensure modules are always re-imported when needed, the following has been done: When importing modules we no longer use add-on name to create a name space package in which the module would be imported, the last segment of the add-on path is used instead. This ensures that the package for add-on which is installed would just be its name, and for add-on which is pending install it would end with the pending install suffix. This fixes issue 1. loadModule memorizes all modules it imported for a given add-on. After installation / removal is done they're removed from sys.modules. This fixes issue 2. To remove cached modules imported with the standard import command, we store names of all modules imported before installation / removal, perform it, create list of newly imported modules and remove all newly added modules which belong to the add-on from the cache. This fixes issue 3.
Adriani90
pushed a commit
to Adriani90/nvda
that referenced
this pull request
Mar 13, 2024
…tween add-ons (nvaccess#15967) Fixes regression from PR nvaccess#14481. Discovered due to intermittent failures occurring when installing add-ons, when working on nvaccess#15852 and nvaccess#15937 Summary of the issue: When installing and uninstalling add-ons bundling install tasks, installation either fails or behaves unexpectedly in the following scenarios: 1: User installs an add-on User removes it and restarts NVDA User tries to install the same add-on In this scenario when installing the install tasks included in the version of add-on which was just removed are used, which is unexpected. Note that this worked well before nvaccess#14481, not sure why pkgutil.ImpImporter behaves better (I haven't investigated, as it is deprecated). 2: User installs an add-on, which imports its sub module in install tasks using loadModule and also imports a sub module in the same way inside global plugin / app module during normal operation User upgrades the add-on to a different version In this scenario after upgrade the add-on module from the uninstalled version is used during normal operation of NVDA. 3: User installs an add-on which imports one of its modules in install tasks by manipulating sys.path, for real life examples see Instant Translate and Mouse Wheel Scrolling User uninstalls this add-on, restarts NVDA, and tries to install the new version In this scenario installation fails since the imported module from the old version is used. A slight variation of this scenario is to try to install both Instant Translate and Mouse Wheel Scrolling the add-on installed last would fail to be installed, since they both bundle module named donate_dialog but with different functions Description of user facing changes Add-ons are successfully installed in all scenarios described above. Description of development approach All these problems exist because when importing modules in Python if module with the same name exists in sys.modules it is just used without importing it again. To ensure modules are always re-imported when needed, the following has been done: When importing modules we no longer use add-on name to create a name space package in which the module would be imported, the last segment of the add-on path is used instead. This ensures that the package for add-on which is installed would just be its name, and for add-on which is pending install it would end with the pending install suffix. This fixes issue 1. loadModule memorizes all modules it imported for a given add-on. After installation / removal is done they're removed from sys.modules. This fixes issue 2. To remove cached modules imported with the standard import command, we store names of all modules imported before installation / removal, perform it, create list of newly imported modules and remove all newly added modules which belong to the add-on from the cache. This fixes issue 3.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
Fixes #15613
Summary of the issue:
When installing add-ons developers may want to present a GUI prompt as part of the install tasks. When installing from the external bundle this prompt is blocking, in other words user has to answer it before installation is complete and NVDA asks to be restarted. In comparison installations from the add-on store shows the prompt, but then the restart prompt was shown, making it impossible to interact with the add-ons installation GUI.
Description of user facing changes
When installing add-ons from the store the restart message is shown only when all installations are done, giving user a chance to interact with eventual GUI prompts they raise.
Description of development approach
Similar to the code path used when installing external bundles, call to the
addonHandler.installAddonBundleis executed in a separate, blocking threat usingExecAndPump.ExecAndPumpwas also moved fromguitosystemUtils, as it has no relation to GUI, other than the fact that it was used only in this package.Testing strategy:
Known issues with pull request:
ExecAndPumpshould be placed, I'm totally happy to move itaddonHandler.installAddonBundleinExecAndPump, so that this does not need to be done two times. The problem with this approach is that add-on authors may calladdonHandler.installAddonBundleusingExecAndPumpthemselves (add-on updater certainly does), and this may cause hard to diagnose bugs.Code Review Checklist: