New addon api versioning approach#9151
Conversation
NVDA now uses two values to determine addon compatiblilty.
Read from addon manifest and validate and convert to tuple. When checking for NVDA updates, convert to tuple and save state as tuple. This saves confusion about whether its a string or a tuple. Fix a few other bugs.
…mnCheckListCtrl Clean up of the accPropServices needs to happen when the control is destroyed. The Destroy method is not called by the wx framework, but we can register to receive the event. wxWidgets/Phoenix#630
All usages of ConfirmAddon specify the argument with a keyword so its safe to do this.
Extracts out common showAddonInfo function.
It's not sensible to have a minRequiredVersion later than the version tested against.
This is already checked (and an AddonError raised) when creating the Addon
Have you considered that this case also applies when one manually downloads a new version of NVDA and runs the particular snapshot? I'm afraid this will happen:
Would it help to disable the add-ons manager on launcher copies altogether and disallow launcher copies to touch the state? Alternatively, we can just disable the enable/disable functionality from the add-ons manager when the launcher is active? |
LeonarddeR
left a comment
There was a problem hiding this comment.
Not yet fully reviewed, will have more time later today.
Yes, thanks for reminding me. I'll look into what we can do about this. |
Review actions for #9151
More consistent initialisiation. There is also nowhere that is checking for None Review actions for #9151
Review actions for #9151
Ensure that a warning is put in the log for access denied errors when setting audio ducking.
Use windows sounds for addon install warning / error.
|
Recently we had a tech support request where the user stated that sayAll did not work unless restarting with add-ons disabled. However, the user actually had no add-ons installed. It ended up pointing to the fact that they probably had some very old stand-alone globalPlugins etc in their NVDA config directory. |
|
If we feel this might be still used for development, then we could consider adding an extra commandline argument to enable this. |
When scripting corporate applications for customers, I tend to throw the appModules directly in the %appdata%\nvda folder, since the creation of add-ons causes some overhead. Good to know that I misbehaved in that case😜. Having said that, I prefer a configuration setting over a command line parameter, but would at least prefer either one of them.
Note that the addon subsystem relies on these calls, see the bottom of |
|
@michaelDCurran wrote:
It is useful not only for development, but also for more advanced users, who wants to change default NVDA behavior. I've for example created global plugin which fixes #8600 until that would be addressed in the core. Disabling tis possibility would be very bad for users. |
|
While developing I understand. But for advanced users you *should*
package it as a versioned add-on. If this is currently too complex, then
we should look at how to make it easier to quickly create an add-on that
has appropriate version info.
There are people who still have unpackaged code floating around in those
directories, and no amount of version checks we implement on add-ons are
going to disable that.
Code distributed to clients' machines really should be in an add-on. If
that is not performant, then we really must look at why that is.
|
|
But yes you are right, we need to change the function to not add the
NVDA user config directory, but still keep calling the function itself.
|
|
Replying to @lukaszgo1
For anyone else interested, this quote comes from the NVDA devel mailing list thread, specifically this message (I recommend downloading the message as HTML by the way, since there are many block quotes which are not displayed very well in sourceforge)
No, I don't think the problem is as you are stating. The addons will only become incompatible when the internal NVDA API version is moved on.
A solution could be found on the add-on distribution side allowing NVDA alpha users to be able to install an "alpha" version of the add-on. This add-on would be targeting the unreleased version of NVDA. This essentially becomes a distribution problem. This option is essentially:
Despite my dislike for this in the quoted email thread, I think it would be ok if this were very clearly alpha software, and not mixed with the stable add-ons that most would be installing. Another possible solution might be to allow NVDA (only) users on the alpha update channel to "allow incompatible add-ons" from an advanced section of the NVDA preferences. We will have to think carefully about the implications and behavior of this. Because there is no imminent cause for add-ons to become incompatible. I won't be addressing this concern in this PR. There is at least one full release cycle, likely more before there will be a change to the addon API version. |
|
Replying to @michaelDCurran
I am totally on board with this. However I would prefer to do this in a follow up PR. |
- Allow the possibility of using a parent window other than the addon manager. Useful in the case of installing via a shell extension, where gui.mainframe should be the parent. - Remove coupling between installing an Addon and addon manager - Reduce nesting - Moved methods to module level. These had no dependence on instance, or class level members.
|
Hi @LeonarddeR I have pushed a commit that I am fairly confident resolves the issue with installing addons from the shell integration. However, I think there are still issues with this when you are running an unsigned build. As such, please test with the following try build: https://ci.appveyor.com/project/NVAccess/nvda/builds/21975742/artifacts In terms of unifying the way that versions are printed, I have decided to leave this as is for now. There are several different expectations for the version string formatting, trying to force these all to use the same function actually makes it unnecessarily complicated and error prone. Ideally, I would like to see the introduction of a "versionNumber" type, 4-part where build numbers matter, and 3 part elsewhere. But this is actually quite out of scope for the originally issue. |
| # #4460: If we do this immediately, wx seems to drop the WM_QUIT sent if the user chooses to restart. | ||
| # This seems to have something to do with the wx.ProgressDialog. | ||
| # The CallLater seems to work around this. | ||
| wx.CallLater(1, self.Close) |
There was a problem hiding this comment.
I guess the context manager is the alternative approach for this? Are you sure that it won't introduce #4460 again?
There was a problem hiding this comment.
Quite sure, this self.Close is for the AddonManger dialog. The call to wx.CallLater only happened when an instance of the AddonManager had to be created for the purpose of installing an addon from the shell integration. When there was an AddonManager dialog was already opened, or installation was triggered by the AddonManager then this wasn't hit. I expect part of the problem was in calling del progressDialog rather than Destroy as well. But either way, it's no longer necessary to destroy the progressDialog and the AddonManger at once.
LeonarddeR
left a comment
There was a problem hiding this comment.
I vote for merging this as soon as possible, fixing possible regressions it may introduce into subsequent PRs.
One function is for outputting a full version string (including build part) The other is for formatting a version string for the GUI (which will remove the minor part when it is zero).
Clarified the name of the function to report that an addon is no longer supported by NVDA. The addon is considered "too old". As opposed to NVDA being "too old" for a newly developed addon.
|
@LeonarddeR I have pushed a couple of commits that hopefully simplify a few things. I would still like to see a unified approach to passing around a "version" concept. But it should not be part of this PR. @michaelDCurran Unless you have any objections, I will merge this tomorrow. |
|
@feerrenrut Ugh, missed this again, but could you please update the developer guide according to the three integer versioning scheme for minimumRequired and lastTested API versions? |
Oops, yes I will do that now. |
Manifest value explanations needed updating to match the new validation requirements.
Link to issue number:
Fixes #9055
Summary of the issue:
The approach to addon compatibility checks placed a large burden on both users and addon developers.
Description of how this pull request fixes the issue:
The compatibility check now relies on two values within NVDA,
addonAPIVersion.CURRENTandaddonAPIVersion.BACK_COMPAT_TO, as well as the two values expected in the addon manifest,lastTestedNVDAVersionandminimumNVDAVersion. There needs to be an overlap in these version ranges for the addon to be considered compatible.The concept of "untested, but potentially still works" is no longer permitted. As such, the user is no longer able to "whitelist" an addon. When an addon is considered incompatible, the addon itself must be updated and re-installed. However, due to the introduction of a second value within NVDA (
addonAPIVersion.BACK_COMPAT_TO) which will only need to be updated when there are breaking API changes (expected to happen rarely), there is no longer the requirement for the addons to be updated for each release.This PR also stops the error sound from being heard on developer builds of the launcher (access denied error in audio ducking). This was quite distracting while testing and is essentially an expected error.
Testing performed:
Known issues with pull request:
year.month.minor(with all parts present) will need to be re-released.Change log entry:
The current changelog entry could be slightly modified from:
to