Skip to content

Conversation

@LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Sep 12, 2023

Link to issue number:

Closes #15262
Blocked by #15271

Summary of the issue:

When implementing a synthesizer driver, there are two ways to change pitch:

  1. Change the pitch setting
  2. Send a pitch command that changes the pitch for the duration of the sequence or until there is a new pitch command.

Capital pitch change uses pitch commands to change pitch for capital letters. When doing this, it calls isSupported("pitch") on the driver, which returns True when the pitch setting is supported. However in order to support capital pitch changes, the driver should implement support for the PitchCommand instead.

Description of user facing changes

None

Description of development approach

Changed all situations where isSupported("pitch") is called to generate a pitch command to check whether the PitchCommand is in supportedCommands.

Testing strategy:

Tested that capital pitch changes are still working, at least with one core.

Known issues with pull request:

This can be considered API breaking, because capital pitch changes still worked when a driver didn't offer the pitch command. However, the synthDriver class docs explicitly state that supported commands should be implemented.

Change log entries:

For Developers

  • Synthesizers that need to support capital pitch changes must now explicitly declare their support for the PitchCommand in the supportedCommands attribute on the driver for pitch changes to work.

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
  • Security precautions taken.

@LeonarddeR LeonarddeR marked this pull request as ready for review September 12, 2023 15:38
@LeonarddeR LeonarddeR requested a review from a team as a code owner September 12, 2023 15:38
@LeonarddeR LeonarddeR requested a review from seanbudd September 12, 2023 15:38
@LeonarddeR LeonarddeR added this to the 2024.1 milestone Sep 12, 2023
@seanbudd
Copy link
Member

seanbudd commented Sep 22, 2023

Confirming, can this be merged now that #15271 has been?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Sep 22, 2023 via email

@seanbudd seanbudd merged commit 69c8c5b into nvaccess:master Sep 22, 2023
@LeonarddeR LeonarddeR deleted the supportedCommands branch August 23, 2025 06:28
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.

Capital pitch change depends on pitch setting, should check for PitchCommand in supportedCommands instead

2 participants