Skip to content

added support for delayed phonetic character descriptions#13550

Merged
seanbudd merged 51 commits intonvaccess:masterfrom
davidacm:delayedDesc
Jul 19, 2022
Merged

added support for delayed phonetic character descriptions#13550
seanbudd merged 51 commits intonvaccess:masterfrom
davidacm:delayedDesc

Conversation

@davidacm
Copy link
Copy Markdown
Contributor

@davidacm davidacm commented Mar 28, 2022

Link to issue number:

Closes #13509

Summary of the issue:

The main use case is navigating character by character and a user encounters a character that they do not understand. The delayed phonetic character description is something like “alpha”/“beta”/“romeo”/“igloo”.

What makes understanding a character difficult?
This may be because a synthesizer pronounces a character unusually or indistinguishable from another character.
A user may also be hearing impaired or have other trouble processing the character. In this case, the user waits for the delayed phonetic description to occur.

Other ways to report the character description:

  • script_review_currentCharacter
  • script_review_currentWord
  • script_review_currentLine
  • script_reportCurrentLine

What makes determining the delay difficult?
The main determinate of the delay is the speed that a user interacts with the device, e.g. speed of navigating by character.
NVDA currently does not have a way to configure interaction rate, which is a general problem described in #13915.

Meeting auditory processing needs is generally achieved by adjusting the synthesizer rate.
Unfortunately, synthesizer rate varies by synthesizer, and so calculating a delay based on rate is challenging.

While a general solution for the interaction rate is blocked by #13915, using a sensible default will handle most users requirements. This delay can be configured easily by add-ons.

Description of user facing changes

Adds an option, which when enabled, causes character descriptions to be announced after a fixed delay of 1 second.
This delay value has been tested as a default in the “Enhanced phonetic reading” Add-on.

Description of development

This code modifies getTextInfoSpeech to emit a BreakCommand followed by several commands to speak a character description. This only happens if the unit of resolution is a single character, and the reason for querying for textInfo speech is caret. This causes NVDA to emit character descriptions in the following conditions:

  • when moving by character in an edit field.
  • When moving by character in review cursors.
  • When moving by character in browse mode.
  • When reporting single characters at review cursor.
  • Any other time that caret movement is being reported for single characters.

This solution also reads the character description with capitalization settings.

Using the BreakCommand means that the overall approach is simpler as opposed to using a timer, and handled by the synthesizer rather than NVDA.

Testing strategy:

This feature was taken from an add-on called "Enhanced Phonetic Reading". The UX has been tested by many users in form of add-on by years. The default delay is 1 second, and can be configured between 100-5000ms.
The internal code has largely been rewritten in this PR.

System tests have been created to test that delayed descriptions are spoken.

Manual testing of reading characters in notepad has been performed using eSpeak, OneCore, SAPI5 and SAPI4.
This involved testing different delay rates and cancelling the description.

Known issues with pull request:

None

Change log entries:

New features

  • Added ability to read character descriptions when the user is moving by characters, delayed by 1 second.

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

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 7d03e287be

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit f3ead48311

@davidacm davidacm marked this pull request as draft March 28, 2022 21:45
derekriemer and others added 3 commits March 28, 2022 21:32
Co-authored-by: Ozancan Karataş <ozancankaratas96@outlook.com>
Co-authored-by: Ozancan Karataş <ozancankaratas96@outlook.com>
Co-authored-by: Ozancan Karataş <ozancankaratas96@outlook.com>
@cary-rowen
Copy link
Copy Markdown
Contributor

I am interested in this feature, But the current CI build is dead and I can't test it.
can you provide a new one

@davidacm davidacm marked this pull request as ready for review May 2, 2022 04:14
@davidacm
Copy link
Copy Markdown
Contributor Author

davidacm commented May 2, 2022

