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

Fix Sapi4 pitch change #12311 #12354

Merged
merged 5 commits into from
Apr 30, 2021
Merged

Fix Sapi4 pitch change #12311 #12354

merged 5 commits into from
Apr 30, 2021

Conversation

cary-rowen
Copy link
Contributor

@cary-rowen cary-rowen commented Apr 29, 2021

Link to issue number:

#12311

Summary of the issue:

This pr solves the problem in #12311 , To put it simply, Starting with version 2019.3, the "Capital pitch change" when using the SAPI4 synthesizer was invalid.
But this must be incorrect behavior.
For visually impaired developers, this feature is especially important when typing and checking code.

Description of how this pull request fixes the issue,

I don't know why.
this function is marked as unavailable in the speak function, I managed to add this part of the code, and tested it, it works well.

Testing strategy:

  1. Start NVDA and press NVDA + CTRL + S to select "Microsoft Speech API version 4" and confirm.
  2. At this point the NVDA should have switched to the SAPI4 synthesizer and you will need to press NVDA + CTRL + V to open the Voice Setup dialog.
  3. Use the Tab key to navigate until you hear "Capital pitch change percentageeditselected 30" You can change this value to 40 (or higher, or leave it unchanged).
  4. Then use Windows + R to open the Run dialog and type notepad and confirm, this will open notepad allowing you to type some text.
  5. Press the Caps lock key (it may be located to the left of the letter A) You may need to press it twice in a row until you hear "Caps lock on".
  6. Try to enter some letters, such as the letter "C" (or "D" or something else)
  7. Use Left Arrow or Right Arrow to move between these letters.

You will hear NVDA report these capital letters in a higher pitch.

Known issues with pull request:

None

Change log entries:

Bug fixes:

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@cary-rowen cary-rowen requested a review from a team as a code owner April 29, 2021 10:21
@AppVeyorBot
Copy link

See test results for failed build of commit 49948ab928

Copy link
Contributor

@lukaszgo1 lukaszgo1 left a comment

Choose a reason for hiding this comment

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

This should be converted into a draft until Linter warnings are fixed.

source/synthDrivers/sapi4.py Outdated Show resolved Hide resolved
source/synthDrivers/sapi4.py Outdated Show resolved Hide resolved
@cary-rowen
Copy link
Contributor Author

Hi!
How should I fix Linter warning: "E225 missing whitespace around operator"
Other codes in NVDA don't seem to add spaces before and after the operators. Sorry, I may not be familiar with github.
Thanks.
@lukaszgo1

@AppVeyorBot

This comment has been minimized.

@lukaszgo1
Copy link
Contributor

How should I fix Linter warning: "E225 missing whitespace around operator"
Other codes in NVDA don't seem to add spaces before and after the operators. Sorry, I may not be familiar with github.

For the new code there are spaces on both sides of operators so you just need to add them. The code is being updated gradually to the new style that is why it may seems inconsistent.

@XLTechie
Copy link
Collaborator

XLTechie commented Apr 29, 2021 via email

@AppVeyorBot

This comment has been minimized.

@cary-rowen
Copy link
Contributor Author

I corrected all Linter warnings. Can the PR be reviewed now?
Thanks.
@lukaszgo1

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @cary-rowen

@feerrenrut feerrenrut merged commit a3af16e into nvaccess:master Apr 30, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Apr 30, 2021
@cary-rowen
Copy link
Contributor Author

I have improved some descriptions of the PR, and it is now clearer.

@cary-rowen
Copy link
Contributor Author

Most of the work of this PR was done by my friend @chenfu2000 Currently, it looks good.

@@ -123,6 +129,16 @@ def speak(self,speechSequence):
elif isinstance(item, CharacterModeCommand):
textList.append("\\RmS=1\\" if item.state else "\\RmS=0\\")
charMode=item.state
elif isinstance(item, PitchCommand):
offset = int(config.conf["speech"]['sapi4']["capPitchChange"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was looking at this code and this really feels incorrect to me. It looks like the caps pitch change is applied to every pitch change, regardless of whether it is related to capital letters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see you've opened a PR to try and fix this, that's great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants