Revert "Keystrokes to adjust applications volume and mute" #16440
Conversation
This reverts commit 275ab4d.
|
@seanbudd in the expectation that PR templates should be filled properly, could you please take the example role you have on the community and explain the reasons? |
|
@Adriani90 @mltony I've updated the description of the revert PR to make our thinking a bit more explicit. If you (or anyone else in the community) would like to continue the conversation please feel free to open a discussion thread. Your hard work and dedication to improving NVDA is greatly appreciated and what makes this community so valuable. |
|
@gerald-hartig, @seanbudd
|
seanbudd
added
the
conceptApproved
|
@mltony We'd be happy with the PR if it addressed those 5 things. |
|
@gerald-hartig, @mltony I am struggling trying to get why the bar is set higher for this feature in one aspect:
This is not what we have in NVDA. Try as follows:
Currently, the volume mixer does not reflect the volume adjustments done by the audio ducking feature. I think it really makes sense to accept this feature only if it is an NVDA internal change, without any reflection in the Windows mixer. |
|
@gerald-hartig, @seanbudd, as you wrote, there has been a "considerable debate on the approach". @mltony has asked a confirmation about 5 design choices in #16440 (comment).
For point 1, I would add that not only apps volumes but also mute state should be reflected in the Windows Mixer. I have the feeling that our discussions on this topic is quite inefficient because it is scattered in many GitHub issues and PRs. Before going again forward with a PR, we really should have a centralized discussion on this feature with designed choices not only agreed by NV Access but also explained. Among others, the following questions on the mute and apps volume change feature should be answered with justification:
I won't answer here for myself before we know where all this should be discussed but will do as soon as the discussion location is clarified. It would be nice that either @mltony or NV Access indicate where this discussion should take place and initiate it because you are more legitimate. But if no one does, I may do it myself. |
You seem to totally ignore my comments. This approach is not what we have in NVDA currently. So if NV Access decides to reflect the changes in the windows mixer, then this should be changed for audio ducking as well.
Again, @CyrilleB79 and also @mltony you totally ignored my comments. Neither apps volume nor mute / unmute state should be reflected in the mixer. All of them should be restored after NVDA exit and should remain a NVDA internal change. That's what we have since years with all audio related features, e.g. audio ducking.
Offcourse it is useful, at least as useful as audio ducking.
In my opinion these should never touch the volume mixer at all.
Vomume mixer should not be involved at all. And questions 4-6 are obsolete in this case. |
|
For what its worth, I completely second the expectations as raised by @gerald-hartig. At a certain point we really need to respect choices made by NV Access, without continuing to engage in endless discussions. @Adriani90 Comparing the audio ducking feature with lowering app volume is the same as comparing apples and oranges. Audio ducking is a feature which is completely separate from the volume mixer, namely a Windows accessibility feature. More importantly though, the ducker feature is bound to the application window and even when NVDA is forcefully killed with the taskkill command, ducking is disabled. If NVDA fiddles with the volume of any application, it should do so in a standardized way that is reflected by the volume mixer. If not, a sighted user is unable to fix volume changes after a force shutdown of NVDA. |
|
@Adriani90, I have read your comments but I still disagree with it. |
Thanks for this qualified answer, very appreciated. I didn't know audio ducking is a native windows accessibility feature separated from all other volume related things. I thought NVDA found a way to avoide the volume mixer through other internal adjustments. Sometimes a qualified technical answer to some comments would really reduce the amount of noise in such discussions and people like me would feel more seriously taken. |
user_docs/en/userGuide.t2t
Outdated
| Please note, that if NVDA crashes, then it won't be able to restore application sounds volume, and those applications might still output sound only in one channel after NVDA crash. | ||
| In order to mitigate this, please restart NVDA. |
There was a problem hiding this comment.
@mltony - should this section also be kept - i.e. is the issue surrounding sound split being stuck after a crash still an issue?
There was a problem hiding this comment.
Yes, this section should be kept. There is nothing we can do to restore volumes of apps when NVDA crashes.
|
@LeonarddeR Tony wrote:
This is revering to sound split feature. |
|
@Adriani90, I wrote;
So yes, we can be sure that the volume is restored, since it's not NVDA's, but Windows' responsibility to do so. |
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 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. Applications' voluem adjusting logic is implemented in audio/appsVolume.py and is quite straightforward now. 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. Applciation volume adjustment is now completely independent of sound split and both features can operate independently of each other.
Reverts PR
Reverts #16273
Issues fixed
Fixes #16409
Fixes #16402
Issues reopened
#16052
Brief reason for revert
We started with mltony creating:
#16051 Feature request: Sound split
Which was a duplicate of:
#12985 Audio settings with stereo headset (or speakers) - Send NVDA sounds on one side and the rest of Windows sounds on the other side
And then implemented by:
#16071 Sound split
mltony also created:
#16052 Feature request: add command to adjust volume of all applications except for NVDA
Which was implemented in:
#16273 Keystrokes to adjust applications volume and mute
This PR was approved and merged and was then found to cause issues:
#16402 Unmuting other apps does not work as expected
#16409 Apps mute and volume features work very unexpectedly with WASAPI disabled
Due to these issues and the considerable debate on the approach, the above PR #16273 was reverted by:
#16440
As an alternative to the revert #16440 to resolve the 2 issues (#16402, #16409) and keep #16273, mltony created:
#16404
The question now becomes, how do we proceed from here?
NV Access's position is that the sound split functionality (#16051) is a useful feature to add into core. However, due to the following reasons, we believe that further work on the volume adjustment features (#16273, #16404) to improve the UX is required on a branch (off master/alpha) before it can be added back in:
As one of the contributors on the threads said, "So now there are two mixers in the chain, one of which can be invisible, and overrides the other, or makes its settings relative instead of absolute."