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

Revert "Keystrokes to adjust applications volume and mute" #16440

Open
wants to merge 3 commits into
base: beta
Choose a base branch
from

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Apr 23, 2024

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:

  • Windows sound mixer has reasonable accessibility.
  • Sound split on its own provides value to users.
  • The UX of swapping between NVDA volume control and windows volume control needs to be resolved.
  • The UX of resolving volume issues due to NVDA crashes needs to be resolved.

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

@seanbudd seanbudd requested review from a team as code owners April 23, 2024 05:12
@seanbudd seanbudd added this to the 2024.2 milestone Apr 23, 2024
@seanbudd seanbudd changed the base branch from master to beta April 23, 2024 05:14
gerald-hartig
gerald-hartig previously approved these changes Apr 23, 2024
@Adriani90
Copy link
Collaborator

@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?
In #16404 Tony invested time and ressources to fix the problems reported, please explain why that is not accepted.
cc: @jcsteh, @michaelDCurran, @LeonarddeR.

@gerald-hartig
Copy link
Collaborator

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

@mltony
Copy link
Contributor

mltony commented Apr 29, 2024

@gerald-hartig, @seanbudd
To make sure my understanding is correct, can you confirm that you'd accept the next version of PR if:

  1. Application volume when adjusted by NVDA keystrokes is reflected in Windows Mixer.
  2. Application volume and mute commands work regardless sound split being on or off.
  3. Application volume and mute status is restore upon exit.
  4. Application volume, but not mute status is persistent across NVDA restarts. I believe in the previous discussion someone insisted on not muting apps at startup even if they were muted in the previous session.
  5. When apps volume is 100% and mute is off, then this feature won't make a single wasapi call in order to let third party add-ons or applications to work correctly.
    Am I missing any other concerns? It would be good to agree on precise list of requirements since this feature provokes so lengthy discussions.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 30, 2024
@gerald-hartig
Copy link
Collaborator

@mltony We'd be happy with the PR if it addressed those 5 things.

@Adriani90
Copy link
Collaborator

@gerald-hartig, @mltony I am struggling trying to get why the bar is set higher for this feature in one aspect:

  1. Application volume when adjusted by NVDA keystrokes is reflected in Windows Mixer.

This is not what we have in NVDA. Try as follows:

  1. Set audio ducking on an app to always via nvda+shift+d key (e.g. in Firefox)
  2. Start a youtube video
  3. Use NVDA to navigate so that the volume of the youtube video is lowered
  4. Check the volume mixer to see the volume in firefox.

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.
I think then the implementation is less complex if this is NVDA internal only, and does not depend on any major design changes to the volume mixer which Microsoft might plan in the future.

@CyrilleB79
Copy link
Collaborator

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

  1. Application volume when adjusted by NVDA keystrokes is reflected in Windows Mixer.
  2. Application volume and mute commands work regardless sound split being on or off.
  3. Application volume and mute status is restore upon exit.
  4. Application volume, but not mute status is persistent across NVDA restarts. I believe in the previous discussion someone insisted on not muting apps at startup even if they were muted in the previous session.
  5. When apps volume is 100% and mute is off, then this feature won't make a single wasapi call in order to let third party add-ons or applications to work correctly.

For point 1, I would add that not only apps volumes but also mute state should be reflected in the Windows Mixer.
And I disagree with point 3 and 4 as explained here and there in the various discussions; changing the parameters in Windows Mixer when NVDA starts up or exit is quite a bad UX IMO and moreover, having a different behaviour for mute and apps volume at NVDA startup would make things more confusing.
Should I expand on this here again or elsewhere?

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:

  1. Is this feature (apps volume and mute) required for SoundSplit to be useful?
  2. Should the feature impact Windows Volume Mixer (volume sliders and mute buttons) or not?
  3. Should NVDA be allowed to control Windows Volume Mixer on startup / exit or only when keystrokes are used?
  4. Do we accept or not that volume and mute commands do not act at the same level, e.g. internal muting in NVDA but volume changes reflected in the Mixer?
  5. If the commands act on Windows Volume Mixer, are the controls in NVDA's audio panel still needed?
  6. Is the feature working as expected even in case of NVDA crash? If not is there a workaround?

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.

@Adriani90
Copy link
Collaborator

@CyrilleB79

For point 1, I would add that not only apps volumes but also mute state should be reflected in the Windows Mixer.

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.
But then we introduce two major risks:

  1. More complexity in the implementation and
  2. NVDA will depend stronger on designs of volume mixer which might change in the future, resulting in this feature to fail.
    So I really advise to not reflect these changes in the windows mixer.

And I disagree with point 3 and 4 as explained here and there in the various discussions; changing the parameters in Windows Mixer when NVDA starts up or exit is quite a bad UX IMO

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.
You also ignore the use case of people with hearing problems using the machine together with visually impaired people.

  1. Is this feature (apps volume and mute) required for SoundSplit to be useful?

Offcourse it is useful, at least as useful as audio ducking.

  1. Should the feature impact Windows Volume Mixer (volume sliders and mute buttons) or not?

In my opinion these should never touch the volume mixer at all.

  1. Should NVDA be allowed to control Windows Volume Mixer on startup / exit or only when keystrokes are used?

Vomume mixer should not be involved at all.

And questions 4-6 are obsolete in this case.

@LeonarddeR
Copy link
Collaborator

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.

@CyrilleB79
Copy link
Collaborator

@Adriani90, I have read your comments but I still disagree with it.
However, as explained in my previous comment, I am not sure that this PR is the right location to discuss it; that's why I do not expand here on why I disagree with you.

@Adriani90
Copy link
Collaborator

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.

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.

Comment on lines 2007 to 2008
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.
Copy link
Member Author

Choose a reason for hiding this comment

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

@mltony - should this section also be kept - i.e. is the issue surrounding sound split being stuck after a crash still an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this section should be kept. There is nothing we can do to restore volumes of apps when NVDA crashes.

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

User Guide changes (reverts) are fine.

@Adriani90
Copy link
Collaborator

@LeonarddeR Tony wrote:

Yes, this section should be kept. There is nothing we can do to restore volumes of apps when NVDA crashes.

This is revering to sound split feature.
How is it with audio cucking set to !always"? Can we be sure that NVDA restores the volume after exit? Even if it is an accessiility feature in Windows, it is still not reflected in the volume mixer. So if audio ducking is not able to restore volume after NVDA crash, we would ent up with the same problem you describe, that means that sighted people would not be able to help in restoring the volume of apps. In this case it might not be that bad because audio ducking is not lowering the volume that much, but in theory we have the same problem there I guess.

@LeonarddeR
Copy link
Collaborator

@Adriani90, I wrote;

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.

So yes, we can be sure that the volume is restored, since it's not NVDA's, but Windows' responsibility to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. queued for merge
Projects
None yet
7 participants