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

More flexibility to define version of add-ons #407

Open
CyrilleB79 opened this issue Apr 2, 2023 · 14 comments
Open

More flexibility to define version of add-ons #407

CyrilleB79 opened this issue Apr 2, 2023 · 14 comments

Comments

@CyrilleB79
Copy link
Contributor

If I am not mistaken, the validation process of the add-on store requires that the submitted add-on have a manifest with "version" field being strings formatted as "X.Y" or "X.Y.Z" with X, Y, Z belonging to class [0-9]+, i.e. being number.

Issue

Since the "version" field is also the one seen in the add-on manager of NVDA, I feel this policy quite restrictive. When creating dev or beta add-ons, it would be nice for users to have version name such as "2.1beta2", "2023.1-dev" or whatever.

Moreover, a user seeing version number "20230506.0.0" or "2.0.1" in the add-on manager (or the future add-on store client) cannot know easily if the installed version of an add-on is a dev one, a beta one or a stable one.

Solution

Use an optional additional "versionNumber" field in the manifest dedicated to the add-on store:

  • if this field is missing, "version" field should be parsed by add-on store validation process
  • if this field is present, it should be parsed instead of "version" field by the add-on validation script

This would allow to make an add-on with visible version "2.1beta1" and versionNumber "2.1.1" to be parsed for the add-on store submission.

Additional

NVDA itself takes advantage of having a version number (e.g. '2023.1.0.27913') and a version name (e.g. "2023.1")

Cc @nvdaes

@CyrilleB79
Copy link
Contributor Author

Note: In case you disagree with this proposal, the current strong requirement on the version field should be made explicit in the submission process since it is an additional requirement for the store that is not a requirement for a generic NVDA add-on.

@seanbudd
Copy link
Member

seanbudd commented Apr 3, 2023

Would an alternative solution be to add the channel as an optional field in the add-on manifest?
That way publishers can easily mark an add-on as beta or dev just like in the add-on store

@XLTechie
Copy link
Contributor

XLTechie commented Apr 3, 2023 via email

@CyrilleB79
Copy link
Contributor Author

Would an alternative solution be to add the channel as an optional field in the add-on manifest? That way publishers can easily mark an add-on as beta or dev just like in the add-on store

The "channel" field already exists in the add-on template and thus in many add-ons in the wild. Probably not an NVDA requirement but used by Add-on Updater. Historically, it may contain value "dev" or "stable" (or "None"), defaulting to "stable".
Cc @josephsl

Solution 1

The "channel" field may be reused using "dev", "beta" or "stable" values, with the little issue that legacy add-ons advertising "dev" not all conform to the new "dev" definition of the add-on store, i.e. before the add-on store, some add-ons have used "dev" channel for early testers of new features (wich is now called "beta").

In this case, NVDA add-on manager should also present the update channel. For example, an add-on having a manifest with:

version = 2.1
channel = beta

would have the value "2.1 beta" in the version column of the add-on manager.

This causes a difficulty with the legacy dev add-ons which would have a manifest such as:

version = 20230101-dev
channel = dev

since they would be presented as "20230101-dev dev" in the "Version" colum of the add-on manager.

Solution 2

You may create a new "addonStoreChannel" field in the manifest. It would behave the same way as solution 1. Except that it gets rid of the issue with legacy dev add-ons that were using the "dev" channel in a slightly different way that today's add-on store definition.

Issue with solution 1 and 2

If I would like to create the following add-on versions:

  • 2.1beta1 # The beta 1 release of myAddon v2.1
  • 2.1 beta2 # The beta 2 release of myAddon v2.1 with a fix on top of 2.1 beta 1
  • 2.1 # The stable release of myAddon v2.1, same as 2.1beta2 except version

With solutions 1 and 2, I think that the add-on store does not allow me to define 3 versions 2.1 since it does not allow three versions 2.1.0. If instead I use 2.1.0, 2.1.1 and 2.1.2 for these three versions, in the add-on manager I will have: "2.1.0 beta", "2.1.1 beta", "2.1.2", which IMO is less clear.

Solution 3

As described in the initial description of this PR, i.e. use two fields: "version" for the visible version number and "versionNumber" for the number used internally by the add-on store.
This 3rd solution allows to get rid of the issues with solution 1 and 2.

@XLTechie
Copy link
Contributor

XLTechie commented Apr 3, 2023 via email

@CyrilleB79
Copy link
Contributor Author

Solution 3 As described in the initial description of this PR, i.e. use two fields: "version" for the visible version number and "versionNumber" for the number used internally by the add-on store. This 3rd solution allows to get rid of the issues with solution 1 and 2.
Since we already have, in the JSON, "addonVersionNumber" and "addonVersionName", couldn't we do what I already thought was going on before @seanbudd clarified, and allow addonVersionName to be freeform, and give it its own manifest key? Keep number with its current validation, but allow name to be defined however the author thinks will be most clear to the user in the store. Further, if name is left optional, if it isn't set, the number can be used as the name. But if it is set, it will override the name derived from number. The major changes would be that: 1. addonVersionName will be an optional settable key in the manifest. Maybe call it "displayName" or similar. 2. Version, as it is now as a string, will still be converted to addonVersionNumber. If addonVersionName is set in manifest, it becomes the addonVersionName in the JSON, shown as the version in the store. But if addonVersionName is not set in the manifest, it can be generated from the old version field, just as it is now.

This proposal is also interesting.

But I prefer my proposal. Indeed, the field "version" is already the one displayed by NVDA (current, future but also previous versions) in the add-on manager. I think it makes sense to continue to display this version (e.g. "2.1 beta 1") in the add-on manager. In the future, the store could indicate both versions, i.e. "2.1 beta 1" and "2.1.0".

With Luke's proposal, NVDA needs to be modified so that "addonVersionName" be displayed instead of "version" in the add-on manager (until the store replace it of course). And old versions of NVDA can not be modified this way. That mean that a future add-on beta version "2023.06.01 beta" = "2023.6.1" would be displayed "2023.6.1" without indication of "beta" in an old version of NVDA (e.g. 2023.1)

@josephsl
Copy link
Contributor

josephsl commented Apr 3, 2023 via email

@seanbudd
Copy link
Member

While we are using both a manifest and a JSON metadata, implementing this would require a new field in the manifest, and more complex matching rules to the JSON metadata.
This may be hard for submitters to follow and also prone to bugs.
It would mean that we would essentially have to support 3 schemas, the old manifest, the new manifest and the JSON metadata schema.

If we migrate to just using the JSON metadata, rather than the current add-on manifest, this problem becomes much simpler.
addonVersionName can become an independent string, and addonVersionNumber can be kept for an internal ordered version.

I would prefer to hold off until a future breaking release, and migrate to the JSON metadata schema so we only support 1 schema, rather than 2 or 3.

@XLTechie
Copy link
Contributor

XLTechie commented Apr 11, 2023 via email

@XLTechie
Copy link
Contributor

XLTechie commented Apr 11, 2023 via email

@seanbudd
Copy link
Member

I think it would be okay to support both, but I must warn that this is a big task to take on.
It requires updating the JSON schema to support some fields in the manifest that are not covered.
It also requires having to update NVDA, this repository and addon-datastore-validation.
There are a lot of unknowns in supporting both, and you might be blocked on discussions from NV Access a lot.
At the very least, I would hold off on work until nvaccess/nvda#13985 is merged and settled.

@CyrilleB79
Copy link
Contributor Author

I have probably missed something.

But why would this switch to Json be necessary on NVDA side?

Before add-on store, I was already creating versions such as "2.1-dev-20230417", which were supported by NVDA and which were sort of beta for my own add-ons.

The field "version" in the manifest was just a string and NVDA itself had no notion of version ordering. The capability of ordering is being added by the add-on store so this would make sense to add the "versionNumber" field for this usage.

IMO if this versioning proposal is not yet adopted at the time when the add-on store ships in NVDA, all the existing add-ons will define their version scheme according to new add-on store requirements at that time. And if this proposal is ever implemented, say for NVDA 2024.1, there is little chance that add-ons authors change again their versioning scheme.

@seanbudd
Copy link
Member

@CyrilleB79 as mentioned earlier, while we are using both a manifest and a JSON metadata, implementing this would require a new field in the manifest, and more complex matching rules to the JSON metadata.
This may be hard for submitters to follow and also prone to bugs.

Adding a new versionNumber field would require:

  • more complex rules to determine which field to use/validate with when generating the add-on submission JSON
  • more complex rules to determine how to parse, compare and display add-on versions in NVDA

This essentially becomes supporting 3 different schemas for add-on metadata. If we wait until an add-on breaking release, we can support one metadata schema. This is simpler for add-on submitters and NVDA developers. It also causes less churn on updating the schema, which has already caused confusion and difficulty.

@CyrilleB79
Copy link
Contributor Author

@CyrilleB79 as mentioned earlier, while we are using both a manifest and a JSON metadata, implementing this would require a new field in the manifest, and more complex matching rules to the JSON metadata. This may be hard for submitters to follow and also prone to bugs.

Adding a new versionNumber field would require:

  • more complex rules to determine which field to use/validate with when generating the add-on submission JSON
  • more complex rules to determine how to parse, compare and display add-on versions in NVDA

This essentially becomes supporting 3 different schemas for add-on metadata. If we wait until an add-on breaking release, we can support one metadata schema. This is simpler for add-on submitters and NVDA developers. It also causes less churn on updating the schema, which has already caused confusion and difficulty.

I have no concern regarding an additional difficulty for submitters provided the slight modification is correctly documented and advertised. I think that add-on authors are now all aware that we are currently in a test phase regarding the new add-on submission process and may better understand now another evolution than again beginning 2024.

Regarding the added complexity in code of add-on store submission/validation and in NVDA, it does not seem exagerated to me but I have not the precise knowledge on all these topics, so my opinion does not weight so much.

Anyway, I have now expressed all my opinions. The decision is NVAccess' one and you have the global picture better than I. Thanks for the explanation you have given in any case.

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

No branches or pull requests

4 participants