-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Keystrokes to adjust applications volume and mute [take 2] #16591
Conversation
@mltony really very inspiring that you are still motivated to find a more bullet proof solution here, very nice work.
|
|
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.
Glad to see that this is implemented in such a way that it doesn't have impact by default.
Co-authored-by: Leonard de Ruijter <[email protected]>
Co-authored-by: Leonard de Ruijter <[email protected]>
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 are a large number of API breaking changes in this PR. I have highlighted only a couple of them, Do you think backwards compatibility is possible? Otherwise we may need to postpone this to 2025.1
Another way we could handle this is making 2024.2 API changes private in beta ASAP. Note that any API breaks from 2024.1 or earlier cannot be done until 2025.1. |
I agree with this plan, 2024.2 has not yet been released. |
Co-authored-by: Sean Budd <[email protected]>
Co-authored-by: Sean Budd <[email protected]>
Co-authored-by: Sean Budd <[email protected]>
Co-authored-by: Sean Budd <[email protected]>
Co-authored-by: Sean Budd <[email protected]>
|
Following @seanbudd's suggestion in #16591 making classes and functions private. The rationale is that we would like to refactor sound split code so that more code is going to be reused in application volume adjuster, but #16591 will have to be postponed to v2024.3. So we need to make sound split API private to avoid breaking API changes.
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.
@mltony, thanks for reintroducing this.
I have to take more time to review and test this.
Here are already fixes for a case issue in the config causing the following critical error at startup:
CRITICAL - __main__ (15:44:29.962) - MainThread (16076):
core failure
Traceback (most recent call last):
File "nvda.pyw", line 415, in <module>
core.main()
File "core.py", line 689, in main
audio.initialize()
File "audio\__init__.py", line 35, in initialize
appsVolume.initialize()
File "audio\appsVolume.py", line 30, in initialize
state = config.conf["audio"]["ApplicationsVolumeMode"]
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
File "config\__init__.py", line 1136, in __getitem__
raise KeyError(key)
KeyError: 'ApplicationsVolumeMode'
I'll test more in depth and review the PR later. But it seems to mee that the mute UX is quite strange with its 3 states; and I have also experimented strange volume jumps.
Co-authored-by: Cyrille Bougot <[email protected]>
Co-authored-by: Cyrille Bougot <[email protected]>
Co-authored-by: Cyrille Bougot <[email protected]>
Co-authored-by: Cyrille Bougot <[email protected]>
@wmhn1872265132 wrote:
I expect that the majority of people using other app volume or mute commands will enable app volume adjuster once and for all and save it in config and then won't touch it again. So a shortcut key to enable app vol adjuster would be used only once by the majority of the people. It does not matter if it is not enabled by default; what matter is if the majority of people using app volume modifiers and mute will have app volume adjuster enabled in their config or not. You could argue that it would be better in this case to have app volume adjuster enabled by default. I was initially in favor of this. But @LeonarddeR (and maybe others) have exhibited use case with add-ons or other apps requiring it to be disabled by default for clarity regarding sound processing. |
At least for me, this feature will only be used temporarily and should be disabled in most scenarios because it interferes with my volume settings for other applications in the volume synthesizer |
I assigned |
See test results for failed build of commit 1ec6e99a9d |
Co-authored-by: Sean Budd <[email protected]>
Co-authored-by: Luke Davis <[email protected]>
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.
Good work, and good luck with take 2 :)
Thanks @mltony ! |
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 work seems OK to me now.
Thanks @mltony for this very useful new feature, for the very good job, and for your patience with all my comments!
A few questions:
|
Also one small remark from my side, it seems the options for the setting application adjuster are ambiguous:
* Standard (disabled application adjuster): this option is not needed because it is not an experimental setting and thus it is not part of the advanced settings pannel
* Disabled application adjuster: should just be “disabled” without “application adjuster”
* Enabled application adjuster: should just be “enabled”, without “application adjuster”.
@mltony could you please update the GUI accordingly to keep consistency?
Thank you very much for the great work on this.
Von: Bill Dengler ***@***.***>
Gesendet: Dienstag, 3. September 2024 21:41
An: nvaccess/nvda ***@***.***>
Cc: Adriani90 ***@***.***>; Mention ***@***.***>
Betreff: Re: [nvaccess/nvda] Keystrokes to adjust applications volume and mute [take 2] (PR #16591)
A few questions:
* This feature appears to be undocumented in the user guide. What is the drawback to disabling applications volume adjuster?
* I agree that if the feature is off by default, it makes more sense to have the gestures unbound.
o This is especially underscored by the fact that NVDA+Alt+pgup/pgdn are used by the very popular NVDA Remote <https://nvdaremote.com> add-on. This might complicate #4390 <#4390> .
—
Reply to this email directly, view it on GitHub <#16591 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGVCP4KS7RYVR5A2CU6GYT3ZUYGEZAVCNFSM6AAAAABIEURONSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRXGI4TAMJWGA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AGVCP4JS2MSMA5ZRIBCKZOLZUYGEZA5CNFSM6AAAAABIEURONSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUKW6QTA.gif> Message ID: ***@***.*** ***@***.***> >
|
Also, in English you'd say "application volume adjuster", not "applications". |
I'd encourage opening new issues for any work you'd like tracked. Comments on closed PRs are easily lost to the sands of time. |
Thanks @seanbudd, it was a pleasure working with you. I need to focus on my full-time job for now, but as I mentioned before, I plan to come back around January 2025 and at least finish my sentence navigation PR #16384. Also, @codeofdusk, I actually consulted chatGPT on this:
Is ChatGPT wrong? |
Link to issue number:
Closes #16052.
Please note that this is my second attempt to implement this. The first attempt was #16273 which was reverted in #16440# by commit f63841f.
Summary of the issue:
Provide a way to adjust volume of other applications, so that when using sound split the volume of aplications channel can be adjusted separately from NVDA volume.
Description of user facing changes
New keystrokes:
NVDA+alt+pageUp/pageDown
adjusts volume of all other applications.NVDA+alt+delete
Cycles between three statesDescription of development approach
audio/utils.py
. Now it provides an abstract classAudioSessionCallback
and customers (sound split and app volume adjuster) are implementing this class. Two methods that need to be implemented areonSessionUpdate
andonSessionTerminated
, which operate on a single audio session. All the plumbing is hidden inutils.py
so that clients don't need to worry about setting up and dealing with multiple lower-level callbacks.audio/appsVolume.py
and is quite straightforward now.ISimpleAudioVolume
interface, which means that both are reflected in Windows Volume Mixer.4.Initial volume and mute status of applications is storedand upon exit will be restored.
Testing strategy:
Manual.
Edge cases tested:
MUTED
, then upon restart it will be switched toENABLED
.Known issues with pull request:
N/A
Code Review Checklist:
Summary by CodeRabbit
New Features
NVDA+alt+pageUp
), decreasing (NVDA+alt+pageDown
), and muting (NVDA+alt+delete
) the volume of other applications.Enhancements
Bug Fixes