No longer remove failed add-ons installs and warn when add-on installs, removals and updates cannot complete by lukaszgo1 · Pull Request #15937 · nvaccess/nvda

lukaszgo1 · 2023-12-18T16:08:01Z

Link to issue number:

Reverts #15921
Closes #12823
Addresses the most likely cause of #15719
Hopefully fixes #15927

Summary of the issue:

PR #15921 started removing all add-ons which failed to be installed without showing warnings to the user. Users generally expect NVDA to attempt installation / removal of add-ons on the next restart if they failed, to have a warning when one of these operations didn't succeed and not to be able to install add-on which is not fully removed from Add-ons Store.

Description of user facing changes

When one or more add-ons fails to be updated, uninstalled or installed, NVDA shows an appropriate warning on startup. Installation / removal is attempted on the next restart. When add-on is pending remove it is shown in the Add-ons Store along with the 'pending remove' status. All add-ons which are no longer present on disk are removed from the list of pending installs to hopefully improve #15719.

Description of development approach

NVDA stores add-ons whose installation / removal failed in a list. All these add-ons are not loaded ,but shown in the Add-ons Store. These lists are used to determine what warnings should be shown to the user. List of pending installs is cleaned-up from add-ons which are no longer present on disk, excluding these which failed to be installed, as their install is going to be attempted on the next restart.

Testing strategy:

Installed an add-on from Add-ons Store, opened its folder to make sure it cannot be uninstalled, tried to uninstall it and verified that after the restart there is a warning, code from the add-on is not loaded and that it is shown in the store with an 'pending remove' status
Keeping folder of the installed add-on open tried to update it, verified that expected warnings are shown and that add-on' status in the store is 'pending installation'
Performed the above steps with an incompatible add-on, verified that when its folder is closed and therefore it can be installed, the fact that user overridden its compatibility is not lost
Added a non existent add-on to the pending install list, restarted NVDA and make sure that it was removed from the list and an appropriate message was logged

Known issues with pull request:

None known

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

This reverts commit 81e17be.

… no longer existing pending installation entries.

AppVeyorBot · 2023-12-18T16:50:40Z

See test results for failed build of commit 01bb1c5b70

lukaszgo1 · 2023-12-19T11:27:33Z

Thanks for your review @seanbudd @CyrilleB79 ! I believe all your points are now addressed.

CyrilleB79

LGTM globally. Many thanks for this!

I have made various failing tests for the 3 cases (keep folder open in Explorer, create a file with the same name as the folder to be created, etc.). The UX is as expected.

I have just added suggestions about GUI phrasing or variable names.

CyrilleB79 · 2023-12-19T12:58:22Z

+				"Some operations on add-ons failed. See the log file for more details.\n{}"
+			).format("\n".join(addonFailureMessages)),
+			# Translators: Title of message shown when requested action on add-ons failed.
+			_("Add-on failures"),


It's not the add-on is failing, it's its install/removal process.

A simpler title would be enough IMO.

Suggested change

_("Add-on failures"),

_("Error"),

CyrilleB79 · 2023-12-19T13:01:39Z

 _shuttingDownFlagLock = threading.Lock()


+def _showAddonsWarnings() -> None:


Since we are displaying an error message:

Suggested change

def _showAddonsWarnings() -> None:

def _showAddonsUpdateErrors() -> None:

or only:

Suggested change

def _showAddonsWarnings() -> None:

def _showAddonsErrors() -> None:

CyrilleB79 · 2023-12-19T13:25:38Z

