Skip to content

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

Merged
seanbudd merged 15 commits into
nvaccess:masterfrom
codeofdusk:app-volume-fixup
Oct 17, 2024
Merged

Various fix-ups for the application volume adjustment feature#17271
seanbudd merged 15 commits into
nvaccess:masterfrom
codeofdusk:app-volume-fixup

Conversation

@codeofdusk

@codeofdusk codeofdusk commented Oct 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

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

Userguide change looks fine.

CyrilleB79
CyrilleB79 reviewed Oct 10, 2024
View reviewed changes

@CyrilleB79 CyrilleB79 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread user_docs/en/userGuide.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
Comment thread user_docs/en/changes.md Outdated
Comment thread source/gui/settingsDialogs.py Outdated
Comment thread source/audio/appsVolume.py
Comment thread source/audio/appsVolume.py Outdated
Comment thread source/gui/settingsDialogs.py Outdated
Comment thread user_docs/en/userGuide.md Outdated
codeofdusk and others added 2 commits October 11, 2024 15:49
@codeofdusk @CyrilleB79
Review actions
ad6ffda 
Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
@codeofdusk
Review action
544b4cf
seanbudd
seanbudd reviewed Oct 14, 2024
View reviewed changes
Comment thread source/audio/appsVolume.py Outdated
Comment thread source/config/featureFlagEnums.py
Comment thread source/gui/settingsDialogs.py
Comment thread source/gui/settingsDialogs.py Outdated
Comment thread user_docs/en/changes.md Outdated
Comment thread user_docs/en/changes.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
@AppVeyorBot

AppVeyorBot commented Oct 14, 2024

Copy link
Copy Markdown
  • 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
Comment thread user_docs/en/changes.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
codeofdusk and others added 2 commits October 16, 2024 16:41
@codeofdusk @seanbudd
Update user_docs/en/changes.md
b8d8036 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@codeofdusk
Review action
c661113
@seanbudd

seanbudd commented Oct 16, 2024

Copy link
Copy Markdown
Member

Thanks @codeofdusk !

@seanbudd seanbudd merged commit 6ecc7d0 into nvaccess:master Oct 17, 2024
@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