Add pre_filter_speechSequence and post_filter_speechSequence Action and use post_filter_speechSequence in speechViewer

[beqabeqa473]

Link to issue number:

None

Summary of the issue:

Add speechSequencePreFilter extensionPoint Action, inject it just before filter in speech.speak and use it in speechViewer as @LeonarddeR stated in #14520

Description of user facing changes

Nothing changed

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.

[seanbudd]

I don't think using a filter function is appropriate in this case. appendSpeechSequence is an action, it doesn't filter the speech sequence.

[seanbudd]

SpeechViewer should also be outputting the "final" speech. If speech is filtered after appendSpeechSequence, then speechViewer will not be accurate. This should rather use an Action extension point pre_speak or similar

[beqabeqa473]

I don't think using a filter function is appropriate in this case. appendSpeechSequence is an action, it doesn't filter the speech sequence.

Yes, I understand that, but is it worth creating an action along with
a filter just to gett a speech sequence?

[beqabeqa473]

SpeechViewer should also be outputting the "final" speech. If speech is filtered after appendSpeechSequence, then speechViewer will not be accurate. This should rather use an Action extension point pre_speak or similar

Are you sure speechViewer is outputting final speech now? in the code
of speech.speak, appendSpeechSequence is called before any filtering.

[seanbudd]

Are you sure speechViewer is outputting final speech now? in the code
of speech.speak, appendSpeechSequence is called before any filtering.

In master, filter_speechSequence.apply is called before appendSpeechSequence.
I believe other filtering further down in speech.speak happens by design after appendSpeechSequence. Is there anything you would expect to happen before appendSpeechSequence?

Yes, I understand that, but is it worth creating an action along with
a filter just to gett a speech sequence?

Well, the current code is fine as is, but if you want to use an extension point an action is more appropriate for the reasons outlined earlier.

[beqabeqa473]

Are you sure speechViewer is outputting final speech now? in the code
of speech.speak, appendSpeechSequence is called before any filtering.

In master, filter_speechSequence.apply is called before appendSpeechSequence. I believe other filtering further down in speech.speak happens by design after appendSpeechSequence. Is there anything you would expect to happen before appendSpeechSequence?

Yes, I understand that, but is it worth creating an action along with
a filter just to gett a speech sequence?

Well, the current code is fine as is, but if you want to use an extension point an action is more appropriate for the reasons outlined earlier.

I've added an extensionPoint Action which is notified just before filtering and used it in speechViewer.
I agree, that filtering is not the best way to go when we don't need to modify a speech.

[seanbudd]

This approach seems off - it should be called after the filter extension point.

I don't really see a value in this change, as mentioned earlier, the code is fine as is.
Unless other consumers find the extension point useful, I wouldn't make this change.

[seanbudd]

what problem is this PR trying to solve?

[beqabeqa473]

This approach seems off - it should be called after the filter extension point.

I don't really see a value in this change, as mentioned earlier, the code is fine as is. Unless other consumers find the extension point useful, I wouldn't make this change.

I don't understand, why speechViewer should get filtered text if in current master it is called before any filtering?
Maybe i am getting something wrong?

[beqabeqa473]

what problem is this PR trying to solve?

I don't think speech.speak is the correct place to have this code and i am not sure current approach is fine as is.
speech.speak is overloaded with unnecessary code which should go to their place, and registering a handler in speechViewer depending on logic the better than everytime importing it in speech.speak and doing unnecessary checks.

[XLTechie]

@seanbudd I hadn't given this much thought (the positioning), but wouldn't a post-filter Action here (though I agree not pre-filter) be far more useful for those consumers who do things with the speech? Sure, incidentally the Speech Viewer, but also those add-ons suggested for the Filter Extension added the other day, would be better off using an Action as close to what is actually spoken as possible. E.g. Speech Logger, Speech History, NVDA Recorder, NVDA Remote ... The Filter created can be used for more appropriate tasks, such as dictionary work, but for actually acting based on what is being spoken, an extension point that provides speech that won't be subsequently changed before speaking, would be highly valuable.

[LeonarddeR]

@seanbudd Note that a similar change has been performed for the Braille Viewer in #14503 , i.e. new extension points were made and the braille viewer has been changed to use them. To me, this pr makes perfect sense.

[CyrilleB79]

@seanbudd, actually, the extension point provided in #16191 is a filter extension point. But as explained in #16191 (comment) and following, for some add-ons, an action point was enough.

It has been decided in #16191 that the filter could also act as an action point.
But if you feel more suitable to have two separate extension points for these two usages, this may provide a use case for this PR.

[beqabeqa473]

I think preFilter speech would be good for getting raw speech sequence without modifying it for addons like nvda remote. If it will be needed, i think postFilter action also could be added to get final speech, but according to the code, speechViewer was always at the top of speech.speak code, and i am not sure it was getting final speech at all.

…

On 2/23/24, Cyrille Bougot ***@***.***> wrote: @seanbudd, actually, the extension point provided in #16191 is a filter extension point. But as explained in #16191 (comment) and following, for some add-ons, an action point was enough. It has been decided in #16191 that the filter could also act as an action point. But if you feel more suitable to have two separate extension points for these two usages, this may provide a use case for this PR. -- Reply to this email directly or view it on GitHub: #16213 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: ***@***.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

[seanbudd]

speech.speak is overloaded with unnecessary code which should go to their place,

This makes sense, though I would still use a post-filter action here.

[beqabeqa473]

speech.speak is overloaded with unnecessary code which should go to their place,

This makes sense, though I would still use a post-filter action here.

I have nothing against that, but in master, speechViewer appendSpeechSequence was always at the top in the speech.speak code, and changing it to postFilterAction could change expected behaviour.
Frankly speaking, i still cannot get a point in this. Do you want to change current behaviour? If yes, everything is clear for me in this case.

[XLTechie]

I have nothing against that, but in master, speechViewer appendSpeechSequence was always at the top in the speech.speak code, and changing it to postFilterAction could change expected behaviour.

It probably would, but I think for the better. See for example, #12077, and @feerrenrut's comment there. In fact, it might be worth considering to move the extension point filter a bit further down as well, at least to right after cancellation detection.

[seanbudd]

please do not change the behaviour here, the extension point should be called where speechViewer.appendSpeechSequence is called

[seanbudd]

this should not be above here, we do not want to write empty sequences to speech viewer. this should not be below here, we still want to write speech to speech viewer with speech off.

[AppVeyorBot]

• PASS: Translation comments check.
• FAIL: Unit tests. See test results for more information.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ort4n6n06ng3sykq/artifacts/output/nvda_snapshot_pr16213-31248,d8d5ecbd.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.1,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 30.9,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.2,
  TEST_START 0.0,
  TEST_END 0.6,
  FINISH_END 0.2

See test results for failed build of commit d8d5ecbd68

[beqabeqa473]

@seanbudd is it ok now?
While at it, i've added both pre and post filter actions to have more controll on speech from outside.

[seanbudd]

Looks good to me, please add an item in the change log for changes for developers. Use a list for new extension points like in 2024.1.

[XLTechie]

Shouldn't this be after the speech mode checks (the if statements below it)?
If those trigger, nothing is spoken. Therefore nothing should appear in the viewer, nothing will have been spoken.

[lukaszgo1]

I believe this has been discussed here before, but to recap this notification has to occur before checking what speech mode is active, to support the following use cases:

• As a sighted tester who generally works with speech disabled I want to know what would have been spoken
• As a Braille user I disable the speech, but want to see it on my Braille display in some cases (this, of course, will be possible only after Add abillity for braille to be tethered to speech #15898 is fixed)
• If this extension point would be useful for a remote access solutions it is also important the notification is send even in cases in which the person accepting help disabled the speech, i.e. since they're sighted and find the fact that their computer talks during the remote session annoying.

In general there should be no change in logic here.

[seanbudd]

@beqabeqa473 - do you intend to follow up to @XLTechie's suggestions?

[seanbudd]

Closing as abandoned

[Adriani90]

cc: @hwf1324 you might find this interesting.