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

Add audio feedback for text selection #19850

Merged
merged 8 commits into from
Aug 26, 2022
Merged

Conversation

nekodex
Copy link
Contributor

@nekodex nekodex commented Aug 19, 2022

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

Is it intended that both the caretMoved and deselect samples play when the selection is dismissed by keyboard arrow keys.

Eg. select some word in a middle of a sentence and then press left or right arrow key.


default:
channel = selectCharSample?.GetChannel();
pitch += (SelectedText.Length / (float)Text.Length) * 0.15f;
Copy link
Member

Choose a reason for hiding this comment

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

Possible division by 0 here, resulting in NaN pitch. Shockingly, this doesn't crash, but instead plays the sound high-pitched. It's probably not intended for selection to change and Text.Length to be 0 (framework-side bug) but I think this should be guarded against.

Also, pitch is double, but this is casting to float.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How were you able to trigger a selection change with Text.Length being zero? It's probably better to ensure OnTextSelectionChanged is never called when Text.Length is zero than guarding here?

@nekodex nekodex self-assigned this Aug 24, 2022
@peppy peppy self-requested a review August 25, 2022 10:17
@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 25, 2022
@peppy
Copy link
Member

peppy commented Aug 26, 2022

Test failures here will be fixed via #19828, marking as prereq for now.

@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Aug 26, 2022
@pull-request-size pull-request-size bot added size/M and removed size/L labels Aug 26, 2022
@peppy peppy enabled auto-merge August 26, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants