Skip to content

Various fix-ups for the application volume adjustment feature #17271

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Oct 17, 2024
Merged

Various fix-ups for the application volume adjustment feature #17271

merged 15 commits into from
Oct 17, 2024

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Oct 9, 2024 •
edited
Loading

Link to issue number:

#16591 (comment), #16591 (comment)

Summary of the issue:

  • The user guide is unclear on the drawbacks of enabling application volume adjustment.
  • Various grammatical and stylistic issues.

Description of how this pull request fixes the issue:

Updates what's new, the user guide, and various namings accordingly, per offline discussion with @mltony.

Testing strategy:

Tested that feature works as expected. Alpha testing.

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.

@codeofdusk codeofdusk requested review from a team as code owners October 9, 2024 20:37
Qchristensen
Qchristensen approved these changes Oct 10, 2024
View reviewed changes
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.

Userguide change looks fine.

CyrilleB79
CyrilleB79 reviewed Oct 10, 2024
View reviewed changes
Copy link
Contributor

@CyrilleB79 CyrilleB79 left a comment

Choose a reason for hiding this comment

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

Thanks @codeofdusk for working to clarify this new feature. It's a key point to make this feature a real success.

Here is my first feedback; maybe there will be more comments after we have discussed it.

codeofdusk and others added 2 commits October 11, 2024 15:49
seanbudd
seanbudd reviewed Oct 14, 2024
View reviewed changes
@AppVeyorBot
Copy link

  • FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • PASS: System tests (tags: installer NVDA).
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/bnf766t2ps84wwm3/artifacts/output/l10nUtil.exe nvda_snapshot_pr17271-34288,63a055e7.exe
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.5,
    INSTALL_END 1.0,
    BUILD_START 0.0,
    BUILD_END 27.0,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.4,
    TEST_START 0.0,
    TEST_END 18.8,
    FINISH_END 0.2

See test results for failed build of commit 63a055e7c5

seanbudd
seanbudd reviewed Oct 16, 2024
View reviewed changes
@seanbudd
Copy link
Member

Thanks @codeofdusk !

@seanbudd seanbudd merged commit 6ecc7d0 into nvaccess:master Oct 17, 2024
5 checks passed
@github-actions github-actions bot added this to the 2025.1 milestone Oct 17, 2024
@codeofdusk codeofdusk mentioned this pull request Oct 17, 2024
Merged
@wmhn1872265132 wmhn1872265132 mentioned this pull request Oct 18, 2024
Closed
seanbudd added a commit that referenced this pull request Apr 3, 2025
@seanbudd
Revert "Various fix-ups for the application volume adjustment feature (…
fbc8785 
…#17271)"

This reverts commit 6ecc7d0.
@seanbudd seanbudd mentioned this pull request Apr 3, 2025
Merged
SaschaCowley pushed a commit that referenced this pull request Apr 4, 2025
@seanbudd
Revert Applications Volume Adjuster (#17886)
37949a8 
### 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
Reviewers

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

+2 more reviewers

@XLTechie XLTechie XLTechie left review comments

@CyrilleB79 CyrilleB79 CyrilleB79 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2025.1
Development

Successfully merging this pull request may close these issues.
6 participants
@codeofdusk @AppVeyorBot @seanbudd @XLTechie @Qchristensen @CyrilleB79