Fix various issues when removing addons and improve state management in addonHandler

[lukaszgo1]

Link to issue number:

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

Point 1 i.e. add-on 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.
Point 2 disabled add-ons 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.
Point 3 add-ons 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 successffully 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.

Known issues with pull request:

None known

Change log entries:

Bug fixes

• Some bugs related to add-ons removal has been fixed.

Code Review Checklist:

• Pull Request description is up to date.
• Unit tests.
• System (end to end) tests.
• Manual testing.
• User Documentation.
• Change log entry.
• Context sensitive help for GUI changes.
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers

[lukaszgo1]

cc @seanbudd Given you've added #12629 to 2021.3 you may want to review this. Also @CyrilleB79 your testing and review (if you have time) would be greatly appreciated.

[CyrilleB79]

I have tested the STR in #12629 (comment) on a portable copy of nvda_snapshot_pr12792-23663,da8779bd.exe. Here is my feedback:

1. I confirm that the old add-on's folder is removed and that the new add-on's installation is completed as soon as you restart without an opened folder in Windows Explorer.

2. At first restart (with Windows Explorer still opened), the following error message can be seen in nvda-old.log:

ERROR - core.restart (12:52:00.892) - MainThread (7012):
NVDA already in process of exiting, this indicates a logic error.

This needs to be clarified.

3. After first restart (with Explorer still opened), the add-on is not present at all in the add-on manager. I would have expected it to be present with a status of pending install

4. At first restart (with Windows Explorer still opened), there are many error messages indicating permission denied to copy new files. I do not know if there is a way to clarify it in the log. I am even wondering if it would be worth warning the user with a message box, e.g. "The installation of one or more add-on could not be finalized and thus are not available. Please check that you have closed all files and folders and restart NVDA".

[lukaszgo1]

I have tested the STR in #12629 (comment) on a portable copy of nvda_snapshot_pr12792-23663,da8779bd.exe. Here is my feedback:

thanks for testing.

1. I confirm that the old add-on's folder is removed and that the new add-on's installation is completed as soon as you restart without an opened folder in Windows Explorer.

Great!

2. At first restart (with Windows Explorer still opened), the following error message can be seen in nvda-old.log:

ERROR - core.restart (12:52:00.892) - MainThread (7012):
NVDA already in process of exiting, this indicates a logic error.

This needs to be clarified.

I cannot reproduce this - are you sure this was not just a coincidence?

1. After first restart (with Explorer still opened), the add-on is not present at all in the add-on manager. I would have expected it to be present with a status of pending install

That is quite tricky to do - I'll see what I can do.

2. At first restart (with Windows Explorer still opened), there are many error messages indicating permission denied to copy new files. I do not know if there is a way to clarify it in the log. I am even wondering if it would be worth warning the user with a message box, e.g. "The installation of one or more add-on could not be finalized and thus are not available. Please check that you have closed all files and folders and restart NVDA".

I agree that would be nice and worth implementing at some point however there was no such warning previously. Should lack of the warning block this PR in your opinion?

[CyrilleB79]

2. At first restart (with Windows Explorer still opened), the following error message can be seen in nvda-old.log:

ERROR - core.restart (12:52:00.892) - MainThread (7012):
NVDA already in process of exiting, this indicates a logic error.

This needs to be clarified.

I cannot reproduce this - are you sure this was not just a coincidence?

I cannot reproduce this again. Let's ignore it.

4. At first restart (with Windows Explorer still opened), there are many error messages indicating permission denied to copy new files. I do not know if there is a way to clarify it in the log. I am even wondering if it would be worth warning the user with a message box, e.g. "The installation of one or more add-on could not be finalized and thus are not available. Please check that you have closed all files and folders and restart NVDA".

I agree that would be nice and worth implementing at some point however there was no such warning previously. Should lack of the warning block this PR in your opinion?

This PR is already an improvement (bugfix) with respect to the current situation. The warning message would be a plus and may be thought and integrated with this PR if possible. But in case of UX discussion, design problem or whatever, it should not block this PR in my opinion.

