Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An opened file prevents add-on removal in the future #12629

Closed
CyrilleB79 opened this issue Jul 9, 2021 · 7 comments · Fixed by #12792
Closed

An opened file prevents add-on removal in the future #12629

CyrilleB79 opened this issue Jul 9, 2021 · 7 comments · Fixed by #12792
Milestone

Comments

@CyrilleB79
Copy link
Collaborator

CyrilleB79 commented Jul 9, 2021

Steps to reproduce:

Example with the Character Information add-on

  1. Open a file of the add-on in an editor, e.g. %appdata%\nvda\addons\charInfo\globalPlugins\charinfo\__init__.py
  2. Open Add-on manager and remove Character Information add-on
    The status of the add-on is "Enabled, Removed after restart"
  3. Close add-on manager and restart NVDA as asked.
  4. Open add-on manager
    The status of the add-on is "Enabled, Removed after restart". This is expected because the add-on could not be removed due to the file opened in the editor.
  5. Close the file of the add-on opened in the editor.
  6. Close the add-on manager and restart NVDA as asked.
  7. Open add-on manager and check the add-on's status
  8. Execute steps 5. to 7. as many times as you want.

Actual behavior:

At step 7., the status of the add-on is "Enabled, Removed after restart".
The only way I have found to remove this add-on is to remove manually its folder or to reinstall it before removing it again.

Expected behavior:

At step 7., the add-on should not appear anymore in the list since nothing prevents NVDA anymore to remove the add-on after step 6.

System configuration

NVDA installed/portable/running from source:

Installed

NVDA version:

2021.1rc2

Windows version:

Windows 10 1809 (64-bit) build 17763.1935

Name and version of other software in use when reproducing the issue:

N/A

Other information about your system:

N/A

Other questions

Does the issue still occur after restarting your computer?

Yes

Have you tried any other versions of NVDA? If so, please report their behaviors.

Yes, this error has always been present, surely in all versions of NVDA 2019.3 and beyond.

If add-ons are disabled, is your problem still occurring?

N/A

Does the issue still occur after you run the COM Registration Fixing Tool in NVDA's tools menu?

Yes

@Brian1Gaff
Copy link

Brian1Gaff commented Jul 11, 2021 via email

@seanbudd seanbudd added this to the 2021.3 milestone Jul 29, 2021
@lukaszgo1
Copy link
Contributor

I cannot reproduce this with 2021.1 nor with latest Alpha running from sources. At first I thought this might be specific to Char Info but its not reproducible with it either. @CyrilleB79 Perhaps you've forgot to close the folder from which you've started the editor - this happened to me several times in the past and caused add-on removal process to fail.

@CyrilleB79
Copy link
Collaborator Author

@lukaszgo1 I cannot re-test for now.
Anyway if Windows Explorer is opened at step 1. along with the file but closed at step 5., the add-on may not be removed at step 2. but should be removed at step 6. in any case. Do you agree?
Would it be reproducible on your side keeping opened not only the file but also Windows Explorer before step 5.?

@lukaszgo1
Copy link
Contributor

I agree that add-on should be removed as soon as both the file in the editor and Windows Explorer opened in the add-on folder are closed and that is already the case for me in the latest Alpha / 2021.1.

@Adriani90
Copy link
Collaborator

@CyrilleB79 could it be possible that the process of the editor is still running in the task manager after closing the file? Did you check that?

@CyrilleB79
Copy link
Collaborator Author

Retesting it, I cannot reproduce with the exact same steps but I can reproduce with slightly differents steps. To simplify the issue and avoid phantom processes, I do not open any file in the editor but only keep a subfolder of the add-on opened in the Explorer, e.g. the doc subfolder.
And instead of only removing the add-on, I re-install (or update) an add-on that is already installed.
This may happen with any add-on and not only charInfo. This time, I have tested with sayProductNameAndVersion.

@lukaszgo1 could you try this? Thanks.

@lukaszgo1
Copy link
Contributor

Yeah - I can reproduce with your latest STR - caused by a very simple coding mistake. I'll submit a PR for this and some other issues related to the add-on removal which I've found along the way when testing later today or tomorrow.

@feerrenrut feerrenrut removed this from the 2021.3 milestone Sep 3, 2021
seanbudd pushed a commit that referenced this issue Sep 8, 2021
…in `addonHandler` (#12792)

Fixes #12629

Summary of the issue:
As described in #12629 it is impossible to update add-on when its folder is opened when initiating upgrade. During investigation I also encountered various other errors during add-on removal in particular:

- If a disabled add-on is uninstalled and then reinstalled it stays disabled even though source code comments clearly states that it should not be happening.
- If a folder of the add-on is renamed and its name no longer agrees with the name in the manifest it cannot be uninstalled via add-ons manager and there is absolutely no indication in the log as to what went wrong.

Description of how this pull request fixes the issue:
1. addon installation cannot be finished when add-on folder was opened during installation was caused by the fact that after attempting to install add-ons pending installation list of all add-ons pending install was emptied even if the given add-on failed to install. This has been fixed by removing add-ons from list of pending installs only if installation was successful - otherwise NVDA would retry on the next restart. In addition for configurations which can be potentially affected by this I've also made sure that for all folders which name ends with suffix denoting add-ons pending installation NVDA tries to install them regardless of their presence on the list.
2. disabled addons are not removed from the list of disabled add-ons after uninstallation was caused by the fact that there were two lists of disabled add-ons. The first one in the state and the second one _disabledAddons was duplicating content of the list in the state Unfortunately the second list was populated after add-ons were removed yet names of the disabled plugins were removed from it rather than from the list in the state. This has been fixed by no longer using _disabledAddons at all - it ended up to create more confusion than it is worth and added no real benefits. I've also make sure that for old configs all add-ons which are no longer installed which are present in the list of disabled add-ons are removed.
3. addons for which directory is named differently than the name in the manifest was solved by uninstalling add-ons after getting manifest data i.e. no longer rely on directory name matching the manifest name.
In the process I've also made addons state a class which has methods for loading, saving etc. rather than just having a simple dict with various top level function operating on it. This is backwards compatible.

Testing strategy:
Made sure that state created by previous version of NVDA can be successfully loaded
Made sure that state created by the code from this PR can be successfully read by previous versions of NVDA
Renamed folder of an add-on so that name of the directory is different than the name in the manifest - made sure that it can be uninstalled from add-ons manager
Uninstalled disabled add-on - made sure that it is removed from the list of disabled add-ons in the state
Opened folder of an add-on in Windows Explorer, tried to update the plugin, closed the opened folder - made sure that after NVDA restart add-on has been successfully installed.
@nvaccessAuto nvaccessAuto modified the milestone: 2021.3 Sep 8, 2021
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 a pull request may close this issue.

7 participants