Skip to content

Encapsulate NVDA state write paths #15021

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

Merged
merged 5 commits into from
Jun 20, 2023
Merged

Encapsulate NVDA state write paths #15021

merged 5 commits into from
Jun 20, 2023

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jun 19, 2023

Link to issue number:

None

Summary of the issue:

NVDA uses a variety of paths to write to.
These can be hard to discover or track in the code, as write paths are not stored in globally available named symbols.
For example, if a developer wishes to find code instances where NVDA writes to the profile config directory, searching "profile" will yield additional results, e.g. code comments.

Description of user facing changes

N/A

Description of development approach

Created a centralised class to list all available write paths in NVDA.
This improves discoverability of write paths.

Testing strategy:

Ran NVDA, tested saving some config options.

Known issues with pull request:

None

Change log entries:

For Developers

``speechDictHandler.speechDictVars.speechDictsPath`` is deprecated, instead use ``WritePaths.speechDictsDir``

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@seanbudd seanbudd requested a review from a team as a code owner June 19, 2023 01:54
@seanbudd seanbudd requested a review from michaelDCurran June 19, 2023 01:54
@seanbudd seanbudd merged commit 9e8d425 into master Jun 20, 2023
@seanbudd seanbudd deleted the createCentralWritePaths branch June 20, 2023 00:17
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jun 20, 2023
@ultrasound1372
Copy link

ultrasound1372 commented Jun 20, 2023

I have a known issue to this, do I submit it here or create a new issue? This is an API breaking change that unexpectedly broke some add-ons available, I think, through the store. At least one of them.

@seanbudd
Copy link
Member Author

Please create a new issue - thanks

@ruifontes
Copy link
Contributor

@ultrasound1372 , if you are talking about applicationDictionary add-on, I have already noticed the problem and we are going to fix that still this week...

@ultrasound1372
Copy link

That and enhanced dictionaries. Looking at the code it looks like there is supposed to be an alias and deprecation warning but it doesn't seem to work.

@ruifontes
Copy link
Contributor

@ultrasound1372 , it works, but the information given by @seanbudd is missleading...
You have to use:
speechDictHandler.WritePaths.speechDictsDir
and not only WritePaths.speechDictsDir...

@ruifontes
Copy link
Contributor

@seanbudd , if this broke, at least two add-ons, it should not be done in 2024.1?
Normally, it should remain as a deprecation untill 2024.1, but in the new Alphas the old way do not work at all...

@seanbudd
Copy link
Member Author

As mentioned earlier - please open an issue so we can track this.

Note, that removing relative imports is not an API breaking change. API consumers should import symbols from the original place.

For example, WritePaths should be imported from NVDAState, not speechDictHandler. speechDictsDir should have originally been imported from speechDictHandler.speechDictVars not speechDictHandler directly. The alias and deprecation warning should work correctly if the add-on's original import was from the correct module.

@seanbudd seanbudd mentioned this pull request Jun 23, 2023
6 tasks
seanbudd added a commit that referenced this pull request Jun 27, 2023
Follow up of #15021

Summary of the issue:
The deprecation was incorrectly documented, missing the module name and referring to NVDAState.WritePaths.speechDictsDir instead of WritePaths.speechDictsDir.

The change also caused some issues for add-ons who were relying on the deprecated code being imported into a different module.

Description of user facing changes
N/A

Description of development approach
For ease of use, compatibility for the imported code has been retained, despite not being an API breaking change.

Documentation of the deprecation has been fixed.
seanbudd pushed a commit that referenced this pull request Oct 15, 2023
Closes #15614

Summary of the issue:
speechDictHandler.speechDictVars was marked as deprecated in PR #15021. Unfortunately since the module was not imported anywhere in core py2exe failed to bundle it, which made it impossible to import this code in binary versions of NVDA. Additionally warning which was raised when importing speechDictHandler.speechDictVars was problematic when building developer documentation. Since this API breakage was already included in two releases of NVDA, it was decided to remove the code and document the removal.

Description of user facing changes
None - this only removes deprecated code.

Description of development approach
The deprecated code is removed. Change log is updated to document the removal for add-ons developers.
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

Successfully merging this pull request may close these issues.

5 participants