Skip to content

Change dash symbol pronunciation defaults#13857

Merged
seanbudd merged 6 commits into
masterfrom
sendDashesConsistently
Feb 14, 2023
Merged

Change dash symbol pronunciation defaults#13857
seanbudd merged 6 commits into
masterfrom
sendDashesConsistently

Conversation

@seanbudd

@seanbudd seanbudd commented Jun 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Closes #13830

Summary of the issue:

In #13830, changing the "send to synthesizer" to "always" is suggested for dashes.
Dash and em-dash are proposed here.

Description of user facing changes

Changes defaults for synthesizer pronunciation

Description of development approach

Update symbols.dic

Testing strategy:

Manual testing of reading each dash type with this config using different synthesizers.

Known issues with pull request:

None

Change log entries:

Changes

TODO: when defaults are agreed upon, notify users in changes.

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

AppVeyorBot commented Jun 30, 2022

Copy link
Copy Markdown

See test results for failed build of commit e60f823585

@seanbudd

seanbudd commented Jun 30, 2022

Copy link
Copy Markdown
Member Author

Feedback welcome: cc @sukiletxe @CyrilleB79 @tspivey @TheQuinbox

@seanbudd

seanbudd commented Jun 30, 2022

Copy link
Copy Markdown
Member Author

PR build: https://ci.appveyor.com/api/buildjobs/re89hfmw1ewvpwjj/artifacts/output%2Fnvda_snapshot_pr13857-25735%2Ce60f8235.exe

@sukiletxe

sukiletxe commented Jun 30, 2022

Copy link
Copy Markdown

This is consistent with what was reached, and works fine with espeak NG. Thanks!

CyrilleB79
CyrilleB79 reviewed Jun 30, 2022
View reviewed changes
Comment thread source/locale/en/symbols.dic Outdated
Comment thread source/locale/en/symbols.dic Outdated
Comment thread source/locale/en/symbols.dic Outdated
@trypsynth

trypsynth commented Jul 1, 2022

Copy link
Copy Markdown
Contributor

All looks good to me.

@seanbudd seanbudd added the blocked/needs-info The issue can not be progressed until more information is provided. label Jul 19, 2022
@CyrilleB79

CyrilleB79 commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

@seanbudd, you have added the blocked/needs-info label for this PR.

With my last comments, I am actually waiting for information from yourself. But since you have put yourself the label, maybe you are waiting information from someone else? Could you please clarify so that we may progress this PR?

Also for clarity, you may remove the soft hyphen from this PR and keep only the dash, en-dash and em-dash. Indeed the soft hyphen question is something distinct from other dashes and may impact the synth drivers as seen in #13668's discussion; thus it would be more clear to handle it in a separate PR.
Just my opinion though.

@seanbudd seanbudd added blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. and removed blocked/needs-info The issue can not be progressed until more information is provided. labels Sep 9, 2022
@seanbudd @CyrilleB79
Apply suggestions from code review
09c305a 
Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
@seanbudd seanbudd marked this pull request as ready for review February 14, 2023 02:06
@seanbudd seanbudd requested a review from a team as a code owner February 14, 2023 02:06
@AppVeyorBot

AppVeyorBot commented Feb 14, 2023

Copy link
Copy Markdown

See test results for failed build of commit d6e50eac06

@seanbudd seanbudd merged commit db94112 into master Feb 14, 2023
@seanbudd seanbudd deleted the sendDashesConsistently branch February 14, 2023 23:48
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Feb 14, 2023
seanbudd added a commit that referenced this pull request Feb 15, 2023
@seanbudd
Fix up dash pronunciation in system tests after #13857
984c543
@seanbudd seanbudd mentioned this pull request Feb 15, 2023
Merged
6 tasks
seanbudd added a commit that referenced this pull request Feb 16, 2023
@seanbudd
Fix up dash pronunciation in system tests after #13857 (#14640)
4b3735b 
Fixup of #13857

Summary of the issue:
#13857 was merged assuming the system test failures were random false positives.
#13857 changed symbol pronunciation of the dash symbol, and as such, relevant system tests had to be updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

+1 more reviewer

@CyrilleB79 CyrilleB79 CyrilleB79 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@seanbudd seanbudd

@michaelDCurran michaelDCurran

Labels

blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases.

Projects

None yet

Milestone

2023.2

Development

Successfully merging this pull request may close these issues.

Send dashes to the synthesizer by default

7 participants

@seanbudd @AppVeyorBot @sukiletxe @trypsynth @CyrilleB79 @michaelDCurran @nvaccessAuto