When re-installing incompatible add-ons preserve compatibility override#15765
Conversation
| import addonAPIVersion | ||
| raise AddonError("Add-on is not compatible and over ride was abandoned") | ||
| state[AddonStateCategory.PENDING_ENABLE].add(self.name) | ||
| if self.overrideIncompatibility: |
There was a problem hiding this comment.
This code also appears not needed - enableCompatibilityOverride removes add-on from the list of blocked add-ons, so there is no reason to do so again.
|
have you tested the various scenarios when upgrading from a non-breaking release to a breaking release? I think much more thorough testing is required before changing this code. |
|
Marking as a draft until the requested testing is done. Having said that the code which handles blocked add-ons when upgrading / downgrading has not been touched in this PR, so I don't expect much troubles there. |
|
@seanbudd I have performed the test scenarios for upgrading / downgrading to different API versions from our manual testing steps, and mentioned this in the 'testing performed' section in the initial comment, so this should be ready for review. As an aside it would be really nice to create unit tests for these, as this seems quite doable, and performing these testing steps by hand is pretty tedious. |
seanbudd
left a comment
There was a problem hiding this comment.
Thanks for testing @lukaszgo1
I agree unit tests would be ideal if you'd like to add some basic ones.
| return _createGUIModelFromManifest(self) | ||
|
|
||
| @property | ||
| def _hasOverriddenCompat(self) -> bool: |
There was a problem hiding this comment.
This property should probably be added to SupportsVersionCheck like similar properties
I think this property might easily get confused with overrideIncompatibility, which now is only used when compatibility should already be overriden, not pending override.
| def _hasOverriddenCompat(self) -> bool: | |
| def _hasOverriddenCompat(self) -> bool: | |
| """If True, this add-on has been manually overriden. The affects of override may be pending restart""" |
There was a problem hiding this comment.
It would be worth adding a docstring to overrideIncompatibility as well
If
TrueNVDA should enable this add-on where it would normally be blocked due to incompatibility
There was a problem hiding this comment.
I've added docstrings you have suggested.
As to moving the property into SupportsVersionCheck, that was also my first thought when writing this PR, but now I'm not too sure about it. I placed it in the `AddonBase since:
- It is explicitly used only in
AddonBasesubclasses - Adding it into
AddonBasemakes boundaries betweenaddonHandlerand add-on store code more clear, thoug this is probably somewhat subjective - I cannot come with an use case in which it would have to be called against other subclasses of
SupportsVersionCheck, and since this is private code we can always move it if the need arises
There was a problem hiding this comment.
I'm not sure I understand the boundaries argument, as _hasOverriddenCompat is used in addonStore.
Logically grouping it with other version related checks is more useful in my opinion.
I think long term we want to whittle down and migrate addonHandler and addonGui code to addonStore and addonStoreGui.
There was a problem hiding this comment.
There's a huge number of properties for various add-on datastructures now.
There were 2 dev approaches to simplify them:
- Properties based on context (bundle, installed add-on, available from add-on store)
- Properties based on utility (version checking, status checking)
There was a problem hiding this comment.
Thanks, the last two comments quite clearly explained what was the intent behind the code layout, and also what is the eventual goal for the final code layout. I have moved the property in 5a433c1, though the fact that addonStore.models.version and addonStore.models.status require lazy imports to cooperate suggests to me that there is a room for improvements in the code here, though this is certainly out of scope for this PR.
I was thinking more about adding unit tests for various upgrading / downgrading with incompatible add-ons scenarios, as testing these is pretty time consuming, though that is something for a follow up PR IMO. |
| - When using say all or commands which spell text, pauses between sentences or characters no longer gradually decrease over time. (#15739, @jcsteh) | ||
| - NVDA no longer sometimes freezes when speaking a large amount of text. (#15752, @jcsteh) | ||
| - More objects which contain useful text are detected, and text content is displayed in braille. (#15605) | ||
| - When reinstalling an incompatible add-on it is no longer forcefully disabled. (#15584) |
There was a problem hiding this comment.
Do you wish to credit yourself on this topic? I am asking since you have done so on other topics.
IMO, adding the GitHub's name in the change log helps recognize one's work, but is also useful to know who has worked on each topic, e.g. for other devs/contributors.
There was a problem hiding this comment.
Thanks, that was an oversight on my part.
Link to issue number:
Fixes #15584
Summary of the issue:
When reinstalling incompatible add-on the fact that user manually overridden its compatibility was lost i.e. add-on was disabled after restart. This occurred because even though name of the add-on was added to the set of add-ons for which compatibility should be overridden, it was removed from the set after restart when uninstalling previous version of the add-on. Note that removal during uninstall is pretty much intentional i.e. when reinstalling add-on with the same name it should not be automatically marked as overridden compatibility, even if its previous version was.
Description of user facing changes
Reinstalled incompatible add-ons should no longer be forcefully disabled.
Description of development approach
Add-ons for which compatibility should be overridden on the next restart are placed in a separate state
PENDING_OVERRIDE_COMPATIBILITY. They're moved to the overridden compat state after restart. The code which enables add-ons, and verifies that given bundle can be installed has been modified to take the new state into account.Testing strategy:
Known issues with pull request:
I have removed some (apparently dead) code from
addonHandler.Addon.enablemethod. I'll add review comments explaining why it seems to be not needed. Please verify my assumptions when reviewing.Code Review Checklist: