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

Backwards compatible control types refactor #12510

Merged
merged 2 commits into from
Jun 30, 2021
Merged

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jun 3, 2021

Link to issue number:

Closes #10732

Summary of the issue:

controlTypes.py is a large file with many constants. To maintain backwards compatibility, the file length would increase to 1000+ LOC. It would be an improvement to split this code up, and group the constants into enums. As 2021.2 is not a compatibility breaking release, all changes must be backwards compatible.

Description of how this pull request fixes the issue:

This is a large refactor. The changes are split into individual commits for reviewers.

The first commit can be reviewed using the following tool

for file in $(ls source/controlTypes/*.py)
do
git diff master:source/controlTypes.py 8642581:$file > $file-controlTypes.diff
done

The second commit is generated using the following sed commands.

sed -i'' -bre "s/ROLE_([A-z0-9_]+) ?= ?/\t\1 = /g" source/controlTypes.py
sed -i'' -bre "s/ROLE_([A-z0-9_]+) ?:([^ ])/Role.\1: \2/g" source/controlTypes.py
sed -i'' -bre "s/ROLE_([A-z0-9_]+)/Role.\1/g" source/controlTypes.py

sed -i'' -bre "s/STATE_([A-z0-9_]+) ?= ?/\t\1 = /g" source/controlTypes.py
sed -i'' -bre "s/STATE_([A-z0-9_]+) ?:([^ ])/State.\1: \2/g" source/controlTypes.py
sed -i'' -bre "s/STATE_([A-z0-9_]+)/State.\1/g" source/controlTypes.py

The mixin strategy follows what is outlined for python 3.7 here. Mixins with Enums are fragile, and this code will have to be changed when updating (or downgrading python) Python.

Testing strategy:

Manual testing.

There is unit testing for the controlTypes module which should help ensure that compatibility doesn't break.

This module is fairly widely utilised, so any breaking changes are likely to be picked up quickly.

Known issues with pull request:

Changes can't be tracked backwards using git blame to controlTypes.py due to how the files are split up. As such, I've linted these files.

I haven't implemented the enums in the wider NVDA codebase. When we break the backwards compatibility, by updating to 2022.1, we will need to update the rest of the NVDA code beforehand (see #12549).

Change log entries:

For developers:

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@AppVeyorBot

This comment has been minimized.

@seanbudd seanbudd added the deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release label Jun 3, 2021
@seanbudd seanbudd marked this pull request as ready for review June 3, 2021 07:26
@seanbudd seanbudd requested a review from a team as a code owner June 3, 2021 07:26
@LeonarddeR
Copy link
Collaborator

This is an awesome change!

Could you consider the following while at it?

  1. Create an abstract class that defines the displayString property e.g. EnumWithTranslatableDisplayString
  2. Base the following classes on this abstract class: IsCurrent, Role and State
  3. Role and State's displayString should return the role/state label, respectively
  4. Instead of letting code use the role and state label dictionaries, use the displayString property instead.

source/controlTypes/role.py Outdated Show resolved Hide resolved
source/controlTypes/state.py Outdated Show resolved Hide resolved
@seanbudd

This comment has been minimized.

@LeonarddeR

This comment has been minimized.

@seanbudd

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

source/utils/mixins.py Outdated Show resolved Hide resolved
source/utils/mixins.py Outdated Show resolved Hide resolved
source/controlTypes/__init__.py Show resolved Hide resolved
@AppVeyorBot
Copy link

  • FAIL: System tests. See test results for more information.
  • Build (for testing PR)
  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.

See test results for failed build of commit aad9fadbb0

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

For now I'll assume your self review has ensured that the movement of code hasn't introduced changes. I'll first focus on the layout and introduced code.

As usual, with moved code, we'll merge rather than squash merge. Can you confirm that the commits are logical for that please. I can recommend using the the "autosquash" feature in git. It makes it easier to keep track of what while be the final commits while making updates to them, addressing review actions etc.

source/utils/mixins.py Outdated Show resolved Hide resolved
source/utils/mixins.py Outdated Show resolved Hide resolved
source/controlTypes/processing.py Outdated Show resolved Hide resolved
source/controlTypes/processing.py Outdated Show resolved Hide resolved
seanbudd added a commit that referenced this pull request Jun 25, 2021
@seanbudd seanbudd marked this pull request as ready for review June 25, 2021 05:03
seanbudd added a commit that referenced this pull request Jun 25, 2021
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

This is good, it's really close to being ready.

I have confirm there are only minor differences since the old HEAD 6ee01a53e
I have confirmed that the moved code still matches the old code from master, except for now using Enums and other minor changes. The code seems logically equivalent

I haven't yet confirmed everything previously exposed from controlTypes.py is exposed from controlTypes package.

source/controlTypes/__init__.py Outdated Show resolved Hide resolved
source/controlTypes/isCurrent.py Outdated Show resolved Hide resolved
devDocs/codingStandards.md Show resolved Hide resolved
source/controlTypes/__init__.py Outdated Show resolved Hide resolved
source/controlTypes/state.py Outdated Show resolved Hide resolved
source/controlTypes/role.py Outdated Show resolved Hide resolved
source/controlTypes/isCurrent.py Outdated Show resolved Hide resolved
@seanbudd seanbudd requested a review from feerrenrut June 30, 2021 04:23
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

I have confirmed (by comparing the files with git difftool origin/master:source/controlTypes.py HEAD:source/controlTypes/__init__.py ) that what is exposed from control types hasn't changed.

Another way to verify this may be to load use the NVDA python console:

  • Import control types
  • pprint(dir(controlTypes)).
  • Save this output for both before and after this change and compare with a diff tool.

The file controlTypes.py is already very complex and long, and the
code can easily be divided into 5 groups.
With the desire to group constants into Enums and maintain backwards
compatibility, which adds length and complexity, it is a canidate to be
split into submodules.

Actions:
- move controlTypes role data and functions to role.py
- move controlTypes state data and functions to state.py
- move isCurrent class to isCurrent.py
- move outputReason to outputReason.py
- move processing states functions to processAndLabelStates.py
seanbudd added a commit that referenced this pull request Jun 30, 2021
In order to improve the NVDA API and internal typing, Enums are created
for ROLE_* and STATE_* constants.

Changes:
- retain backwards compatibility until 2022.1 in controlTypes
- improve type information for controlTypes processing
- create a clickableRoles set in role.py for processAndLabelStates
- update docstrings to reflect new types
- lint controlTypes due to the mass changes
- deprecate helper functions for processAndLabelStates
- standardise members of the State enum from 0X[0-9]+ to 0x[0-9]+
- implement a displayStringEnumMixin
- deprecate roleLabels, stateLabels, negativeStateLabels
- update coding standards for enum usage and deprecations

Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
In order to improve the NVDA API and internal typing, Enums are created
for ROLE_* and STATE_* constants.

Changes:
- retain backwards compatibility until 2022.1 in controlTypes
- improve type information for controlTypes processing
- create a clickableRoles set in role.py for processAndLabelStates
- update docstrings to reflect new types
- lint controlTypes due to the mass changes
- deprecate helper functions for processAndLabelStates
- standardise members of the State enum from 0X[0-9]+ to 0x[0-9]+
- implement a displayStringEnumMixin
- deprecate roleLabels, stateLabels, negativeStateLabels
- update coding standards for enum usage and deprecations

Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
@AppVeyorBot

This comment has been minimized.

@seanbudd
Copy link
Member Author

I've updated the deprecations.md file to reflect your verification step, and changes.t2t

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR)

See test results for failed build of commit 721241b832

@seanbudd seanbudd merged commit 90c08ef into master Jun 30, 2021
@seanbudd seanbudd deleted the controlTypes-refactor branch June 30, 2021 10:08
@nvaccessAuto nvaccessAuto added this to the 2021.2 milestone Jun 30, 2021
aaclause added a commit to aaclause/BrailleExtender that referenced this pull request Oct 26, 2021
See nvaccess/nvda#12510, nvaccess/nvda#11856 and nvaccess/nvda#12636

According to the NVDA change log:

> - `characterProcessing.SYMLVL_*` constants should be replaced using their equivalent `SymbolLevel.*` before 2022.1.
> - controlTypes has been split up into various submodules, symbols marked for deprecation must be replaced before 2022.1.
@seanbudd seanbudd mentioned this pull request Feb 22, 2022
5 tasks
seanbudd added a commit that referenced this pull request Feb 24, 2022
Remove code deprecated in #12510

Summary of the issue:
Deprecated code needed to be removed from the API proposed in #12510

Description of how this pull request fixes the issue:
Commits to the API proposed in #12510
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert control types constants into enums
7 participants