Implement an extension point deprecation strategy #17644
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SaschaCowley
added
the
conceptApproved
SaschaCowley
requested changes
Feb 4, 2025
| @@ -115,6 +123,11 @@ def register(self, handler: HandlerT): | |||
| sig = inspect.signature(handler) | |||
| if sig.parameters and list(sig.parameters)[0] == "self": | |||
| raise TypeError("Registering unbound instance methods not supported.") | |||
| if self._deprecationMessage: | |||
| if NVDAState._allowDeprecatedAPI(): | |||
| log.warning(self._deprecationMessage, stack_info=True) | |||
There was a problem hiding this comment.
can we use warnings.warn here instead?
Suggested change
| log.warning(self._deprecationMessage, stack_info=True) | |
| warnings.warn(self._deprecationMessage, DeprecationWarning) |
There was a problem hiding this comment.
@SaschaCowley, for deprecation we usually use log.warning in NVDA code base.
Here, you suggest to use warnings.warn instead. Is this suggestion applicable to this specific case only or to all deprecation codes in NVDA? If the latter, I'd suggest to discuss this and handle this in a specific issue / PR. If you suggest to use warnings.warn in the current specific case, could you explain why it differs from other deprecation code in NVDA?
Without any other explanation, I prefer to stick to the current use of log.warning using stack_info=True kwarg, since this give an indication to add-on authors where the deprecated code needs to be modified.
There was a problem hiding this comment.
My preference would be to switch to using warnings.warn(msg, DeprecationWarning) everywhere. That would allow us the ability to further customise the way deprecation warnings are shown in future - for instance, we could make unit tests fail on deprecation warnings. Note that we can fairly easily add traceback information to warnings, though we don't currently do so when we log them. We could also potentially add a helper method to automatically call warnings.warn with category=DebugWarning and stacklevel=1 (to avoid warning in the deprecated method itself), and to exclude non core files when we update to python>=3.12 with the skip_file_prefixes argument. @seanbudd what are your thoughts? Is this worth opening an issue/pr about? Is it worth doing here?
| filter_displaySize = extensionPoints.Filter[int]() | ||
| filter_displaySize = extensionPoints.Filter[int]( | ||
| _deprecationMessage="braille.filter_displaySize is deprecated. Use braille.filter_displayDimensions instead.", | ||
| ) | ||
| """ |
There was a problem hiding this comment.
Please add a comment to the docstring mentioning that filter_displaySize is deprecated.
There was a problem hiding this comment.
There was a deprecaation notice at the bottom of the doc string, I moved it to the top.
Co-authored-by: Sascha Cowley <[email protected]>
Fixes nvaccess#17516 Summary of the issue: After the move to exclusively Windows core audio APIs, the SAPI4 driver stopped working. Description of user facing changes The SAPI4 driver works again. A warning is shown the first time the user uses SAPI4 informing them that it is deprecated. Description of development approach Implemented a function to translate between MMDevice Endpoint IDs and WaveOut device IDs, based on <https://learn.microsoft.com/en-gb/windows/win32/coreaudio/device-roles-for-legacy-windows-multimedia-applications>. Added a config key, `speech.hasSapi4WarningBeenShown`, which defaults to False. Added a synthChanged callback that shows a dialog when the synth is set to SAPI4 if this config key is False and this is not a fallback synthesizer. Testing strategy: Ran NVDA, from source and installed, and on the user desktop, in secure mode, and on secure desktops, and used it with SAPI4. Changed the audio output device to ensure audio was routed as expected. Known issues with pull request: When first updating to a version with this PR merged, if the user uses SAPI4 as their primary speech synth, they will be warned about its deprecation in the launcher and when they first start the newly updated NVDA. This is unavoidable as we don't save config from the launcher. The dialog is only shown once per config profile, so may be missed by some users. Other options I have considered include: * Making this a nag dialog that appears, say, once a week or once a month. * Also making a dialog appear whenever the user manually sets their synth to SAPI4. * Adding a new dialog in 2025.4 (or the last release before 2026.1) that warns users that this will be the last release to support SAPI4. * Adding a dialog when updating to 2026.1 that warns users that they will no longer be able to use SAPI4. * Adding a Windows toast notification that appears every time NVDA starts with SAPI4 as the synth. The warning dialog is shown after SAPI4 is loaded. In the instance that the user is already using SAPI4, this is correct behaviour. In the case of switching to SAPI4, perhaps a dialog should appear before we terminate the current synth and initialise SAPI4.
) Fixes nvaccess#17637 Follow-up to nvaccess#17505 Summary of the issue: The config upgrade steps for moving from version 14 to 15 of the config schema may leave the user's config in an invalid state if either of `["keyboard"]["speakTypedCharacters"]` or `["keyboard"]["speakTypedWords"]` is not a boolean. This is because the upgrade steps, as implemented, take no action if those values are not bools. For most upgrade steps, this approach is perfectly valid. However, version 15 of the schema expects these values to be integers, so validation of the upgraded config will fail. Description of user facing changes The configuration upgrade should not corrupt the user's config. Description of development approach Instead of just `return`ing when we get a `ValueError` in `upgradeConfigFrom_14_to_15`, delete the offending config entry. Testing strategy: Tested by creating valid and invalid config strings, instantiating `ConfigObj`s with them, and feeding those to `upgradeConfigFrom_14_to_15`. Known issues with pull request: A user may lose their setting for "Speak typed characters" or "Speak typed words", if configobj doesn't recognise it as a boolean. Theoretically this shouldn't be possible, as the config wouldn't have validated under any of the prior schema versions. Nevertheless this seems to have happened, and it is better to lose (at most) 2 settings than all settings.
Fixes nvaccess#17502 Summary of the issue: In nvaccess#15386, support was added for numRows and numCols on braille displays. Setting numCells explicitly would also set numCols. In nvaccess#17011, we're much more relying on numCols and numRows. For the eco braille and Lilli drivers, numCells was implemented by overriding _get_numCells. However this way, the setter of numCells would never be called and therefore numCols would always be 0. Description of user facing changes Fix braille for Eco Braille and Lilli displays. Description of development approach When calculating display dimensions on the braille handler, when display.numRows is 1, use display.numCells instead of display.numCols. This accounts for cases where numCells is implemented and numRows/numCols are not. Worked around the ZeroDevisionError when getting windowStartPost on a buffer Initialize _windowRowBufferOffsets to [(0, 0)] instead of [(0, 1)]. The latter suggests that the window contains one cell, which is not the case. @michaelDCurran you might want to check this.
Soft-integration of new Appveyor webhook which should run after the normal build on zebi in a non-blocking fashion.
Try using the deploy webhook the AppVeyor way; we should be able to pass those variable in the environment instead of setting up a custom deploy pipeline. Using the native way allows deploys to be retried via the UI, which is very helpful. https://www.appveyor.com/docs/deployment/webhook/
Fixes nvaccess#17658 Summary of the issue: There is currently no way to pass information between message dialogs and their callbacks. This reduces the functionality available to callbacks in future. Description of user facing changes None. Description of development approach Add a `Payload` dataclass to `gui.message`. It currently has no fields, though these can be added without breaking the API in future, for example to implement nvaccess#17646 . In `MessageDialog._executeCommand`, if a command has a callback, instantiate a `Payload` and pass it to the callback when calling it. Update the few uses of the message dialog API in NVDA which use callbacks: * `synthDrivers.sapi4._sapi4DeprecationWarning` Updated the developer guide to note the new requirements for callback functions. Testing strategy: Ran NVDA from source, and triggered the affected dialogs. Ensured they worked as expected. Ran from source, with `versionInfo.updateVersionType` set to `"snapshot:alpha`, and checked that updating worked. Built and installed this branch, then ran the CRFT, cancelling and continuing, and ensured that no issues occured. Known issues with pull request: None known.
Closes nvaccess#13333 Summary of the issue: There are two unmaintained files, which are translated versions of the developer guide. These have been unmaintained for over a decade. Description of user facing changes None Description of development approach Remove files
|
@LeonarddeR looks like something's gone wrong when merging in master |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Follow up of #17011
Slightly related to #17635, as this oversight was found there
Summary of the issue:
In #17011,
braille.filter_displaySizewas deprecated, but there was not yet a mechanism in place that logs warnings about usage of the deprecated extension point.Description of user facing changes
Warnings in the log when
braille.filter_displaySize.registeris called.Description of development approach
filter_displayDimensionsfilter_displayDimensionsTesting strategy:
Tested that a warning is logged when registering
Known issues with pull request:
None known
Code Review Checklist:
@coderabbitai summary