Hi @cary-rowen , the links expire after one month, try with this new one that was generated by the new commit. Please take in account that your antivirus can try to delete this file because the installer is unsigned.
[]NVDA build with delaied descriptions(https://ci.appveyor.com/api/buildjobs/bkjt0rnrc065vvno/artifacts/output%2Fnvda_snapshot_pr13550-25331%2C7afa5930.exe)

@cary-rowen
Copy link
Copy Markdown
Contributor

Hi @davidacm
I tested it,
For the "Time for delayed phonetic descriptions" control, can it be displayed or not based on the selected state of "Delayed phonetic descriptions for characters on cursor movement"?

…imeoutMsEdit if delayedPhoneticDescriptionsCheckBox is toggled.
@davidacm
Copy link
Copy Markdown
Contributor Author

davidacm commented May 3, 2022

Hi @cary-rowen.
Thanks for the suggestion. I implemented it now, please try to see if it works for you!

Thanks!

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 18e0ee0daa

@LittleStar-VIP
Copy link
Copy Markdown

Hello,
I have downloaded https://ci.appveyor.com/api/buildjobs/bkjt0rnrc065vvno/artifacts/output%2Fnvda_snapshot_pr13550-25331%2C7afa5930.exe and tested it in Notepad++, Brave web browser, Powershell, and everything is as expected.
Once again, may I thank you again for this wonderful addon and feature.

@cary-rowen
Copy link
Copy Markdown
Contributor

Hi, @davidacm
Can you keep improving this pr, I'm looking forward to him appearing in NVDA core.

@davidacm
Copy link
Copy Markdown
Contributor Author

Hi @cary-rowen , I don't know what more improvements I can do to this pr. I have some ideas related to this feature, but those ideas should be implemented in another pr because I don't want to mix features.

I'm open to suggestions in code implementation and improvements, and ideas from users, but at this time, I did receive suggestions about that.

@cary-rowen
Copy link
Copy Markdown
Contributor

Hi @davidacm

AppVeyor CI seems to have a check failure, you might be able to take a look,


What is NV Access's attitude towards this, can you take a look at this PR?
@seanbudd
@feerrenrut

@seanbudd seanbudd marked this pull request as ready for review July 18, 2022 04:52
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 2303847532

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 3970f37de2

@LittleStar-VIP
Copy link
Copy Markdown

Thank you every one for making this pr possible, especially @seanbudd @davidacm

@derekriemer
Copy link
Copy Markdown
Collaborator

derekriemer commented Jul 18, 2022 via email

@seanbudd
Copy link
Copy Markdown
Member

@derekriemer - no difference, except that the delay parameter will be generic, and used to configure other settings as well.

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 9e3e9a94c4

@davidacm
Copy link
Copy Markdown
Contributor Author

I don't agree with the delay parameter to be generic if the feature is still relying on the synth break command. Even espeak has imprecision times on rate speed changes, so this can be a issue when changing the synthesizer because with breakCommands, the same value is not equivalent for each synthesizer.
But if the #13915 PR solves the problem of the inaccurate pauses, then I do agree that the parameter should be generic, in fact I find it more useful (at least for me).

I did not see the new code yet, and I didn't see the code of #13915 (although the idea of that pr sounds interesting) at least in the way I understood it.

P.S. Why not reuse the code to read the character description, and the code to obtain the delayed character description?
It's the same thing, the difference is that the first is caused by the user (E.G. by pressing numpad 2 twice) and the second is read after some predefined ms.

I don't know if this has been made, but is just a suggestion.

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit fcd1b10179

@seanbudd
Copy link
Copy Markdown
Member

P.S. Why not reuse the code to read the character description, and the code to obtain the delayed character description?

This change is simpler, it adds the character description to the same speech sequence, as opposed to starting a new one. The code to read a character description when activating a script is a different scenario - I don't think it makes sense to activate that script as a side effect of speech.

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit fcd1b10179

@davidacm
Copy link
Copy Markdown
Contributor Author

I don't mean the script, just the function to get this information. So, both features can call the same function to obtain the information.

P.S. Consider the possibility to use indidual values for the delay for each synthesizer, since most of synths are not accurate in the pause time. I would not like that if i'm using IBMTTS with 1000ms delay and when change to sapi5 or one core, get a different pause time for the delay. Because then, I would need to adjust the delay each time I change the synthesizer.

@seanbudd seanbudd merged commit 3894e25 into nvaccess:master Jul 19, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jul 19, 2022
@TechHorseG
Copy link
Copy Markdown

I don't know if everything is already locked in, but I would like to register an opinion.

I am very nervous about using a generic setting for the time delay for Delayed descriptions for characters.

To explain. I am used to using a 1 second delay in the Enhanced Phonetic Reading addon, so I would naturally want to initially set this generic idle time delay to 1s.

However, if I then find myself having to wait for 1s delays in other places to get information I used to be able to obtain more quickly, this will be problematic.

If I have spent years getting the information relatively fast, then I know I am going to find any new delays a drag, and would want to turn them off to retain the quickness I am used to. But then that would be highly problematic for the Delayed descriptions for characters, where I definately do not want an immediate description.

That's it, just wished to register my high feelings of caution on putting the time delay for Delayed descriptions on a generic one-size-fits-all time delay setting, as I feel that this sort of thing has the potential to adversely affect my experience of using NVDA.

On a more positive note. Thanks for introducing this essential feature at all. I haven't updated NVDA in over two years for no other reason than I didn't want to lose the Enhanced Phonetic Reading addon's functionality, when it stopped being compatible.

@seanbudd
Copy link
Copy Markdown
Member

seanbudd commented Aug 4, 2022

@TechHorseG - I suggest you read #13915 (and comment there - not here) before making assumptions of what could be affected by a generic parameter.

Additional note: As far I am aware Enhanced Phonetic Reading is compatible with the latest NVDA, and has been regularly updated with each breaking release. Thanks to @davidacm!

@davidacm
Copy link
Copy Markdown
Contributor Author

davidacm commented Aug 4, 2022

Hi @TechHorseG.
I'm the developer of the add-on you mentioned.
About enhanced phonetic reading, I will continue with the maintenance until all my concerns about the current implementation in the core of NVDA, are resolved and that may take some years I think.
Get the latest version of the enhanced phonetic reading here
That link always point to the latest release.

Is hard to me to update all my add-ons in the main NVDA's repo, because I still found the process a little tedious, but this add-on will always be available to everyone until it's no longer useful.

You can see my comments about my concerns in this thread. In my opinion timers are the correct way to go, but I know there are some technical limitations and also architecture implementations to be satisfied first.

seanbudd added a commit that referenced this pull request Aug 4, 2022
Fixes #13969
Fixes #13966

Background for the feature in PR #13550

Summary of the issue:
NVDA does not correctly handle language switching commands when speaking character descriptions.

Description of user facing changes
Ensure character descriptions are read in the proper locale where known.

Description of development approach
Use the same code other spelling commands use, _getSpellingSpeechWithoutCharMode, which handles language switching.
@TechHorseG
Copy link
Copy Markdown

TechHorseG commented Aug 5, 2022

seanbudd and davidacm thanks for the enhanced phonetic reading links. I had no idea that the addon was still alive. Good to know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audience/hearing-impaired blocked/needs-code-review 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.

NVDA add automatically speak character description function when cursor move