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

Convert NVDA's code base to use output reasons from enum in controlTypes and remove deprecated REASON_* module level constants #11969

Merged
merged 5 commits into from
Jan 26, 2021

Conversation

lukaszgo1
Copy link
Contributor

Link to issue number:

Discussion in #10703 and #10732

Summary of the issue:

PR #10703 introduced new OutputReason class which should be used in place of previously used REASON_* constants from the controlTypes module. To provide backward compatibility old module level constants were kept in place, but marked as deprecated. Since 2021.1 intents to break backwards compatibility anyway it makes sense to use output reasons from the new enum throughout the code and remove these deprecated constants.

Description of how this pull request fixes the issue:

This pr converts all usages of REASON_* constants to the new enum (git grep was used to look for all occurrences of them) and removes old constants from the controlTypes and browseMode modules.

Testing performed:

  • Ensured that unit tests still passes.
  • Used NVDA for around a day with these changes without experiencing any regressions in functionality.
  • With git grep ensured that only mentions of REASON_ in the code is in the changelog file

Known issues with pull request:

I'd be great to merge this soon not only to give it a good amount of testing on Alpha, but it touches many files and therefore is quite prone to conflicts.

Change log entry:

Changes for developers:

  • Module level REASON_* constants are removed from controlTypes - please use controlTypes.OutputReason instead.
  • REASON_QUICKNAV has been removed from browseMode - use controlTypes.OutputReason.QUICKNAV instead

@lukaszgo1
Copy link
Contributor Author

@feerrenrut As I've predicted in the PR description due to the amount of modified files this had conflicts already. Would it be possible to review this to avoid multiple back and forth of fixing merge conflicts here?

@feerrenrut feerrenrut added the deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release label Jan 14, 2021
@feerrenrut
Copy link
Contributor

feerrenrut commented Jan 14, 2021

Ok, I'll get #11782 delivered first , then I'll come and review this. To speed up the review, please go over the approach in #11782 to ensure the approach here is consistent.

@feerrenrut

This comment has been minimized.

@lukaszgo1
Copy link
Contributor Author

@feerrenrut This PR merely starts using enum added in #10703 so I believe consistency should not be an issue. In deed OutputReason matches style used in #11782

@michaelDCurran michaelDCurran merged commit 856adec into nvaccess:master Jan 26, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Jan 26, 2021
@lukaszgo1 lukaszgo1 deleted the useSpeechReasonsFromEnum branch January 26, 2021 23:12
CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Jan 27, 2021
AAClause added a commit to AAClause/BrailleExtender that referenced this pull request Jan 29, 2021
AAClause added a commit to AAClause/BrailleExtender that referenced this pull request Jan 29, 2021
michaelDCurran pushed a commit that referenced this pull request Jan 31, 2021
AAClause added a commit to AAClause/BrailleExtender that referenced this pull request Feb 7, 2021
@seanbudd seanbudd mentioned this pull request Mar 5, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants