nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.1k stars 634 forks source link

Revised approach to addon compatibility #9055

Closed feerrenrut closed 5 years ago

feerrenrut commented 5 years ago

This issue aims to discuss a revised approach to addon compatibility.

Issues with the existing Addon versioning checks

User stories

Consider the following situations:

The following use cases can be derived:

 

Addon API background

 

Version checks

An addon specifies its window of compatibility with two manifest values, and will be considered compatible if the following are all True:

A more in depth look at these properties:

NVDA specifies a window for the API versions in (a new file) addonAPIVersion.py

Examples

When the two windows of compatibility overlap, then the add-on is considered compatible and can be enabled. For the ease of discussion, we will assume API version values are consecutive integers, however the exact scheme used for API version values will be discussed later. Consider an installation of NVDA 2020.2, with current API version of 6 and backwards compatible to API version 3:

First version with addon version checks

Release 2019.1 with add-on version check ASAP

Addon API Version Value Scheme

So far, we have used strings formatted as NVDA version strings for addon API version values. This caused difficulty when pulling the NVDA version string from the file information in the launcher executable because of the presence of other information. We have decided to ignore the minor part when considering whether an addon has been tested.

Consider:

With regards to decoupling:

 

Alternatives to NVDA version string:

My preference is to use the tuple representation.

Changes to the UX

When there is an incompatibility, we would no longer state merely that the addon is untested and allow the user to override the incompatibility state. Instead, we will warn that it is incompatible, and the addon will be disabled after installation. The user should always be able to abort the installation.

One improvement could be to specifically detect and highlight incompatible addons that are providing the active synth / braille display. Since these are considered critical to the user.

Addon publishing platforms

Those responsible for addon publishing platforms and reviewing the addons contained there-in should note the importance of the accuracy of the last tested addon manifest value. If it is set to as-yet undeveloped future API versions, it will likely result in instability and reflect badly on the add-on publishing platform and those who reviewed the addon.

LeonarddeR commented 5 years ago

Thanks for this extensive write-down!

* A numerical (`float`) representation of the version string like `19.20` E.G. `{year}.{major}{minor}`

  * If there were more than 10 minor releases, ordering is violated.

This is very unlikely to happen.

* A tuple, structured in the same way as the build version tuple.

  * Rather than getting the file version info from the launcher executable, the server would provide the information with other update information.

Note that we would still have to parse a string within updateCheck.checkForUpdate. Alternatively, the update server should return json data for versions of NVDA that support it.

  * The tuple would have to be parsed from the addon manifest (as it currently is), however we can be strict about the format of this.

I believe that configobj can parse them quite will. a definition would look like: lastTestedAPIVersion = 2018, 4, 10 We can even enforce the length of the tuple, though I'd rather see a minimum length of 2 (year.major)

My preference is to use the tuple representation.

Agreed!

When there is an incompatibility, we would no longer state merely that the addon is untested and allow the user to override the incompatibility state. Instead, we will warn that it is incompatible, and the addon will be disabled after installation. The user should always be able to abort the installation.

That basically means the following:

Apart from that, I think that the gui wording and experience is pretty ok as it is now. Thus, I propose keeping the "I understand that these add-ons will be disabled" checkboxes, etc.

One improvement could be to specifically detect and highlight incompatible addons that are providing the active synth / braille display. Since these are considered critical to the user.

This makes sense, and is pretty straight forward when the add-on is running:

>>> import addonHandler
>>> import braille
>>> addonHandler.getCodeAddon(braille.handler.display.__class__)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "addonHandler\__init__.pyc", line 534, in getCodeAddon
AddonError: Code does not belong to an addon package.
>>> addonHandler.getCodeAddon(speech.getSynth().__class__)
<addonHandler.Addon object at 0x04296DD0>

In my case, this only applies to the speech synthesizer.

feerrenrut commented 5 years ago

Note that we would still have to parse a string within updateCheck.checkForUpdate

Yes, but I'm not so worried about this, since we can be strict about the formatting of this. Unlike the version string coming from the executable's file info.

I believe that configobj can parse them quite will.

I'm happy for the definitions to remain as strings in the addon manifest, and parse them with regex as we currently do. I suggest that we strictly validate that there are 3 numbers provided.