Enable unicode normalization by default for speech #17017
Conversation
See test results for failed build of commit 84782cfb4e |
seanbudd
added
the
conceptApproved
|
@seanbudd Based on #16616 (comment) by @ruifontes, I think we should reconsider this. What do you think? |
|
@LeonarddeR - we think that we should go ahead, and that normalization is generally preferred behaviour to that example. However, if cases like this begin to stack up, we can reconsider switching it back to disabled by default |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
# Conflicts: # user_docs/en/changes.md
WalkthroughThe changes involve updating the NVDA application to enable Unicode normalization by default for speech output. This modification affects the configuration settings, command line options, and documentation, ensuring that Unicode characters are consistently processed unless explicitly disabled by the user. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
user_docs/en/changes.md (4)
92-93: Consider clarifying the Unicode Normalization feature.The description of the new Unicode Normalization feature could be clarified:
- Explain what kinds of characters have "a compatible alternative" that can be read by speech synthesizers and braille displays.
- Give some examples of what the normalized output would be.
- Clarify if this impacts reading by character, word, etc. or just when reading the full text.
This would help users better understand the behavior and benefits of this new feature.
Line range hint
2059-2060: Explain the architectural changes in more detail for add-on developers.The note about NVDA now being compiled with Visual Studio 2017 and the Windows 10 SDK is very brief. Add-on developers would benefit from more details such as:
- Specific versions of Visual Studio 2017 and the Windows 10 SDK that are required.
- Any changes required in add-on projects to support the new build environment.
- Guidance on when add-ons need to drop support for older NVDA versions.
Consider expanding this note or linking to more detailed documentation for add-on developers to ease the transition.
Line range hint
2466-2468: Example of the new NVDA+d command for annotations is incorrect.The description states:
For example, when text has a comment and a footnote associated with it.
However, the example command is:
E.g. NVDA+d now cycles through reporting the summary of each annotation target for origins with multiple annotation targets.
The
NVDA+dcommand is not mentioned before this, so introducing it as "NVDA+d now cycles" is confusing.Rewrite this to introduce and explain the
NVDA+dcommand first:- E.g. NVDA+d now cycles through reporting the summary of each annotation target for origins with multiple annotation targets. + You can press NVDA+d to cycle through reporting the summary of each annotation associated with the current text, such as comments and footnotes.
Line range hint
2999-3002: Clarify the description of the new NVDA notification behavior.The item about NVDA notification behavior could be clarified:
NVDA will no longer make the user wait while particular messages are spoken. Fixes some crashes/freezes with particular speech synthesizers. (#117)
- What kinds of messages did this apply to before?
- What is the new behavior - are the messages skipped entirely or spoken non-blocking?
- What caused the crashes and freezes in synthesizers?
Providing a bit more detail would help users better understand the change.
|
(#16616 (comment)) was ment to be here; copying it below: Could we add at least in the "Known issues" of this PR what is described in #16616 (comment), i.e. that the normalization may have unwanted effect in some situation / with some characters (e.g. "nº", common indices or exponents in math expressions such as "ⁱ" or "ₙ") Moreover, I wonder if we could add a third option for normalization which would be "When suitable", forcing normalization in Word equations and other situations where it would be required and keeping it disabled in other cases. But that would be for another PR in any case. |
|
@seanbudd, @Qchristensen what do you think about #17017 (comment)? |
|
I agree that it should be listed in the known issues section of the PR description. I think a third option for "when suitable" should be proposed in a separate issue |
|
@coderabbitai Please create an issue for adding a "when suitable" option to the unicode normalization setting of NVDA's speech options. Please remember to use the NVDA feature request template, and use the discussion in this PR to inform the reasoning in the issue you create. |
|
Tip For best results, initiate chat on the files or code changes. @SaschaCowley, the issue for adding a "when suitable" option to the Unicode normalization setting in NVDA's speech options has been created. However, the "feature request" label could not be added because it was not found in the repository. You may want to manually add the correct label if needed. Analysis chain@SaschaCowley, I will create the issue for adding a "when suitable" option to the Unicode normalization setting in NVDA's speech options, using the NVDA feature request template and incorporating the discussion from this PR. Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Create a new issue using the NVDA feature request template.
gh issue create --title "Add 'When suitable' option to Unicode normalization setting in NVDA's speech options" --body "### Feature Request
**Description:**
Currently, Unicode normalization is either enabled or disabled by default in NVDA's speech options. However, there are situations where normalization may have unwanted effects, such as with certain characters (e.g., 'nº', common indices or exponents in math expressions like 'ⁱ' or 'ₙ'). To address this, we propose adding a third option, 'When suitable,' which would enable normalization only in specific contexts, such as Word equations, where it is required.
**Rationale:**
This feature would provide users with more granular control over Unicode normalization, ensuring that it is applied only when necessary, thereby avoiding potential issues in other contexts. The discussion in PR #17017 highlighted the need for this option, and it was agreed that it should be proposed in a separate issue.
**References:**
- PR #17017: Enable Unicode normalization by default for speech
- Issue #16616: Enable Unicode normalization by default for speech output in NVDA
**Proposed Solution:**
- Add a 'When suitable' option to the Unicode normalization setting in NVDA's speech options.
- Ensure that this option enables normalization only in contexts where it is required, such as Word equations.
**Impact:**
This change would improve the user experience by preventing unwanted normalization effects while still providing the benefits of normalization in appropriate contexts.
**Additional Notes:**
This feature request is based on the discussion in PR #17017 and the related issue #16616." --assignee SaschaCowley --label "feature request"
Length of output: 1790 |
See test results for failed build of commit 6f8674d156 |
See test results for failed build of commit e36c008753 |
|
@LeonarddeR - this PR seems to break system tests. Can you investigate |
|
I think this is because |
|
The way NVDA introduces spaces in speech sequences is really messy and should be investigated at some point
Can you open an issue or discussion on that, so it can be tagged for someone to work on in the future, when you're not available to explain the problems you noticed with it?
I haven't looked at that in particular, so maybe it's obvious, but one person's "obvious" is often another's "what is he talking about?" :)
|
See test results for failed build of commit 664174e83e |
Link to issue number:
Fixes #16616
Summary of the issue:
In #16521, Unicode normalization was added, but it is disabled by default.
Description of user facing changes
Unicode normalization is now enabled by default for Speech.
Description of development approach
Change default values
Testing strategy:
Ensured that the default is now enabled for speech.
Known issues with pull request:
Unicode normalization is still disabled by default for Braille. For speech, if someone defines symbol pronunciation for a character, it is never normalized, even when normalization is on. For Braille however, we are unable to find out pre-translation whether liblouis knows about a character. It is possible that an unnormalized character translates correctly, whereas the normalized equivalent does not.
The normalization may have unwanted effects in some situations, such as the Portuguese abbreviation "nº" being misread by some synthesizers after normalization, or making it difficult to tell the meaning of some mathematical symbols such as "ⁱ" (superscript i) or "ₙ" (subscript n).
Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
-c/--config-pathand--disable-addonsnow respected during updates.Documentation
Chores