Force disable older versions of tony's enhancements add-on with 2023.3#15402
Conversation
|
While I appreciate this action, wouldn't it be better to disable WASAPI by default in 2023.3 and enable it in 2024.1 instead? People who really need WASAPI can still enable it. |
|
Hi @seanbudd I release a new version of tony's enhancements with Tony's permission. This version is compatible with 2023.2 and 2023.3. The add-on is currently available on the NV Access add-on store. Cheers |
|
But the overall goal of this PR still makes sense, and there may be other add-ons that require such a solution. |
|
@LeonarddeR Arguments can be made for and against this, but, IMO, it would be better to make WASAPI the default in 2023.3. It's become a lot more stable with the recent alphas. |
|
Now that Tony's add-on seems to be fixed. Should this PR still be merged? If yes, I think a strong clarification / explanation from NV Access should be done. Else, this gives the impression that:
IMO if the add-on is very popular and NV Access wants to help, it would be a better choice to submit a PR to the add-on (as for NVDA remote) than putting fixes in NVDA's code base. At last, regarding this PR, should only the add-ons in the add-on store be looked at? Or should we mention and report add-ons that are not (yet) in the store but that are popular in the various communities? |
|
While I appreciate this action, wouldn't it be better to disable WASAPI by default in 2023.3 and enable it in 2024.1 instead? People who really need WASAPI can still enable it. NVDA core will be prioritised over add-ons. Especially in this case, where the add-on breakage is due to relying on private API functions. Incompatible add-ons themselves being disabled is more typical of the NVDA UX as well.
We think so, yes. This case when updating with an older version of this add-on installed is still really bad. Per section 15 of the product vision: So I would suggest your second point "if things break NVDA in the future due to the use of private methods, NV Access will provide a fix, not to fix the add-on's capability but at least to avoid that NVDA be out of order" is closer to our current policy.
Fixing community made add-ons is also beyond our product vision as stands, rather we try and safeguard NVDA core to prevent such issues. Reasoning for this is fairly clear in this example, with no simple fix to the problem requiring the author to remove features from the add-on.
We've only tested add-ons in the store. Testing every add-on outside of the store is beyond our capacity. If other add-on issues are reported before the RC we can consider adding them to this fix. |
| from addonHandler import AddonStateCategory, state | ||
| overiddenAddons = state[AddonStateCategory.OVERRIDE_COMPATIBILITY] | ||
| assert self.name not in overiddenAddons and self.canOverrideCompatibility | ||
| assert self.name not in overiddenAddons, f"{self.name}, {overiddenAddons}" |
There was a problem hiding this comment.
The single assertion was an 'and' of two conditions. The two assertions replacing this are therefore an 'or'. Is this definitely what you meant?
There was a problem hiding this comment.
I don't think this is true, both assertions must pass and therefore it's an and? If one of these is false, the other assertion would still fail.
There was a problem hiding this comment.
Yep, you're right, sorry about that.
See test results for failed build of commit 054031a933 |
|
Curiosity: is there a reason why overridden is being spelled wrong in this code? :) (overidden)
|
Removes hack introduced in #15402 This removes the hack introduced for 2023.3 from 2024.1. The hack in 2023.3 specifically recognises older versions of the add-on as incompatible. The hack is not necessary with 2024.1, as the usual incompatibility warning applies when updating
…e disabled (#15444) Fixes #15440 Fixup of #15402 Summary of the issue: Some add-ons have version strings which we cannot parse for ordering. When checking if an add-on should be force disabled, we don't handle the case where the version string cannot be parsed. If a version string cannot be parsed, we should assume the add-on should not be force disabled, instead of throwing an error Description of user facing changes Add-ons which have unparsable version strings now work again Description of development approach If a version string cannot be parsed, we should assume the add-on should not be force disabled, instead of throwing an error
…r below (#15443) Follow-up of #15402 See #15402 (comment) Summary of the issue: As with Tony's Enhancements add-on, "NVDA global commands extension" add-on (name = NVDAExtensionGlobalPlugin) version 12.0.8 or below causes speech muted with WASAPI, so with NVDA 2023.3beta1. This add-on is not in NVDA store but is popular in French community as well as in international community; it is translated in 10 languages. A fix release (13.0) has already been created by the author @paulber19 on August 20. But we cannot ensure that people will have updated before updating NVDA. Description of user facing changes No problem when running NVDA with NVDAExtensionGlobalPlugin 12.0.8: NVDA is speaking. Description of development approach Added this add-on and its version in the list to force incompatibility.
Link to issue number:
None
Summary of the issue:
Tony's enhancements version 1.51 and older prevents any speech from happening when WASAPI is enabled.
This is due to the add-on relying on private API functions.
Now that WASAPI is enabled by default, upgrading to 2023.3 resulting in silence should be avoided if possible.
Description of user facing changes
When updating to NVDA 2023.3, if tony's enhancement add-on is installed it will trigger the typical UX warning for upgrading to an API breaking release, where incompatible add-ons are force disabled.
Description of development approach
Force known versions of problematic add-ons to be considered disabled with 2023.3.
This code should be cleaned up in 2024.1.
Testing strategy:
Known issues with pull request:
None
Change log entries:
Refer to diff
Code Review Checklist: