Use enum for speech reason#10703
Conversation
See test results for failed build of commit f9f04fb6f3 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@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 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. |
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.
4ebabcb to
91cb815
Compare
See test results for failed build of commit eef331ff60 |
|
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have created #10732 to do this follow up work.
There was a problem hiding this comment.
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.
|
Wow, nice idea. I once implemented something like a __getattr__ in the
controlTypes module, but this is way cleaner.
|
|
Hi, another option might be to try using module-level `getattr` (unless it is specific to Python 3.8). Thanks.
|
Link to issue number:
Stemmed from comment: #10593 (review)
Summary of the issue:
Speech
reasonwas 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
reasonconstants with an enum.Note there was one
reasonfound defined outside ofcontrolTypes,REASON_QUICKNAV = "quickNav"inBrowseMode.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
controlTypesChange log entry:
Changes for developers:
- speech 'reason' has been converted to an Enum, see controlTypes.OutputReason class.