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

Allow add-on API version strings to exclude Minor, defaulting to 0. #9331

Merged
merged 5 commits into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@michaelDCurran
Copy link
Contributor

michaelDCurran commented Feb 28, 2019

Link to issue number:

None.

Summary of the issue:

NVDA Alpha currently refuses to install or load add-ons whos minimumRequiredNVDAVersion and lastTestedNVDAVersion strings are insufficient for the current version of NVDA. However, NvDA is very strict about these version strings, inforcing that they must be in the form of Year.Major.Minor. Although this allows for matching on minor releases such as 2019.1.1, specifying the most common case of 2019.1 would require the author to write 2019.1.0, which although is logical, does not match other version strings displayed in NVDA. This even includes in the About Add-on dialog where currently the MinimumRequiredNVDAVersion and LastTestedNVDAVersion strings are displayed without the Minor part if it is 0.

Description of how this pull request fixes the issue:

Discussing with @feerrunrut we have now agreed to allow the Minor part to be dropped from the strings in the add-on manifest, and in this case 0 will be assumed.
This PR changes the regular expression used by the manifest validator to handle this case by making the .Minor part optional. Code already existed to handle the case where a match grouping was None (like it would be here) and falls back to adding 0 to the generated version tuple.
Examples in code comments have also been updated to reflect this, the developer guide has been corrected, and changes.t2t also now contains a more true list of changes regarding the add-on versioning features.

Testing performed:

Installed the UnicodeBrailleInput add-on from the add-on repository, where its API version requirements were 2017.3 to 2019.1.
Before this change, NVDA would complain that the add-on was invalid and refuse to install it in the first place. Now it installs, and meets the requirements for being enabled.
Similarly, an edited version of the same add-on with 2017.3.0 and 2019.1.0 respectively, also successfully installs and is enabled.
Also tested the same add-on but with a mimumRequriedNVDAVersion of 2019.1.1. (above the current API version). this refused to install.

Known issues with pull request:

None.

Change log entry:

None.

michaelDCurran added some commits Feb 28, 2019

The Minor part of an Addon's minimumNVDAVersion or lastTestedNVDAVers…
…ion is now optional and defaults to 0. E.g. It is now valid to write 2019.1 rather than having to write 2019.1.0.

@michaelDCurran michaelDCurran requested review from leonardder and feerrenrut Feb 28, 2019

@leonardder
Copy link
Collaborator

leonardder left a comment

Only some spelling things

Show resolved Hide resolved source/addonAPIVersion.py Outdated
Show resolved Hide resolved user_docs/en/changes.t2t Outdated
Show resolved Hide resolved user_docs/en/changes.t2t Outdated
Apply suggestions from code review
Co-Authored-By: michaelDCurran <michaelDCurran@users.noreply.github.com>
Show resolved Hide resolved user_docs/en/changes.t2t Outdated
Fix typo
Co-Authored-By: michaelDCurran <michaelDCurran@users.noreply.github.com>

@michaelDCurran michaelDCurran merged commit 5fbd648 into master Feb 28, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.1 milestone Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.