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

Fixed text for allow NVDA to control the volume of other apps #17317

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Oct 22, 2024

Link to issue number:

Fixes #17303

Summary of the issue:

Associated to the audio new option "Allow NVDA to control the volume of other applications" (ex Volume adjuster), there is a command (script) with no default bound gesture to toggle the value of this parameter. There are two issues with this script:

  • Using it only reports "Yes" or "No", whereas other toggle scripts are usually more precise, e.g. the command "Toggles on and off the inclusion of layout tables in browse mode" reports "layout tables off" or "layout tables on".
  • The input help or command name in the input gestures dialog still mentions volume adjuster, whereas the option in the audio settings and in the User Guide has been reworded.

Description of user facing changes

  • Reworded the command name / input help.
  • Calling the command now reports something more explicit.

Description of development approach

See code.

Testing strategy:

Manual test.

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

@CyrilleB79
Copy link
Collaborator Author

@XLTechie, would you mind reviewing the wording changed in this PR? Maybe what is reported when using the toggle command is a bit long, but I have not found something shorter.

Also cc @wmhn1872265132 (author of #17303)

@CyrilleB79 CyrilleB79 marked this pull request as ready for review October 22, 2024 21:02
@CyrilleB79 CyrilleB79 requested review from a team as code owners October 22, 2024 21:02
Copy link
Collaborator

@XLTechie XLTechie left a comment

Choose a reason for hiding this comment

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

@CyrilleB79 Here are some English wording suggestions. Note that I have not been following all of the intricacies of this feature. But these more closely represent in English, what I think was intended.
I have given two alternatives for each message (action word first, action word last), and have also given two slightly different wordings (one set in the enabled suggestions, and one set in the disabled suggestions), that you can of course adjust to your preference.

HTH.

source/audio/appsVolume.py Outdated Show resolved Hide resolved
source/audio/appsVolume.py Outdated Show resolved Hide resolved
source/audio/appsVolume.py Outdated Show resolved Hide resolved
source/audio/appsVolume.py Outdated Show resolved Hide resolved
source/audio/appsVolume.py Outdated Show resolved Hide resolved
source/audio/appsVolume.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
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 and UI text looks fine.

@CyrilleB79
Copy link
Collaborator Author

Many thanks @XLTechie for your review. I have committed your suggestions and/or reworded taking into account of them. Should be OK now.

Copy link
Collaborator

@XLTechie XLTechie left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Some issues after #17271
3 participants