Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sound split #16071
Sound split #16071
Changes from 3 commits
f73618e
adf2a8e
a856a87
8f8ce12
501737f
bfef0fc
eff0c6d
7834395
7873e96
1f6d4a4
4202ab6
11f5edc
68e5d8d
74f525c
f2d5701
1d4ffb3
62f9415
64aac39
8bddb3b
007e108
6d60437
780a402
14db885
66753cb
0fc96c7
5da261d
4671781
549dde3
ce52f0f
a9bfe6a
01ce970
04eb90a
466075c
5832eb7
cfe3015
b6adad1
cf1f6ba
26d319d
520b4fc
633b814
880544a
34cc3ab
161bc2a
ef5c81c
fb9c561
26f75fd
e3227ca
9c109d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer having two options, one to select where NVDA's sound is played and one for the application sounds. That feels more intuitive to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original and main use case of this PR is sound split, so I would like users to be able to turn it on with a single keystroke instead of having to press two separate keystrokes one for NVDA and one for other apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the list of possible sound split modes is a bit long, and if we can find something clearer, why not.
But, as @mltony writes, @LeonarddeR's suggestion of one option for NVDA sound and one for the sound of other apps would completely defeat the main use case of this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re the concern with the sound split modes being too long:
If we really want 2 options, we may divide the size of the list by 2 and use a checkbox to switch left and right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH yes, I see. Having two combo boxes would also allow a user to set NVDA right, apps right, which makes no sense at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we include pycaw as a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pycaw has its own dependencies that we don't need. I don't want to pollute NVDA with subdependency libraries that we don't need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, the only extra dependency Pycaw introduces is psutil. It is very likelike that py2exe will strip that, so in the end, there's no extra dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think generally we should avoid copying code from external dependencies where possible.
I think it would be much more elegant to use pycaw as a pip dedependency and patch as needed from NVDA, and/or make upstream PRs to fix pycaw.