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

Add "Only in edit controls" mode for typing echo #17505

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

cary-rowen
Copy link
Contributor

Link to issue number:

Fixes #16848, related #10331, #3027

Summary of the issue:

Currently NVDA can only toggle typing echo (characters and words) on or off globally. Users want more granular control to only have typing feedback in edit controls, while keeping it off in other contexts like listss or non-edit areas.

Description of user facing changes

  • Added a new option "Only in edit controls" for both "Speak typed characters" and "Speak typed words" settings in Keyboard Settings
  • Instead of checkboxes, these are now combo boxes with three options:
    • Off: No typing echo
    • On: Echo all typed text
    • Only in edit controls: Only echo text typed in edit fields
  • Updated relevant documentation in the user guide

Description of development approach

The implementation:

  1. Added a TypingEcho enum in configFlags.py with values:
    • OFF (0)
    • ON (1)
    • EDIT_CONTROLS (2)
  2. Changed keyboard typing echo configuration from boolean to integer values
  3. Updated speech.py, behaviors.py and inputComposition.py to use the new enum
  4. Modified settings dialog to use combo box instead of checkbox
  5. Updated documentation

Testing strategy:

Tested the following scenarios:

  1. Basic functionality:
  • Open Notepad (edit control)
    • Set to "Only in edit controls"
    • Type text - should be announced
    • Verify both character and word echo settings
  • Explorer file lists(non-edit control)
    • Type text - should not be announced
  • Test all three modes (Off, On, Only in edit controls)
  1. Different contexts:
  • Web browser input fields
  • Rich text editors
  • Read-only text areas
  • Terminal windows

Known issues with pull request:

None identified.

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

@cary-rowen cary-rowen marked this pull request as draft December 11, 2024 23:28
@Adriani90
Copy link
Collaborator

Great work on this, thank you.

@Adriani90
Copy link
Collaborator

I think this needs a changelog entry as well.

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results and lint artifacts for more information.
  • PASS: System tests (tags: installer NVDA).
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/2b1503020gqhri7g/artifacts/output/l10nUtil.exe nvda_snapshot_pr17505-34744,8a5bed98.exe
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.2,
    INSTALL_END 0.9,
    BUILD_START 0.0,
    BUILD_END 25.6,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.3,
    TEST_START 0.0,
    TEST_END 18.7,
    FINISH_END 0.1

See test results for failed build of commit 8a5bed9868

@cary-rowen cary-rowen marked this pull request as ready for review December 12, 2024 03:33
@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results and lint artifacts for more information.
  • PASS: System tests (tags: installer NVDA).
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8dcauldlcfjvaodn/artifacts/output/l10nUtil.exe nvda_snapshot_pr17505-34752,259e6d54.exe
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.2,
    INSTALL_END 0.9,
    BUILD_START 0.0,
    BUILD_END 24.6,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.3,
    TEST_START 0.0,
    TEST_END 18.6,
    FINISH_END 0.2

See test results for failed build of commit 259e6d5452

@CyrilleB79
Copy link
Collaborator

Very nice addition @cary-rowen! Thanks.

I have some questions:

  1. Shouldn't there be a config upgrade? I.e. a value in the old config should remain in the new one.
  2. I think that "Only in edit controls" should become the default behaviour; I guess that most people would be happy and moreover, it's a security fix as described in NVDA reading out words it shouldn't if 'speak typed words' is on #10331.
  3. If NV Access reviewers (@SaschaCowley or @seanbudd) do not accept the new behaviour to be the default for now, should we use a feature flag to be able to change the default in the future?
  4. With the new option, can typing be heard when typing in an edit field but when text is not actually written, e.g.:
    • In cmd, when initiating an ssh connection, typing the password outputs no visible character
    • In a read-only edit field, e.g. the synth field of NVDA speech settings panel.

Copy link
Collaborator

@nvdaes nvdaes left a comment

Choose a reason for hiding this comment

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

@cary-rowen , thanks for this.
I think that you may add a helper function in globalCommands, similar to the toggleBooleanValue function, for config flags, and use it for toggling typed characters and words.
For reference, see toggleBooleanValue in globalCommands.py, introduced in PR #16994

@cary-rowen
Copy link
Contributor Author

Hi @CyrilleB79
Thank you for your review! I'd like to clarify your last point about special input scenarios. Are you asking whether the new "Only in edit controls" option works as expected in these cases:

  1. When typing passwords in cmd during SSH connection:
  • Should typed characters be announced in this case?
  1. In read-only edit fields (like the synthesizer field in NVDA speech settings):
  • Though it looks like an edit field, it's read-only
  • I've tested that typed characters are not announced here with the new option, as no text can be entered
  • Is this the expected behavior?

Could you please confirm if these behaviors are what you're asking about, and what the expected behavior should be in these cases?

Many thanks

@cary-rowen
Copy link
Contributor Author

