Skip to content

Fix-up of the toggle apps volume control command#17885

Closed
CyrilleB79 wants to merge 1 commit into
nvaccess:masterfrom
CyrilleB79:appsVolCtrl
Closed

Fix-up of the toggle apps volume control command#17885
CyrilleB79 wants to merge 1 commit into
nvaccess:masterfrom
CyrilleB79:appsVolCtrl

Conversation

@CyrilleB79

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #17882

Summary of the issue:

The toggle command for apps volume control does not report its state after having performed the switch.
Also, while fixing this, I have realized that some state comparison were not performed as expected: confusion between volume control state (enabled/disabled) vs config state (enabled/disabled/default)

Description of user facing changes

The command now works as expected.

Description of development approach

  • Changed variable names and added type hints to distinguish between config state and real apps volume control state
  • Calculate the config state when needed

Testing strategy:

Manual tests:

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

@mltony, @LeonarddeR, @ABuffEr, @codeofdusk:

Since you were interested or impacted by this feature in some way, could you please perform additional user tests, in case I have forgotten one case to test.
Feel free to cc other people that may be interested or impacted.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review April 2, 2025 20:40
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner April 2, 2025 20:40
@CyrilleB79 CyrilleB79 requested a review from SaschaCowley April 2, 2025 20:40
@seanbudd

seanbudd commented Apr 3, 2025

Copy link
Copy Markdown
Member

Closing - we are reverting apps volume command due to #17654

@seanbudd seanbudd closed this 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.

NVDA does not announce when application volume control feature is toggled using assigned gesture

2 participants