[lukaszgo1]

@CyrilleB79 I believe I've succeeded in making add-on show up in add-ons manager with the right status i.e. "Install, enabled after restart". Can you retest?
As to the showing a warning when add-ons install fail I'm not sure about right UX here. What you've proposed i.e. "The installation of one or more add-on could not be finalized and thus are not available. Please check that you have closed all files and folders and restart NVDA"
is not really indicative as to what files / folders should be closed and getting the exact list of what is being used is quite involved. TBH I'd rather leave this as is and if you feel strongly about showing a warning please create a new issue and someone can work on this subsequently.
The amount of errors in the log is partial indication of how complex this would be - the first set is about inability to remove folder of the previous add-on version and the second group about not being able to rename folder of add-on pending install to the new name since it exists already. Showing what exactly should be closed by the user in this case is going to be difficult.

[CyrilleB79]

I have just tested the last build (portable version of nvda_snapshot_pr12792-23664,b05d9cae.exe).
The indication in the add-on manager dialog is now correct.
And as already previously, the add-on actually finishes its installation on next restart provided that no folder of the add-on is opened anymore.
Good job!

Regarding the warning, again, I think it would be better to have it but it should not block this PR which is already a progress with respect to what happens now. Just to say that IMO there is no need to specify which file or folder cannot be renamed/deleted. Someone wanting to know it may look at the log.

[lukaszgo1]

Regarding the warning, again, I think it would be better to have it but it should not block this PR which is already a progress with respect to what happens now. Just to say that IMO there is no need to specify which file or folder cannot be renamed/deleted. Someone wanting to know it may look at the log.

You're looking at this from a perspective of a power user for whom looking at and understanding the log is not a problem. For a power user however, this dialog is not really necessary since they would look into the log anyway so the main group for whom a graphical warning is an improvement are beginner to intermediate users.

[CyrilleB79]

If I should consider myself a power user, I would say that the friendly reminder with the warning message box is a plus, even if I can cop without it.

For someone who is not used to dig into the log, there are two possibilities:

1. I expect that such people will not encounter the issue described in An opened file prevents add-on removal in the future #12629 because I guess they do not open the %appdata%\nvda\addons\myAddon folder at all. Or do I miss a use case?
2. If it ever happens for them, they just need to know to close all open files and folders and restart NVDA (or the computer to be sure that all is closed). Maybe the message should be refined this way.

[seanbudd]

I have a few concerns around backwards compatibility, which I have raised below. If it can't be maintained easily here, this can be looked at as a priority for 2022.1 - which isn't too far away from starting.

[seanbudd]

I also agree that a warning message may be helpful here, but it won't block this getting merged. A new issue should be raised for it, and perhaps a code comment on where it should go.

[lukaszgo1]

@seanbudd I believe all your review comments are now addressed.

[seanbudd]

Thanks for addressing those comments. I was thinking the following for the changelog entry - what are your thoughts? is this accurate? I think it was worth mentioning the change specifically.

Changes

- If a disabled addon is uninstalled and then re-installed it is re-enabled. (#12792)

Bug fixes

- Fixed bugs around updating and removing addons where the addon folder has been renamed or has files opened. (#12792, #12629)

Changes for developers

- The usage of functions ``addonHandler.loadState`` and ``addonHandler.saveState`` should be replaced with their equivalents ``addonHandler.state.save`` and ``addonHandler.state.load`` before 2022.1. (#12792)

[lukaszgo1]

Changes

- If a disabled addon is uninstalled and then re-installed it is re-enabled. (#12792)

IMO this should be a bug fix - this was always the intent it just never worked as it should.

Bug fixes

- Fixed bugs around updating and removing addons where the addon folder has been renamed or has files opened. (#12792, #12629)

Changes for developers

- The usage of functions ``addonHandler.loadState`` and ``addonHandler.saveState`` should be replaced with their equivalents ``addonHandler.state.save`` and ``addonHandler.state.load`` before 2022.1. (#12792)

The rest looks pretty nice.