Skip to content

Add-on store: Clean up failed installs#15921

Merged
seanbudd merged 2 commits into
betafrom
fix-15719
Dec 14, 2023
Merged

Add-on store: Clean up failed installs#15921
seanbudd merged 2 commits into
betafrom
fix-15719

Conversation

@seanbudd

@seanbudd seanbudd commented Dec 14, 2023

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #15719

Summary of the issue:

If an add-ons install failed, it might never be cleaned up, causing NVDA to be constantly expecting there are add-ons pending install.
This causes a warning dialog that NVDA must be restarted when closing the add-on store.

Description of user facing changes

Pending installs that fail are removed and cleaned up, ensuring it is easy to attempt a future install.
The warning dialog no longer fires as NVDA cleans up failed installs.

Description of development approach

Clear the pending install set and delete pending install add-on folders after loading add-ons.
When loading add-ons, all installs should complete, and there should be no new pending installs from the user.

If an install fails, also attempt to clean up the files immediately after.

Testing strategy:

Tested in source code by forcing an add-on to be in the pending install set, and preventing the "pendingInstall" folder of an add-on be renamed/moved.

Known issues with pull request:

None

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.

@seanbudd seanbudd requested a review from a team as a code owner December 14, 2023 00:50
@seanbudd seanbudd requested review from michaelDCurran and removed request for a team December 14, 2023 00:50
@seanbudd seanbudd added this to the 2024.1 milestone Dec 14, 2023
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 03a31808b0

@lukaszgo1

Copy link
Copy Markdown
Contributor

Merging this PR will reintroduce #12629, fixed in #12792.
From the discussion on that issue, it looks like user expectation was that when given add-on fails to install the installation would be retried when whatever prevents it has been resolved.
To improve #15719 it should be sufficient to keep a list of add-ons which failed to install, compare it with the pending install state and remove only add-ons which are no longer on the file system, but still in the pending install set. Obviously in the ideal world we would also understand how they ended up there, but that is secondary. Also cc @CyrilleB79 reporter of #12629.

@CyrilleB79

Copy link
Copy Markdown
Contributor

Tanks for tagging me.

Actually, with this PR, if an add-on cannot be removed, it will not show up anymore in the installed add-on list (as pending remove).
Thus it allows to reinstall the same add-on, what is probably not desirable before the previous version of the add-on is fully installed.

More generally, about #12629, we can accept that in such situation, the add-on fails to install and does not try again at next restart. But in this case, the user should be warned of this failure.

@seanbudd

Copy link
Copy Markdown
Member Author

Agreed - I don't see how this reopens #12629. After a failed install is cleaned up it can always be re-installed.
Generally, the user should be warned if an installation fails. If we can find new edge cases where this doesn't happen, it should be fixed,

@CyrilleB79

Copy link
Copy Markdown
Contributor

This PR does not address all the unexpected restart dialog issues and performs silent fixes (folder removal), leading to unexpected and inconsistent results.

The test plan was quite light.

I have opened #15927 to describe the issues. My opinion is that #15927 should be addressed in NVDA 2024.1.

lukaszgo1 added a commit to lukaszgo1/nvda that referenced this pull request Dec 16, 2023
seanbudd pushed a commit that referenced this pull request Dec 19, 2023
…s, removals and updates cannot complete (#15937)

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.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Fixes nvaccess#15719

Summary of the issue:
If an add-ons install failed, it might never be cleaned up, causing NVDA to be constantly expecting there are add-ons pending install.
This causes a warning dialog that NVDA must be restarted when closing the add-on store.

Description of user facing changes
Pending installs that fail are removed and cleaned up, ensuring it is easy to attempt a future install.
The warning dialog no longer fires as NVDA cleans up failed installs.

Description of development approach
Clear the pending install set and delete pending install add-on folders after loading add-ons.
When loading add-ons, all installs should complete, and there should be no new pending installs from the user.

If an install fails, also attempt to clean up the files immediately after.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
…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.
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.

5 participants