Only strip from right of string when processing for symbol pronunciation and normalization is enabled#16786
Conversation
WalkthroughThe Changes
Sequence Diagram(s)N/A Assessment against linked issues
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
cc @Adriani90 |
See test results for failed build of commit 13ddf8f24e |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
|
Great suggestion @seanbudd |
|
@coderabbitai could you open an issue for the known issue as provided in the summary? |
|
@LeonarddeR, the issue has been successfully created. You can view it here. Analysis chain@LeonarddeR, I will open an issue for the known issue as provided in the summary. Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
gh issue create --title "OneCore does not pronounce spaces correctly when reporting normalized characters" \
--body "### Summary
When using OneCore with Unicode normalization enabled, NVDA fails to pronounce normalized characters correctly during character-by-character navigation. Specifically, it announces characters like \`C\` as \`Cnormalized\` without any pause, making it unclear to users. The expected behavior is to pronounce the character distinctly followed by the word \"normalized\", potentially with a short pause in between. This issue affects the readability and clarity of normalized characters for users.
### Relevant Information
- **PR:** https://github.com/nvaccess/nvda/pull/16786
- **Comment:** https://github.com/nvaccess/nvda/pull/16786#issuecomment-1662867366
### Steps to Reproduce
1. Enable Unicode normalization in NVDA.
2. Use OneCore as the speech synthesizer.
3. Navigate through text character by character.
### Expected Behavior
The character should be pronounced distinctly followed by the word \"normalized\" with a short pause in between.
### Actual Behavior
The character is pronounced as \`Cnormalized\` without any pause.
### Environment
- **NVDA version:** Latest
- **OneCore version:** Latest
### Additional Notes
This issue is a follow-up to the fix provided in PR #16786." \
--assignee "LeonarddeR"
Length of output: 1386 Actions performedComments resolved. |
|
@LeonarddeR wrote:
This bug is confirmed in French, where "cap %s" is translated to "%s majuscule". The bug is very noticeable with "V" or "Z", a little less noticeable with other letters such as "C", where OneCore speaks something like "C MAjuscule". I have also tested with 2024.1 where it was already present. Though, from what I know, no one has ever complained of this bug. |
|
@CyrilleB79 Thanks for confirming this. Could you please create an issue for this if is isn't there already? I really want to apply rstrip globally, though I agree that it shouldn't be done that way in 2024.3, but rather in 2024.4. |
Link to issue number:
Fixes #16772
Summary of the issue:
When reporting of normalized characters is enabled and OneCore is used, it does not pronounce the space that should be part of the speech sequence, so it announces
Cnormalizedrather thanc normalizedDescription of user facing changes
Normalized characters are pronounced correctly, e.g. as
C normalizedDescription of development approach
OneCore uses spelling functionality, which adds the character mode command to the speech sequence. This results in a sequence like
[CharacterModeCommand(True), "C", CharacterModeCommand(False), " normalized"].Every distinct string in the sequence is passed through
ProcessText, which strips spacing from the left and right of the string. This effectively results in a string ofCnormalizedpassed to the synthesizer. Therefore, I changed processText to only strip from the right of the string when normalization is enabled.Note that this bug is not related to the normalization code as such, the normalization work just uncovers an existing bug, which is now only fixed in the case of normalization.
Testing strategy:
Tested str from #16772
Known issues with pull request:
The underlying bug is still present when normalization is off. It would for example still apply if a translator translates the message for cap announcements:
cap %sto something like%s cap. In that case, capital C would be announced asCcapby OneCore.Code Review Checklist:
Summary by CodeRabbit