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

create Enums for audioDucking and minor lint #12926

Merged
merged 6 commits into from
Oct 20, 2021
Merged

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Oct 13, 2021

Link to issue number:

None

Summary of the issue:

While debugging #12913, I created some Enums to help with logging and understanding the code.
This is not a focused refactor, and as such, more work could be done to improve the audioDucking API.

Description of how this pull request fixes the issue:

  • Implements IntEnums for constants
  • Add some comments
  • Clean up imports

Testing strategy:

with a try build of this PR:

  • Test the audio ducking menu
  • Test the input gesture(s) to trigger audioducking
  • Test audio ducking as a feature

Known issues with pull request:

Note, this is an API breaking change and should not be merged until 2022.1.
I have just created this PR as the work should not be lost, but adding backwards compatibility is not worth prioritising.

Change log entries:

See diff in changes.t2t

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

@seanbudd seanbudd added this to the 2022.1 milestone Oct 13, 2021
@seanbudd seanbudd marked this pull request as ready for review October 19, 2021 22:55
@seanbudd seanbudd requested a review from a team as a code owner October 19, 2021 22:55
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

Looks good other than the query re _displayStringLabels

self.duckingList = settingsSizerHelper.addLabeledControl(
duckingListLabelText,
wx.Choice,
choices=audioDucking.AudioDuckingMode._displayStringLabels.values()
Copy link
Member

Choose a reason for hiding this comment

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

does this really work?
I think that _displayStringLabels is an instance property, and therefore can not be accessed on the class, rather it can only be accessed on the instance (one of the enum values).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this doesn't work. I've tested this PR using a try build now, and fixed how the choices are generated

@seanbudd seanbudd merged commit 0c09fff into master Oct 20, 2021
@seanbudd seanbudd deleted the audioducking-refactor branch October 20, 2021 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants