Simplify application volume control

[codeofdusk]

Link to issue number:

Closes ##17124.
Closes #17272.
Supersedes #17317.
Supersedes #17321.

Summary of the issue:

• The default app volume gestures conflict with the popular NVDA Remote add-on.
• Binding gestures to a command by default then putting that functionality behind a feature flag doesn't feel in line with NVDA conventions.

Description of how this pull request fixes the issue:

• Unbind the app volume control gestures by default.
• Remove the app volume control feature flag.
• Move the app volume control state out of config and into module-level private variables.

Testing strategy:

Verified that all functionality works as expected:

• Started NVDA, and used the app volume control gestures to adjust application volume.
  • Volume adjustment works.
• Exited NVDA.
  • App volumes restored.
• Started NVDA, adjusted an app's volume using volume mixer, and exited.
  • App volume not modified.

Known issues with pull request:

None known

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.

[Adriani90]

@codeofdusk thanks for this great work and simplification. Indeed I don't think feature flag is needed for this. But yeah, this is my own opinion about feature flags outside of advanced pannel in NVDA in general.

Could you please generate a try build with this PR so we can test?
Are the controls in the audio settings pannel still available with this PR? This would be important to have because this would be consistent with voice parameter settings, and it might be important for users who want to keep the gesture unbinded but still use this feature.

[codeofdusk]

Could you please generate a try build with this PR so we can test?

I don't have push rights to the main repo, but you can create a self-signed build for personal use.

[codeofdusk]

Are the controls in the audio settings pannel still available with this PR?

For app volume control? No. The config settings have been completely removed.

[Adriani90]

Could you add them back please? The combo box could become a checkbox because there is no feature flag anynore.Von meinem iPhone gesendetAm 29.10.2024 um 00:10 schrieb Bill Dengler ***@***.***>: Are the controls in the audio settings pannel still available with this PR? For app volume control? No. The config settings have been completely removed. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

[SaschaCowley]

@codeofdusk while we are happy to remove the default gesture assignments for this feature, we are not willing to remove the configuration of this feature. Doing so is not acceptable as it interferes with other applications' volume in too many ways.

[codeofdusk]

@SaschaCowley the reasoning behind the feature flag in the original PR was to avoid NVDA's unintentional interference with externally set volumes (such as in Windows Volume Mixer). Since the feature is effectively disabled by default with the gestures unbound, the feature flag feels both redundant and confusing.

[codeofdusk]

One advantage to keeping the set volume level in config is to persist it across NVDA restarts, but this introduces the issues described by #17124.

[SaschaCowley]

I think this still may cause issues. What happens, for instance, if you adjust an application's volume in the sound mixer, then launch a fresh copy of NVDA?

[codeofdusk]

That'd cause issues with both this and the old approach though, right?

[codeofdusk]

What happens, for instance, if you adjust an application's volume in the sound mixer, then launch a fresh copy of NVDA?

@SaschaCowley I've tested this:

1. Using a build of NVDA latest master (without this PR), open volume mixer. Set Foobar2000 (audio player) volume to 4.
2. Start NVDA from source, with this PR. Observe that the Foobar2000 volume has not changed.
3. Exit the source copy and restart my installed copy. Observe that Foobar volume has not changed.

[CyrilleB79]

There had been a long discussion during the development of this feature with pros and cons on various aspects of the feature. And this PR seems to ignore some of them.

This PR is trying to address various issues:

• the definition of shortcuts for this feature (or the fact of unbinding them)
• the complexity of the feature with, before this PR, the "audio adjuster" option which may be a bit hard to understand.

IMO, this should be handled separately for clarity in the discussions.

Regarding unbinding of the shortcut keys, I am still expecting more details from NV Access on why such a proposal has been accepted.

Regarding the complexity of the feature with the "volume adjuster" option, according to the discussion during the development of the feature, it seemed to me that it was mandatory to cover all use cases. If the simplification in this PR is accepted by NV Access as a first approach, I'd request a double-check from @LeonarddeR and @mltony. With the new design in this PR, isn't there the (undocumented) risk to assign the unbound commands and to mess up other applications or add-ons using volume controls?

[SaschaCowley]

@codeofdusk we cannot accept this PR as it stands. As @CyrilleB79 has pointed out (in #17335 (comment)), this PR disregards a lot of discussion that went in to this feature. Furthermore, we are unwilling to accept removal of the enabled/disabled option for this feature, as having this feature enabled, even if it seems to be doing nothing, may interfere with add-ons or other applications that modify volume levels. See, for instance, #16287.