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

Use enum for speech reason #10703

Merged
merged 3 commits into from Jan 30, 2020
Merged

Use enum for speech reason #10703

merged 3 commits into from Jan 30, 2020

Conversation

feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Jan 20, 2020

Link to issue number:

Stemmed from comment: #10593 (review)

Summary of the issue:

Speech reason was a string, this made it hard to know what the "allowed" values are and restrict values to domains that make sense.

Description of how this pull request fixes the issue:

Replace speech reason constants with an enum.
Note there was one reason found defined outside of controlTypes, REASON_QUICKNAV = "quickNav" in BrowseMode.py. It's possible, but unlikely that there are others.
Should not break compatibility, as long as addons are using the constants for reason defined in controlTypes.

Testing performed:

Ran from source, attempting to exercise as many speech "reason" paths as possible.

Known issues with pull request:

If addons or other code are using string literals for the reason argument, they will break. This is considered low risk, since it's expected that they are using the constants in controlTypes

Change log entry:

Changes for developers:
- speech 'reason' has been converted to an Enum, see controlTypes.OutputReason class.

@feerrenrut feerrenrut added this to In progress in [Project] Speech Refactor via automation Jan 20, 2020
@AppVeyorBot
Copy link

AppVeyorBot commented Jan 20, 2020

See test results for failed build of commit f9f04fb6f3

@leonardder

This comment has been minimized.

@codeofdusk

This comment has been minimized.

@feerrenrut
Copy link
Member Author

feerrenrut commented Jan 23, 2020

@leonardder and @codeofdusk I have updated the description, this PR should not cause backwards incompatibility as long as addons are using the constants defined in controlTypes. If they instead use string literals, that will cause a problem. I expect this risk is minor.

This will target 2020.1. Generally we attempt to minimize the risk of incompatibility. This PR maintains the old constant names, which will be removed at "some point" in the future. In case something is overlooked which may affect addons, addon authors should be regularly testing, staying up to date with changes happening in NVDA, and letting us know of any issues being created for them. Communication is key.

@feerrenrut feerrenrut requested a review from michaelDCurran Jan 28, 2020
feerrenrut added 2 commits Jan 29, 2020
This changes the type of all REASON_* constants.
One outcome of this is that they no longer have equality with
their old string values e.g. REASON_FOCUS == "focus" >> False

This would also be the case if the enum used string values instead
of int values.

All occurrences of the previous string values were inspected, and with a
reasonable degree of certainty, there seemed not to be used as REASON_*
values.
Browsemode introduces a new value, this is now added to the enum.
@AppVeyorBot
Copy link

AppVeyorBot commented Jan 29, 2020

See test results for failed build of commit eef331ff60

@feerrenrut feerrenrut marked this pull request as ready for review Jan 29, 2020
@feerrenrut
Copy link
Member Author

feerrenrut commented Jan 29, 2020

I have rebased this on top of the speech command splitting work in #10593. It is now ready for review.

# In future, OutputReason should be used directly


REASON_FOCUS = OutputReason.FOCUS
Copy link
Member

@michaelDCurran michaelDCurran Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there are several other places we should use enums in future (textInfos.position, textInfos.unit, controlTypes.role, controlTypes.state), I'd like to see the exploding of the enum values into the outer scope here be more generic rather than explicitly listing each one, as it would be very easy to forget one.
Something like:

for r in OutputReason:
	globals()['REASON_%s' % r.name] = r

Copy link
Member Author

@feerrenrut feerrenrut Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I don't like about doing this, is that it means new constants also become available automatically at this scope. Forgetting to update this "exploded enum" when adding new constants is a feature in my mind. I agree it means when creating the enum and exploded values we have to be careful to double check that they are all available, but just like the quickNav value it is easy to miss one because it is defined in another file. I want to deprecate the constants at this scope and remove them in 2021.1

Copy link
Member

@michaelDCurran michaelDCurran Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I'm not sure how I'd feel about a new value being added to the enum in the future and not being then duplicated at module level, unless we go through the code and remove all model-level references to the values. I think defining some differently is extremely confusing. I certainly agree that values for new enums should never be duplicated, but pre-existing ones should either be duplicated and or all module-level references removed.
However, I'll approve this as is, and you can make the final decision.

Copy link
Member Author

@feerrenrut feerrenrut Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this stemmed from the speech function refactor I only did "the minimal". But I think we should follow up with another PR to do the same for the rest of the constants in control types, and ensure that all of NVDA uses the enum versions and officially deprecate (and give a date of removal for) the module level constants.

Copy link
Member Author

@feerrenrut feerrenrut Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created #10732 to do this follow up work.

Copy link
Member Author

@feerrenrut feerrenrut Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also created a new label deprecated/2021.1 to keep track of deprecations and their removal date. This should make it easier to give a concrete set of "breaking changes" in the 2021.1 release.

@leonardder
Copy link
Collaborator

leonardder commented Jan 30, 2020

@josephsl
Copy link
Collaborator

josephsl commented Jan 30, 2020

@feerrenrut feerrenrut added component/speech deprecated/2021.1 labels Jan 30, 2020
@feerrenrut feerrenrut merged commit d1fe2eb into master Jan 30, 2020
1 check passed
[Project] Speech Refactor automation moved this from In progress to Done Jan 30, 2020
@feerrenrut feerrenrut deleted the useEnumForSpeechReason branch Jan 30, 2020
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jan 30, 2020
feerrenrut added a commit that referenced this issue Jan 30, 2020
@feerrenrut feerrenrut modified the milestones: 2019.3, 2020.1 Jan 30, 2020
@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
component/speech deprecated/2021.1
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants