Skip to content

Add speechSequence filter extension point to speech.speak method#16191

Merged
seanbudd merged 5 commits into
nvaccess:masterfrom
beqabeqa473:filter_speech_extension_point
Feb 21, 2024
Merged

Add speechSequence filter extension point to speech.speak method#16191
seanbudd merged 5 commits into
nvaccess:masterfrom
beqabeqa473:filter_speech_extension_point

Conversation

@beqabeqa473
Copy link
Copy Markdown
Contributor

@beqabeqa473 beqabeqa473 commented Feb 18, 2024

Link to issue number:

Fixes #14520

Summary of the issue:

Added a filterSpeechSequence to speech.speak method to make it possible for addons to filter speech in better way

Description of user facing changes

None

Description of development approach

Added filter extension point in speech.extensions and injected it in the beginning of speech.speak

Testing strategy:

Tested with a plugin and confirmed that it is possible to filter and return filtered speech.

Known issues with pull request:

None at the moment

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

Additional notes:

@LeonarddeR I understand, that to make it ideal, some things should be refactored, but can it be considered for potential merging now to eliminate patching speech.speak by numerous addons?

@CyrilleB79
Copy link
Copy Markdown
Contributor

I have no doubt that this extension point would be useful.

Though, could you give one or more concrete use case to illustrate the need of this PR?

@beqabeqa473
Copy link
Copy Markdown
Contributor Author

It would be mutch easier to modify speech sequence with filter rather than override speech.speak method in addons.

@CyrilleB79
Copy link
Copy Markdown
Contributor

@beqabeqa473, sorry, it's not the answer I was expecting.

To ask it in another way:
Have you in mind some specific add-ons which would benefit of this feature? If yes, please indicate which ones? Thanks.

@beqabeqa473
Copy link
Copy Markdown
Contributor Author

beqabeqa473 commented Feb 18, 2024 via email

@ruifontes
Copy link
Copy Markdown
Contributor

And some more: NVDARecord, NVDA Global extension...

@cary-rowen
Copy link
Copy Markdown
Contributor

There are also add-ons such as openLinkWith, SearchWith, ClipboardEnhancement, etc. that also rely on this feature.

@CyrilleB79
Copy link
Copy Markdown
Contributor

Thanks to all for these examples:

@beqabeqa473 you write:

They all are patching speech.speak method to get or modify speech sequence.

Actually, when they just want to get the speech sequence but not modify them (e.g. Instant Translate, Speech History), an Action extension point would be enough.

But I guess it's not needed to implement an Action extension point at the same place as a Filter extension point since a Filter extension point can also be used to trigger an action without modifying the speech sequence.

Just mentioning here to check if NV Access agrees with such way to define extension points.

@LeonarddeR
Copy link
Copy Markdown
Collaborator

But I guess it's not needed to implement an Action extension point at the same place as a Filter extension point since a Filter extension point can also be used to trigger an action without modifying the speech sequence.

That's exactly why I suggested this approach earlier.

Copy link
Copy Markdown
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

Please also update the dev guide

Comment thread source/speech/extensions.py Outdated
@beqabeqa473
Copy link
Copy Markdown
Contributor Author

But I guess it's not needed to implement an Action extension point at the same place as a Filter extension point since a Filter extension point can also be used to trigger an action without modifying the speech sequence.

That's exactly why I suggested this approach earlier.

I also don't think Action extension point is needed if there will be a filter, where we just can get a sequence and return it as is.

@beqabeqa473
Copy link
Copy Markdown
Contributor Author

@LeonarddeR Thanks, done.

@XLTechie
Copy link
Copy Markdown
Collaborator

XLTechie commented Feb 19, 2024 via email

@beqabeqa473 beqabeqa473 marked this pull request as ready for review February 19, 2024 11:48
@beqabeqa473 beqabeqa473 requested a review from a team as a code owner February 19, 2024 11:48
@beqabeqa473 beqabeqa473 requested a review from seanbudd February 19, 2024 11:48
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Feb 19, 2024
Comment thread source/speech/extensions.py Outdated
Comment thread source/speech/extensions.py Outdated
@seanbudd seanbudd marked this pull request as draft February 20, 2024 05:33
@beqabeqa473 beqabeqa473 marked this pull request as ready for review February 21, 2024 06:18
Comment thread projectDocs/dev/developerGuide/developerGuide.t2t Outdated
Comment thread source/speech/extensions.py
@seanbudd
Copy link
Copy Markdown
Member

In future please put the word "fixes" before the issue number in the PR description. This ensures the issue is automatically linked to the PR.

@seanbudd
Copy link
Copy Markdown
Member

please also tick items in the code review checklist. Without these items ticked it is assumed that testing hasn't happened, the PR description is out of date, etc

Comment thread source/speech/extensions.py Outdated
@seanbudd seanbudd merged commit 66f0161 into nvaccess:master Feb 21, 2024
@nvaccessAuto nvaccessAuto added this to the 2024.2 milestone Feb 21, 2024
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
…ccess#16191)

Fixes nvaccess#14520

Summary of the issue:
Added a filterSpeechSequence to speech.speak method to make it possible for addons to filter speech in better way

Description of user facing changes
None

Description of development approach
Added filter extension point in speech.extensions and injected it in the beginning of speech.speak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add extension points for speech to avoid NVDA Remote monkeypatching

8 participants