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

Unbind application volume adjustment gestures #17634

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Jan 21, 2025

Link to issue number:

Closes #17272

Summary of the issue:

The application volume adjuster feature introduced in #16591 includes default keyboard gestures that:

  1. Conflict with the default gestures used by NVDA Remote (and thus Remote Access #17580); and
  2. Don't do anything by default, as the feature is disabled.

Description of user facing changes

These gestures are unbound by default. Users can still bind gestures to these commands from the input gestures dialog.

Description of development approach

Removed the gesture argument to the script decorator on GlobalCommands.script_increaseApplicationsVolume, GlobalCommands.script_decreaseApplicationsVolume, and GlobalCommands.script_toggleApplicationsMute.

Testing strategy:

Ran NVDA from source. Ensured that the commands were unbound by executing the previous keyboard gestures.
Re-bound the gestures in the input gestures dialog, and ensured they worked as expected.

Known issues with pull request:

None.

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.

@coderabbitai summary

@SaschaCowley SaschaCowley marked this pull request as ready for review January 21, 2025 01:36
@SaschaCowley SaschaCowley requested a review from a team as a code owner January 21, 2025 01:36
@SaschaCowley SaschaCowley requested a review from seanbudd January 21, 2025 01:36
@wmhn1872265132

This comment was marked as duplicate.

@SaschaCowley SaschaCowley requested a review from a team as a code owner January 21, 2025 03:51
@CyrilleB79
Copy link
Collaborator

No time to review this and re-read the discussions for now, but I strongly disagree with this PR and the reason why it exists. I'll write a more detailed reasoning on my point ov view if this PR is not yet merged before I have time to.

@Adriani90
Copy link
Collaborator

are there references in the user guide that need to be updated too?

Ofcourse, the whole feature doesn't work as explained in the user guide,
see the discussion #17124 which led to the discussion of redesigning profiles overall in #17415.

I also second @CyrilleB79, I don't think unbinding gestures is a good practice. Add-ons should follow NVDA's development and not vice versa. I'd rather suggest to add a warning when gestures of add-ons conflict with gestures in the core, so users can adjust the gesture for add-on's feature or core feature themselves as they wish. See #15242. In any case it makes sense to warn the user when an add-on overtakes an already assigned gesture, at least for security reasons.

@CyrilleB79 CyrilleB79 mentioned this pull request Jan 21, 2025
5 tasks
@CyrilleB79
Copy link
Collaborator

I'll comment here on the choice to unassign these gestures with which I disagree. For now I'll not comment on potential gesture conflicts with #17580 since I have commented the gesture choice there and unless NV Access finally decide to assign back gestures to app volume adjustment commands, this discussion would be irrelevant.

When volume app has been developed, as for many new popular feature, we have tried to make this feature known as much as possible so that all people needing it could discover it easily. Having this other apps volume adjustment feature enabled by default or even suppressing the option allowing to disable this feature had been considered. Unfortunately, as demonstrated by @LeonarddeR, enabling other app volume adjustment could cause incompatibility with other add-ons or applications that also manipulate the audio. People using another audio management tool (add-on or app) are a minority; though the consequences were too important (audio not restored at all) to allow the other app volume control to be enabled by default.

Still the feature has demonstrated to be already popular in @mltony's add-on and would help many more people who do not uses Tony's add-on.

I have the feeling that unassigning its gestures would transform this feature in a hidden feature or a feature dedicated only to well informed people. Instead, if we keep the current gestures (or assign other ones if rather needed), we could more easily communicate on it on mailing lists to tell people how to control volume or mute other apps.

If NV Access (@SaschaCowley or @seanbudd) sticks with keeping these gestures unassigned, at least could you please explain clearly that choice, i.e. why unassigning these gestures is preferred over assigning different gestures.
Thanks.

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.

Looks good

@SaschaCowley
Copy link
Member Author

If NV Access sticks with keeping these gestures unassigned, at least could you please explain clearly that choice, i.e. why unassigning these gestures is preferred over assigning different gestures.

  1. This feature is disabled by default, so these gestures are a no-op by default.
  2. The subset of users who are likely to use this feature is probably small, so having them bound just "wastes" gestures.
  3. Gestures are easier to add than they are to remove (psychologically speaking), so if there is a lot of demand for them to be added in future, we can do so.
  4. Binding these commands to gestures is not difficult, and is probably achievable by most NVDA users without assistance.

@SaschaCowley SaschaCowley merged commit b8cf4cf into master Jan 22, 2025
5 checks passed
@SaschaCowley SaschaCowley deleted the unbindAppVolGestures branch January 22, 2025 04:08
@github-actions github-actions bot added this to the 2025.1 milestone Jan 22, 2025
@Adriani90
Copy link
Collaborator

Well, with the current approach NVDA educates only a small subset of users to use it. It‘s because the feature is hidden behind some bareers, especially if you have many profiles. To be honnest, this feature is well wanted by the community, and is not a small subset of users that would benefit. But yeah, these are my few cents on it. I started enough discussions on this matter without any result, so I will not comment on this feature anymore. But as the feature is now, even reverting it entirely would not really cause any negative impact. I wonder why it should be part of the core anyway if it is hidden this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider changing or unbinding default application volume adjustment gestures
6 participants