Regarding default settings, I'd also love to hear suggestions from NV Access, I'd personally like to see new options provided as defaults for the same reasons as @CyrilleB79.

@nvdaes
Thanks for your suggestion, I will implement a helper method to simplify the code.

@CyrilleB79
Copy link
Collaborator

@cary-rowen IMO, the expected behaviour with the new option in ssh password case and in read-only edit fields such as synthesizer is that NVDA should not speak anything since nothing is actually visible on the screen.

@amirmahdifard
Copy link

thaaaaanks! should I really beleave that from now on we can get rid of the t. This pc 10 of 18, or space. playback stopped, and space. Playback resumed, with out needing to install the best ever addon made by @cary-rowen that addon was the best, because I alwayes liked and needed to keep typed characters and words, but I hated how they are announced everywhere, I was thinking, how stupid, the option it self is called speeck ('"typed"') characters and words, but why whe were hearing it everywhere? whe are not typing anything! whe are using our keyboard to use and explor our computer! but from now on, at least we don't have to use an addon to solv this really annoying this. And, really thank you @cary-rowen for resolving such problem for us until now. befor your addon releace on the stor, I was using an old and less buggy vertion of an unknown addon called typing settings to solv this for my self. greate feature!

@Adriani90
Copy link
Collaborator

@cary-rowen speaking typed characters outside of edit fields does not make sense anyway, because they are not typed, but only pressed. For that we have the input help feature already if people need to learn the keyboard layout outside of edit fields.
So unless there is a valid use case for having characters being spoken outside of edit fields,
Why don't you just change the current setting when enabled to be valid only for edit fields?
In this case we could let it be a checkbox and don't need three options.

@amirmahdifard
Copy link

@Adriani90 hi. No, I also don't agree for removal of that in my opinion because, input help is a thing, but this is stil needed for some people where they need to press some keys and need to be announced for them what they've preased and what came up, or what happened as a result, while some newbie people need to turn on input help, press a fue keys that they need, and turn of input help, and make sure they've pressed or pressing the correct key. Also, for a groupe of users with old habit, better to not remove it but rename it instead for example, from on, to everywhere. so the options are as folows. off, in edit boxes only, everywhare.

@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Dec 17, 2024
@cary-rowen
Copy link
Contributor Author

@CyrilleB79
wrote:

the expected behaviour with the new option in ssh password case and in read-only edit fields such as synthesizer is that NVDA should not speak anything since nothing is actually visible on the screen.

I agree that there shouldn't be any feedback in the read-only edit box after enabling the new options introduced by this PR. This is currently as expected.
However, I would like to have feedback when typing the password in ssh, unlike other password fields, in the normal password field NVDA will report 'star' when typing the password.
In the SSH password field, the characters are actually entered, they are just not displayed, unlike read-only fields (which do not produce any input)

cc @seanbudd

@cary-rowen
Copy link
Contributor Author

I agree with @amirmahdifard I am totally not in favor of removing the original mode.

@cary-rowen
Copy link
Contributor Author

Thanks @SaschaCowley and @CyrilleB79 for the review.
For the upgrade step I'm wondering do I need to create a separate upgrade method?
For example, upgradeConfigFrom_14_to_15

As @CyrilleB79 commented:

Should the removal of self.charsCheckBox be documented in the API breaking changes of the change log? Idem for words?
Though, I am not sure that we have always respected the API when modifying the GUI in non-.1 releases in the past...

I'm not sure about this.

source/config/profileUpgradeSteps.py Outdated Show resolved Hide resolved
source/config/profileUpgradeSteps.py Outdated Show resolved Hide resolved
Copy link
Member

@SaschaCowley SaschaCowley 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 copyright headers of any files you have touched to reflect that they have been modified in 2025.

source/config/configFlags.py Outdated Show resolved Hide resolved
source/config/configSpec.py Outdated Show resolved Hide resolved
source/config/configSpec.py Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/speech/speech.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
user_docs/en/changes.md Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

This is looking good, just a few small things to go

source/NVDAObjects/inputComposition.py Outdated Show resolved Hide resolved
source/config/configSpec.py Outdated Show resolved Hide resolved
source/config/profileUpgradeSteps.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@cary-rowen
Copy link
Contributor Author

Thanks @SaschaCowley
learned a lot

@AppVeyorBot
Copy link

  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit e669f7f4cf

