Keystrokes to adjust applications volume and mute [take 2]

[mltony]

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 states
  • Disabled (default)
  • Enabled
  • Muted - all applications except NVDA will be muted

Description of development approach

1. I refactored sound split code. Sound split and applications volume adjustment need to use same callbacks in a similar fashion. In order to reduce code reduplication, I extracted common logic and put it in audio/utils.py. Now it provides an abstract class AudioSessionCallback and customers (sound split and app volume adjuster) are implementing this class. Two methods that need to be implemented are onSessionUpdate and onSessionTerminated, which operate on a single audio session. All the plumbing is hidden in utils.py so that clients don't need to worry about setting up and dealing with multiple lower-level callbacks.
2. Applications' voluem adjusting logic is implemented in audio/appsVolume.py and is quite straightforward now.
3. Volume and mute status are adjusted via 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.
4. Applciation volume adjustment is now completely independent of sound split and both features can operate independently of each other.

Testing strategy:

Manual.
Edge cases tested:

1. Application volume adjustment works regardless of sound split state.
2. Application volume and mute status is correctly reflected in Windows Volume Mixer.
3. Application volume andmute status are remembered at startup and restored upon exit.
4. If an application quits while NVDA is running, its volume and mute status are restored.
5. Application volume is preserved across NVDA restarts. If application volume adjuster status was MUTED, then upon restart it will be switched to ENABLED.

Known issues with pull request:

N/A

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

Summary by CodeRabbit

• New Features

  • Introduced functionality for adjusting and managing the volume of other applications.
  • Added new keyboard shortcuts for increasing (NVDA+alt+pageUp), decreasing (NVDA+alt+pageDown), and muting (NVDA+alt+delete) the volume of other applications.

• Enhancements

  • Updated audio settings panel with new controls for application volume adjustment.
  • Added user guide sections explaining new volume adjustment features and settings.

• Bug Fixes

  • Improved the initialization and termination processes for audio components, enhancing overall stability.

[Adriani90]

@mltony really very inspiring that you are still motivated to find a more bullet proof solution here, very nice work.
Two questions from my side:

1. Just to make sure, is the muted state of all applications restored after NVDA exit? Or is only the volume restored?
2. Why did you add the disabled state here to the nvda+alt+delete command? If everything is restored after NVDA exit, we don't need a completely disabled state in this feature. Or am I missing something?

[mltony]

1. Both Mute and volume are restored.
2. With regards to sound split, there was a strong push by @LeonarddeR to have a proper disabled option. I would think his arguments would apply here. For example an add-on might change volume of applications in a custom way. But if everyone comes to a conclusion that disabled state is not needed - I would be happy to remove it.

[LeonarddeR]

Glad to see that this is implemented in such a way that it doesn't have impact by default.

[seanbudd]

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

[seanbudd]

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.

[cary-rowen]

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.

[mltony]

making 2024.2 API changes private in beta ASAP.
That makes sense to me. Do I just prefix them with underline character? I'll prepare a PR today.
The new approach is much more elegant and I'd think it would be more convenient for add-on authors to make use of. And with my busy schedule I can't promise that I'll have enough time in the end of the year to work on v2025 version.

[CyrilleB79]

@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.

[CyrilleB79]

@wmhn1872265132 wrote:

But it seems a little inappropriate to assign three shortcut keys to a function that is not available by default, which is the main reason why I think it is necessary to keep the default shortcut keys for enable/disable.

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.

[wmhn1872265132]

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

[mltony]

I assigned NVDA+alt+delete to mute command and left "toggle application volume adjuster" unassigned - as this seems to be the consensus by the majority now. LMK if you want me to do any other tweaks.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/7njxfg347jh3de19/artifacts/output/nvda_snapshot_pr16591-33421,1ec6e99a.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.2,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 11.5,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 18.4,
  FINISH_END 0.2

See test results for failed build of commit 1ec6e99a9d

[Qchristensen]

Good work, and good luck with take 2 :)

[seanbudd]

Thanks @mltony !

[CyrilleB79]

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!

[codeofdusk]

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.
  • This is especially underscored by the fact that NVDA+Alt+pgup/pgdn are used by the very popular NVDA Remote add-on. This might complicate NVDA Remote Control #4390.

[Adriani90]

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: ***@***.*** ***@***.***> >

[codeofdusk]

Also, in English you'd say "application volume adjuster", not "applications".

[seanbudd]

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.

[mltony]

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:

If you want to emphasize that the feature adjusts the volume of all running applications at once, then "Applications Volume Adjuster" might be a better choice. This name suggests that the feature is capable of adjusting the volume of multiple applications simultaneously, which is a key aspect of its functionality.

Is ChatGPT wrong?