Skip to content

Add pre_filter_speechSequence and post_filter_speechSequence extension point Actions#17429

Closed
beqabeqa473 wants to merge 1 commit into
nvaccess:masterfrom
beqabeqa473:speech_viewer_use_filter_speechSequence
Closed

Add pre_filter_speechSequence and post_filter_speechSequence extension point Actions#17429
beqabeqa473 wants to merge 1 commit into
nvaccess:masterfrom
beqabeqa473:speech_viewer_use_filter_speechSequence

Conversation

@beqabeqa473
Copy link
Copy Markdown
Contributor

@beqabeqa473 beqabeqa473 commented Nov 23, 2024

Link to issue number:

Superseeds #16213

Summary of the issue:

Add pre_filter_speechSequence and post_filter_speechSequence extensionPoint Actions, inject pre_filter_speechSequence just before filter in speech.speak and use post_filter_speechSequence in speechViewer as @LeonarddeR stated in #14520

Description of user facing changes

Nothing changed from user's point of view

Description of development approach

Removed code responsible for appending speechSequence to speech_viewer from speech.speak and registered appendSpeechSequence as extensionPoint handler function.

Testing strategy:

Manually tested that speech is still visible on the screen when activating speech_viewer

Known issues with pull request:

None at this 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.

@coderabbitai summary

…nPoint Actions, inject pre_filter_speechSequence just before filter in speech.speak and use post_filter_speechSequence in speechViewer as @LeonarddeR stated in nvaccess#14520
@beqabeqa473 beqabeqa473 requested a review from a team as a code owner November 23, 2024 17:51
@beqabeqa473 beqabeqa473 requested a review from seanbudd November 23, 2024 17:51
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Nov 26, 2024
Copy link
Copy Markdown
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Closing in favour of #17428

It doesn't seem that this PR adds any new extension points as requested in #14520

filter_speechSequence has already been added.

post_filter_speechSequence has already been added as pre_speech

pre_filter_speechSequence doesn't appear to be required/requested for any use case. Please correct me if I am wrong here

Comment on lines +1385 to +1387
| ``Action`` | ``pre_filter_speechSequence`` | Notifies before speech sequence filters are processed. |
| ``Action`` | ``post_filter_speechSequence`` | Notifies after a speech sequence has optionally been filtered by NVDA components and/or add-ons. |
| ``Filter`` | ``filter_speechSequence`` | Allows components or add-ons to filter a speech sequence before it is passed to the Synth driver. |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

these should be single backticks, not doubled, please fix the formatting

|`Filter` |`filter_speechSequence` |Allows components or add-ons to filter speech sequence before it passes to the synth driver.|
| ``Action`` | ``pre_filter_speechSequence`` | Notifies before speech sequence filters are processed. |
| ``Action`` | ``post_filter_speechSequence`` | Notifies after a speech sequence has optionally been filtered by NVDA components and/or add-ons. |
| ``Filter`` | ``filter_speechSequence`` | Allows components or add-ons to filter a speech sequence before it is passed to the Synth driver. |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
| ``Filter`` | ``filter_speechSequence`` | Allows components or add-ons to filter a speech sequence before it is passed to the Synth driver. |
| `Filter` | `filter_speechSequence` | Allows components or add-ons to filter a speech sequence before it is passed to the synth driver. |

Comment thread source/speech/speech.py
Comment on lines +1090 to 1091
post_filter_speechSequence.notify(sequence=speechSequence)
pre_speech.notify(speechSequence=speechSequence, symbolLevel=symbolLevel, priority=priority)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

since these are called at the same point in time, aren't they semantically the same?
I don't think we need post_filter_speechSequence any more, pre_speech is the same concept.

Comment thread user_docs/en/changes.md
* Added the [VS Code workspace configuration for NVDA](https://nvaccess.org/nvaccess/vscode-nvda) as a git submodule. (#17003)
* In the `brailleTables` module, a `getDefaultTableForCurrentLang` function has been added (#17222, @nvdaes)
* Added the following extension points:
* ``speech.filter_speechSequence``. (#16191, @beqabeqa473)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this has already been added no? this PR doesn't add it.

@seanbudd seanbudd closed this Nov 27, 2024
@beqabeqa473
Copy link
Copy Markdown
Contributor Author

I am sure NVDA remote should get speech before it will be filterred. Because controller side may not know what addons controlled side has, and how speech will be changed.
@LeonarddeR @ctoth what do you think?

@LeonarddeR
Copy link
Copy Markdown
Collaborator

I don't think so actually. In my view, the speech NVDA Remote receives should be equal to the speech that is spoken at the controlled machine. So if the controlled system filters speech, the controller should speak the filtered speech.
With regard to NVDA Remote however, there is some inconsistency here and there. Some parts of the speech spoken at the controller are demanded by the controller (synth, rate, pitch, etc.) whereas some other parts are demanded by the controlled (puncuation level, automatic language switching). I think more uniformity would be helpful.

@beqabeqa473
Copy link
Copy Markdown
Contributor Author

yes, punctuation and autolanguage switching should not be sent to controller side. if we take remote assistancee from sight persons point of view, they can see punctuations, but here we are listening, and our synthesizer may not able to read what controlled side is sending. Personaly i am mostly surviving because of that.

Comment thread source/speechViewer.py
def onDestroy(self, evt: wx.Event):
self._isDestroyed = True
post_sessionLockStateChanged.unregister(self.onSessionLockStateChange)
post_filter_speechSequence.unregister(appendSpeechSequence)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you open a new PR that uses pre_speech in speechViewer?

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.

3 participants