source/config/configSpec.py Outdated Show resolved Hide resolved
@@ -51,6 +51,8 @@ To use this feature, "allow NVDA to control the volume of other applications" mu
* Short versions of the most commonly used command line options have been added: `-d` for `--disable-addons` and `-n` for `--lang`.
Prefix matching on command line flags, e.g. using `--di` for `--disable-addons` is no longer supported. (#11644, @CyrilleB79)
* Microsoft Speech API version 5 and Microsoft Speech Platform voices now use WASAPI for audio output, which may improve the responsiveness of those voices. (#13284, @gexgd0419)
* The keyboard settings for "Speak typed characters" and "Speak typed words" now have three options: Off, Always, and Only in edit controls. (#17505, @Cary-rowen)
Copy link
Contributor

Choose a reason for hiding this comment

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

The order here·Does it need to be adjusted?

Suggested change
* The keyboard settings for "Speak typed characters" and "Speak typed words" now have three options: Off, Always, and Only in edit controls. (#17505, @Cary-rowen)
* The keyboard settings for "Speak typed characters" and "Speak typed words" now have three options: Off, Only in edit controls, and Always. (#17505, @Cary-rowen)

user_docs/en/userGuide.md Show resolved Hide resolved
user_docs/en/userGuide.md Show resolved Hide resolved
@AppVeyorBot
Copy link

  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit cd4e125c45

@cary-rowen
Copy link
Contributor Author

Hi @nvdaes

Could you please review this commit?

Add toggleIntegerValue helper function and update toggle scripts

Thanks

@AppVeyorBot
Copy link

  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 16ca2516ea

cary-rowen and others added 5 commits January 11, 2025 10:56
Closes nvaccess#13284

Summary of the issue:
Currently, SAPI5 and MSSP voices use their own audio output mechanisms, instead of using the WavePlayer (WASAPI) inside NVDA.

This may make them less responsive compared to eSpeak and OneCore voices, which are using the WavePlayer, or compared to other screen readers using SAPI5 voices, according to my test result.

This also gives NVDA less control of audio output. For example, audio ducking logic inside WavePlayer cannot be applied to SAPI5 voices, so additional code is required to compensate for this.

Description of user facing changes
SAPI5 and MSSP voices will be changed to use the WavePlayer, which may make them more responsive (have less delay).

According to my test result, this can reduce the delay by at least 50ms.

This haven't trimmed the leading silence yet. If we do that also, we can expect the delay to be even less.

Description of development approach
Instead of setting self.tts.audioOutput to a real output device, do the following:

create an implementation class SynthDriverAudioStream to implement COM interface IStream, which can be used to stream in audio data from the voices.
Use an SpCustomStream object to wrap SynthDriverAudioStream and provide the wave format.
Assign the SpCustomStream object to self.tts.AudioOutputStream, so SAPI will output audio to this stream instead.
Each time an audio chunk needs to be streamed in, ISequentialStream_RemoteWrite will be called, and we just feed the audio to the player. IStream_RemoteSeek can also be called when SAPI wants to know the current byte position of the stream (dlibMove should be zero and dwOrigin should be STREAM_SEEK_CUR in this case), but it is not used to actually "seek" to a new position. IStream_Commit can be called by MSSP voices to "flush" the audio data, where we do nothing. Other methods are left unimplemented, as they are not used when acting as an audio output stream.

Previously, comtypes.client.GetEvents was used to get the event notifications. But those notifications will be routed to the main thread via the main message loop. According to the documentation of ISpNotifySource:

Note that both variations of callbacks as well as the window message notification require a window message pump to run on the thread that initialized the notification source. Callback will only be called as the result of window message processing, and will always be called on the same thread that initialized the notify source. However, using Win32 events for SAPI event notification does not require a window message pump.

Because the audio data is generated and sent via IStream on a dedicated thread, receiving events on the main thread can make synchronizing events and audio difficult.

So here SapiSink is changed to become an implementation of ISpNotifySink. Notifications received via ISpNotifySink are "free-threaded", sent on the original thread instead of being routed to the main thread.

To connect the sink, use ISpNotifySource::SetNotifySink.
To get the actual event that triggers the notification, use ISpEventSource::GetEvents. Events can contain pointers to objects or memory, so they need to be freed manually.
Finally, all audio ducking related code are removed. Now WavePlayer should be able to handle audio ducking when using SAPI5 and MSSP voices.
Copy link
Collaborator

@nvdaes nvdaes left a comment

Choose a reason for hiding this comment

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

@cary-rowen I've reviewed changes in globalCommands.py.
Looks good to me, except for minor inconsistencies in comments for translators.

@@ -536,38 +563,36 @@ def script_previousSynthSetting(self, gesture):
ui.message("%s %s" % (previousSettingName, previousSettingValue))

@script(
# Translators: Input help mode message for toggle speaked typed characters command.
description=_("Toggles on and off the speaking of typed characters"),
# Translators: Input help mode message for cycling the reporting of typed words.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Translators: Input help mode message for cycling the reporting of typed words.
# Translators: Input help mode message for cycling the reporting of typed characters.

# Translators: Reported when the user cycles through speak typed characters modes.
# {mode} will be replaced with the mode; e.g. Off, On, Only in edit controls.
messageTemplate=_("Speak typed characters: {mode}"),
)

@script(
# Translators: Input help mode message for toggle speak typed words command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Translators: Input help mode message for toggle speak typed words command.
# Translators: Input help mode message for cycling the reporting of typed words.

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.

Speak typed characters and words applied to edit fields only.
10 participants