+		addonFailureMessages.append(
+			ngettext(
+				# Translators: Shown when one or more add-ons failed to update.
+				"The following add-on failed to update: {}",


Add end of sentence period. It's better when reading the whole dialog (NVDA+B) and various such messages are concatenated.

Suggested change

"The following add-on failed to update: {}",

"The following add-on failed to update: {}.",

CyrilleB79 · 2023-12-19T13:25:58Z

+			ngettext(
+				# Translators: Shown when one or more add-ons failed to update.
+				"The following add-on failed to update: {}",
+				"The following add-ons failed to update: {}",


CyrilleB79 · 2023-12-19T13:26:12Z

+		addonFailureMessages.append(
+			ngettext(
+				# Translators: Shown when one or more add-ons failed to be uninstalled.
+				"The following add-on failed to uninstall: {}",


CyrilleB79 · 2023-12-19T13:26:45Z

+			ngettext(
+				# Translators: Shown when one or more add-ons failed to be uninstalled.
+				"The following add-on failed to uninstall: {}",
+				"The following add-ons failed to uninstall: {}",


CyrilleB79 · 2023-12-19T13:27:57Z

+		addonFailureMessages.append(
+			ngettext(
+				# Translators: Shown when one or more add-ons failed to be installed.
+				"The following add-on failed to be installed: {}",


CyrilleB79 · 2023-12-19T13:28:12Z

+			ngettext(
+				# Translators: Shown when one or more add-ons failed to be installed.
+				"The following add-on failed to be installed: {}",
+				"The following add-ons failed to be installed: {}",


seanbudd

Thanks @lukaszgo1

…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.

…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.

…s, removals and updates cannot complete (nvaccess#15937) Reverts nvaccess#15921 Closes nvaccess#12823 Addresses the most likely cause of nvaccess#15719 Hopefully fixes nvaccess#15927 Summary of the issue: PR nvaccess#15921 started removing all add-ons which failed to be installed without showing warnings to the user. Users generally expect NVDA to attempt installation / removal of add-ons on the next restart if they failed, to have a warning when one of these operations didn't succeed and not to be able to install add-on which is not fully removed from Add-ons Store. Description of user facing changes When one or more add-ons fails to be updated, uninstalled or installed, NVDA shows an appropriate warning on startup. Installation / removal is attempted on the next restart. When add-on is pending remove it is shown in the Add-ons Store along with the 'pending remove' status. All add-ons which are no longer present on disk are removed from the list of pending installs to hopefully improve nvaccess#15719. Description of development approach NVDA stores add-ons whose installation / removal failed in a list. All these add-ons are not loaded ,but shown in the Add-ons Store. These lists are used to determine what warnings should be shown to the user. List of pending installs is cleaned-up from add-ons which are no longer present on disk, excluding these which failed to be installed, as their install is going to be attempted on the next restart.

…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.

lukaszgo1 added 4 commits December 15, 2023 17:25

Revert "Add-on store: Clean up failed installs (nvaccess#15921)"

5f344e8

This reverts commit 81e17be.

Show warnings when add-ons failed to install / uninstall and clean up…

0c896c4

… no longer existing pending installation entries.

Merge branch 'beta' into 15921_fixup

ec9e408

Fix-up for pending override compat

122ad56

lukaszgo1 requested a review from a team as a code owner December 18, 2023 16:08

lukaszgo1 requested review from seanbudd and removed request for a team December 18, 2023 16:08

This was referenced Dec 18, 2023

After #15921, there is still unconsistent information and an unexpected restart dialog #15927

Closed

Add-On Store Dialog is always triggering "Changes were made" dialog, whether changes were made or not. #15719

Closed

Bump for CI

85a2a4f

seanbudd reviewed Dec 19, 2023

View reviewed changes

seanbudd marked this pull request as draft December 19, 2023 00:57

seanbudd added this to the 2024.1 milestone Dec 19, 2023

lukaszgo1 added 2 commits December 19, 2023 10:55

Merge branch 'beta' into 15921_fixup

72cd22b

Review actions

376a7ab

lukaszgo1 marked this pull request as ready for review December 19, 2023 11:27

CyrilleB79 approved these changes Dec 19, 2023

View reviewed changes

Second wave of review actions

d5d2942

seanbudd approved these changes Dec 19, 2023

View reviewed changes

seanbudd merged commit 32a8613 into nvaccess:beta Dec 19, 2023

seanbudd mentioned this pull request Dec 19, 2023

Add-on store, Updatable add-ons show add-ons which you installled and don't require an update #15935

Closed

lukaszgo1 deleted the 15921_fixup branch December 20, 2023 11:08

lukaszgo1 mentioned this pull request Dec 26, 2023

Remove add-on modules imported in install tasks to avoid conflicts between add-ons #15967

Merged

5 tasks

		_shuttingDownFlagLock = threading.Lock()


		def _showAddonsWarnings() -> None:

	def _showAddonsWarnings() -> None:
	def _showAddonsUpdateErrors() -> None:

	def _showAddonsWarnings() -> None:
	def _showAddonsErrors() -> None:

	"The following add-on failed to update: {}",
	"The following add-on failed to update: {}.",

Uh oh!

Conversation

lukaszgo1 commented Dec 18, 2023

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Dec 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukaszgo1 commented Dec 19, 2023

Uh oh!

CyrilleB79 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants