Skip to content

Unbind application volume adjustment gestures#17634

Merged
SaschaCowley merged 3 commits into
masterfrom
unbindAppVolGestures
Jan 22, 2025
Merged

Unbind application volume adjustment gestures#17634
SaschaCowley merged 3 commits into
masterfrom
unbindAppVolGestures

Conversation

@SaschaCowley

@SaschaCowley SaschaCowley commented Jan 21, 2025

Copy link
Copy Markdown
Member

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
Comment thread source/globalCommands.py
@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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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.

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

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

@CyrilleB79

Copy link
Copy Markdown
Contributor

@SaschaCowley, I am quite surprised and a bit disappointed by some of the reasons provided here. During the development of this feature, there were extensive discussions where NV Access appeared receptive to the benefit of defining default gestures to promote the feature.

Also, it seemed to me that this feature had been integrated to core because of the popularity of this feature, that is because a significant subset of users was expected. Could you clarify if NV Access' perspective on the expected user base for this feature has changed?

Also, even if it does not appear in your last answer, I feel that this unassignment has been mainly pushed, at least initially, by the conflict with NVDA remote's gestures (and/or its arrival in core), which is a very bad reason for making such a decision because assigning gestures to one or the other feature should be unrelated as explaned before in this thread.

To be more specific on the reasons you provided:

  1. The subset of users who are likely to use this feature is probably small, so having them bound just "wastes" gestures.

As written above, @Adriani90 and I on one hand and you on the other hand do not share the same perspective on the
target audience for this feature. @mltony's add-on is already used by a significant subset of users and other audio management add-ons are used too. And @Adriani90 and I estimate that the feature will be used by a significant amount of users if correctly integrated in core and promoted.
Now we probably have to see what happens in the future to evaluate the actual target audience.

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

This point is valid.
So I understand that you are probably not very confident if the feature will be widely adopted or not and expect to see what happens. But of course, this creates a chicken-and-egg situation: if the feature lacks default gestures, it may not be promoted effectively and thus might see limited adoption."

  1. This feature is disabled by default, so these gestures are a no-op by default.

It's true that the fact that the feature is disabled by default was not desirable and it degrades the UX, but no other viable solution was found.

Though, I expect the users to divide in 2 categories:

  1. Those using this feature who will enable it and save it in config once and for all
  2. Those not using this feature, either because they do not need to control other apps volumes or because they do it otherwise (e.g. thanks to an add-on). For them, the gestures is a no-op, but they will never use it in any case, so it's as it the gesture was just "wasted" (unused gesture assigned by default).

I.e. I do not expect many users to switch frequently "Allow NVDA to control the volume of other applications" parameter.

Users of group 1 will use the gestures, and users of group 2 will not. What is the difference with a feature such as screen curtain: users of this feature use the gesture and users who never use this feature do not use it; for people never using the screen curtain gesture, the screen curtain gesture is "wasted".

  1. Binding these commands to gestures is not difficult, and is probably achievable by most NVDA users without assistance.

I really think this argument is not a strong reason for avoiding default gestures: this rational can apply to any command and gesture so it should have no impact on deciding if a command should have a default gesture or not. Why would it apply more specifically to one feature than another?
The decision to include default gestures should instead be based on:

  1. if it is expected to be used frequently
  2. if we want to promote a command that we consider useful for a significant user base.

To conclude, I have the same feeling as @Adriani90: I wonder what was the point in integrating this feature in core if it's estimated to target only a small subset of users and if it's not promoted at all? Finally why keeping the add-on wouldn't have been a better solution (integrating extension points in core if needed)?

I have shared my remaining questions, but on the other hand, I also know that we cannot discuss the same subject forever...

Finally, I would also like to cc @gerald-hartig: During the development of this feature, I felt there were inefficiencies caused by back-and-forth discussions. I hope we can streamline similar processes in the future to save time and effort.

@cary-rowen

Copy link
Copy Markdown
Contributor

I completely agree with @CyrilleB79’s comment, and I’d like to discuss it from two perspectives.

  1. The subset of users who are likely to use this feature is probably small, so having them bound just "wastes" gestures.

This seems more like speculation. We don’t actually have a reliable way to gauge how many users might benefit from this feature. Moreover, if it’s hidden, its discoverability drops even further. I recently received real feedback from users in China: many video websites there have poor accessibility, and a video may start playing at very high volume. At that point, screen reader users often struggle to find the pause button or other playback controls, which can be quite challenging. Consequently, they either switch audio outputs or close the page outright. However, with this new feature, they can adjust the volume for other applications. I won’t go into detail about the usefulness of this feature—we already discussed that during development—but I do feel that this argument lacks solid data. It comes across more like a justification for a predetermined stance.

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

I disagree. From the standpoint of the steps required, removing a gesture is clearly simpler. Owing to certain flaws in NVDA’s gesture-assignment process, adding a gesture isn’t always straightforward. In fact, some users have unintentionally assigned the Enter key or arrow keys to a function and then had no idea how to revert them. This points to a broader usability issue with NVDA, but it also shows that adding gestures can be problematic for some people.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

I recently received real feedback from users in China: many video websites there have poor accessibility, and a video may start playing at very high volume.

Most browsers, including Firefox and Edge, have a mute toggle that is easily accessible with ctrl+m. So I'd leave this argument out of the discussion.

2. The subset of users who are likely to use this feature is probably small, so having them bound just "wastes" gestures.

Can we substantiate this assumption on real data, i.e. download count from the add-on store of Tony's add-on?

@CyrilleB79

Copy link
Copy Markdown
Contributor

I recently received real feedback from users in China: many video websites there have poor accessibility, and a video may start playing at very high volume.

Most browsers, including Firefox and Edge, have a mute toggle that is easily accessible with ctrl+m. So I'd leave this argument out of the discussion.

I knew the ctrl+M mute shortcut in Firefox for a long time; is the one in Chrome more recent? I didn't know of it but it's a good news, especially because this shortcut has thus become a standard in browsers.

Though, there are still use cases of audio playing too high with respect to NVDA sound: video conference call systems, multimedia players, etc. The feature is still valid for these other cases.

@gerald-hartig

gerald-hartig commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

Thanks for the discussion on this pull request. We appreciate the time and thought everyone has put into providing feedback, especially @CyrilleB79, @Adriani90 and @cary-rowen.

We acknowledge the points raised and apologise if merging this PR has felt like a misstep. We're currently managing numerous priorities, and our primary goal is always to ensure a stable and high-quality release for our users.

Generally speaking, if we determine that merging a PR was premature or incorrect, we are absolutely prepared to revert the changes.

Regarding the core concern about unbinding the default gestures and feature discovery, our current position is guided by a few key principles and the context of the volume adjustment feature:

  • The primary mechanism for feature discovery should be through documentation such as the changelog for each release, in-depth explanations within the user guide and In Process for major features. Assigning default gestures can contribute to feature visibility, but we don't consider it the primary or most reliable method for user education and feature adoption.
  • The bar for assigning default gestures to new features is intentionally set high. This bar includes considerations such as:
    • Enabled by default for immediate usability. The application volume adjustment feature is currently disabled by default, meaning the default gestures would be non-functional for the majority of users out-of-the-box.
    • Avoiding conflicts with existing popular add-ons or widely used keyboard shortcuts. NVDA Remote is a feature many users rely upon.
    • Maturity and stability. The application volume adjustment feature is still relatively new and, as @LeonarddeR and others have pointed out, is not yet fully configuration profile independent. This indicates development required, potentially even removal if significant issues arise.

Given this we believe that unbinding the default gestures at this stage is a pragmatic decision that avoids potential user confusion or frustration with non-functional gestures. At this stage, in alpha, it's highly unlikely that unbinding these gestures will negatively impact users who are already relying on them. We’ll ensure the feature is properly documented in the changelog for the upcoming release and highlighted elsewhere where appropriate. Users who are interested in this functionality will be able to discover it through these standard channels and easily bind their own preferred gestures via the input gestures dialog.

To address @CyrilleB79's question about the rationale for integrating the feature into core if it's not being actively promoted with default gestures: including it in core is to provide functionality to users who need it (recognising its potential value by the popularity of add-ons providing similar features). We're open to re-evaluating the decision regarding default gestures in future releases.

Regarding @Adriani90's suggestion for a system to handle gesture conflicts - we agree that this is an important piece of work. Let's open a separate discussion on the design and implementation of this feature, taking into account user-defined gestures, add-on defined gestures, and the complexities of module and add-on loading order.

@CyrilleB79

Copy link
Copy Markdown
Contributor

@gerald-hartig, many many thanks for this detailed clarification of NV Access position on all the points that have been raised. Even if we do not reach the same conclusion on all points, all what was written makes sense.

I would still comment on two points:

The primary mechanism for feature discovery should be through documentation such as the changelog for each release, in-depth explanations within the user guide and In Process for major features.

Please note that In process is only an English-speaking channel. It's a content of high quality, and I really appreciate it, but non-English speaking persons do not usually read it (even if browsers are now able to translate webpages on demand). So I'd not list it as part of the primary mechanism for feature discovery.

More generally, please keep in mind that In process only addresses a limited subset of NVDA users.

  • Avoiding conflicts with existing popular add-ons or widely used keyboard shortcuts. NVDA Remote is a feature many users rely upon.

Okay. So, this appears to be a change in position from NV Access. Historically, extension authors have always been asked to modify the gestures they use in their add-ons when new conflicting gestures were introduced in NVDA, even when the add-on was popular.

Also, I'd be curious to know how much popular NVDA remote (or TeleNVDA) is in comparison to other add-ons. Has NV Access download stats from the Add-on Store? Currently, there is also a survey regarding add-on usage. Unfortunately, it mainly targets English-speaking persons, since it is not translated; and the usage statistics likely vary significantly from community to community. But I hope that it will at least give a rough idea of the popularity of add-ons.

@CyrilleB79

Copy link
Copy Markdown
Contributor

@LeonarddeR just to be factual:

I recently received real feedback from users in China: many video websites there have poor accessibility, and a video may start playing at very high volume.

Most browsers, including Firefox and Edge, have a mute toggle that is easily accessible with ctrl+m. So I'd leave this argument out of the discussion.

If I'm not mistaken, Chrome is by far the most used browser, and I have checked that ctrl+M is not available there unfortunately.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

@CyrilleB79 I was probably too quick in assuming that ctrl+m was a Chromium gesture, since it is available in Edge.

@nvdaes

nvdaes commented Jan 23, 2025

Copy link
Copy Markdown
Collaborator

Cyrille wrote:

Also, I'd be curious to know how much popular NVDA remote (or TeleNVDA) is in comparison to other add-ons.

Downloads are not an exact way to know add-ons popularity, since for example some of them need to be updated more often or have more updates.
Also, of course an feature not very popular may be even more needed than popular features, if for example is just popular in academic centers or in specific spaces.
Anyway, years ago I created a form using GitHub API and Jquery to count add-ons downloaded from GitHub. I haven't reviewed the log to see if it's the same created by me, but you may want to try it, in case you don't know this:

https://nvdaaddons.github.io/

@gerald-hartig

gerald-hartig commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

@CyrilleB79 it looks like we could do more in promoting new features to the non-english speaking community. Do you have any suggestions for ways that we can better bring NVDA's new features to non-english speaking communities?

We have on our roadmap plans to gather telemetry on add-on usage to assist these kinds of discussions, but this is still some distance off. @nvdaes thanks for that link I'll have a look.

@CyrilleB79

Copy link
Copy Markdown
Contributor

@CyrilleB79 it looks like we could do more in promoting new features to the non-english speaking community. Do you have any suggestions for ways that we can better bring NVDA's new features to non-english speaking communities?

Unfortunately no ideal solution. Today, the interesting information go through communities in an informal way, e.g. through mailing lists or sometimes local websites. Either some people connected to the English-speaking media provide some information, or people sometimes share their experience from scratch without any previous link to NV Access source.

In-process blog is quite interesting. But not sure that we can ask it to be translated each time. Sometimes, I provide a French translation, to French community (throug mailing list) especially for surveys. Maybe an IA translation could be acceptable if we provide NVDA locale documentation to help the IA use the correct terms? Browsers can already translate webpage, but the specific vocabulary of NVDA is not always honoured.

Or maybe your question may be the topic for the next survey?!

@Adriani90

Adriani90 commented Jan 26, 2025

Copy link
Copy Markdown
Collaborator

@gerald-hartig thanks very much for your detailed and valuable response on this. I have just following cents to add, maybe these points can be considered when deciding on UX design and deciding on priorities before merging a PR.

You wrote:

• The bar for assigning default gestures to new features is intentionally set high. This bar includes considerations such as:
◦ Enabled by default for immediate usability. The application volume adjustment feature is currently disabled by default, meaning the default gestures would be non-functional for the majority of users out-of-the-box.

The fact that this feature is disabled by default should not have per se an impact on key gestures. NVDA could report "volume adjuster is curently disabled" when pressing the coresponding hotkeys.

◦ Avoiding conflicts with existing popular add-ons or widely used keyboard shortcuts. NVDA Remote is a feature many users rely upon.

Imo this should never be a bar set to input gestures in NVDA core. Remote feature is used by advanced screen reader users, and they might have loud voices. Volume adjustments is a feature that targets much lower experience levels as well, and thus much more users are targeted by this feature.

It makes sense in my opinion to let input gestures unassigned for features that tharget advanced users, and provide input gestures by default for features that target users with lower experience level.
Otherwise we will have to assign tons of input gestures on the initial setup of NVDA in order to use it efficiently.

◦ Maturity and stability. The application volume adjustment feature is still relatively new and, as @LeonarddeR and others have pointed out, is not yet fully configuration profile independent.

In my view this was a reason not to merge #16591, until the feature is profile independent. I am really surprised that this has been merged without having this in mind, which indeed didn't really fix the original request in #16052 which clearly requested a profile independent solution. This has also led to wrong documentation in the user guide.

including it in core is to provide functionality to users who need it (recognising its potential value by the popularity of add-ons providing similar features).

This feature was not an add-on before, so actually it was even not stable enough for being included into the core. If @mltony issued an add-on first, we would have noticed at that stage that it is profile independent and probably it would have never been proposed as a PR without solving that part.

seanbudd added a commit that referenced this pull request Apr 3, 2025
SaschaCowley pushed a commit that referenced this pull request Apr 4, 2025
### Reverts PR
Reverts #17271
Reverts #16591
Reverts #17634

### Issues fixed
Fixes #17654
Fixes #17882
Fixes #17124
Fixes #17656

### Issues reopened
Reopens #16052

### Reason for revert
- #17654: The applications volume adjust feature is affecting Windows
sound output even if the feature is disabled.
- #17882: bug: NVDA does not announce when application volume control
feature is toggled using assigned gesture
- #17124: unclear UX in regards to if the settings should be profile
independent
- #17656: NVDA doesn't control the volume of applications which use
non-default windows output

### Can this PR be reimplemented? If so, what is required for the next
attempt
- Refer to debug suggestions in
#17654 (comment)
- Ensure #17124 is considered in the next attempt
- See #17885 for fix for #17882
- #17656 should at least be documented, if not resolved
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

10 participants