Skip to content

Revert Applications Volume Adjuster#17886

Merged
SaschaCowley merged 5 commits into
masterfrom
revAVA
Apr 4, 2025
Merged

Revert Applications Volume Adjuster#17886
SaschaCowley merged 5 commits into
masterfrom
revAVA

Conversation

@seanbudd
Copy link
Copy Markdown
Member

@seanbudd seanbudd commented Apr 3, 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

Can this PR be reimplemented? If so, what is required for the next attempt

@seanbudd seanbudd requested review from a team as code owners April 3, 2025 01:29
@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented Apr 3, 2025

cc @CyrilleB79 @mltony @LeonarddeR @ABuffEr @codeofdusk: as those who contributed to this issue/feature discussion

@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit 4ca4596eb2

@LeonarddeR
Copy link
Copy Markdown
Collaborator

LeonarddeR commented Apr 3, 2025

While it is a pity that this is reverted for those who really like this feature, I think this is the right decision.
That said, do the reverted prs contain any fixes for features that are intertwined, such as sound split? Those should be considered if any.
For example, #16591 states:

I refactored sound split code...

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented Apr 3, 2025

@LeonarddeR - we are not aware of any explicit known fixes for sound split from this PR, no associated logged issues were mentioned in the PR.

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented Apr 3, 2025

I'd also mention that it is fairly possible/likely the bug #17654 was introduced via the refactoring, not necessarily from the feature itself

@beqabeqa473
Copy link
Copy Markdown
Contributor

i don't think soundsplit as well as application volume adjuster need to be in nvda core. They can greatly fit in addon.
NVDA is primarily a screenreader, and if user needs such functionality, they can easily install an addon.

I recall discussion when nvaccess was strongly against including systraylist addon into core, but i am sure that feature more likely is direct screenreader feature rather than these features, which simply overload application.
I think NVaccess philosophy should be as it was earlier, include in the core features, which are directly related to screenreader.

@CyrilleB79
Copy link
Copy Markdown
Contributor

I am one of the ones who would have loved this feature to be integrated in core. But unfortunately, the feature as implemented in last alpha was not robust enough. This revert PR fixes #17654 so that's the way to go.

@beqabeqa473 The integration of the feature in NVDA core has been accepted (triaged) by NV Access.

  1. That's not the place at all to question this decision. Feature request: add command to adjust volume of all applications except for NVDA #16052 is being reopened and this would be a better place to discuss this.
  2. There has already been a very long discussion before the feature was integrated. If you really want to question this triage decision, please be sure to provide new arguments that may really give a chance for the decision to be reconsidered.
  3. Sound split remains in NVDA's core and has already done its path to stable releases. If you really wish to remove it from core, please open a dedicated issue with a strong argumentation: I do not even know if a feature has ever been removed from stable releases.

@HBM2001
Copy link
Copy Markdown

HBM2001 commented Apr 3, 2025

Hello everyone, to be honest, I THINK the sound split feature may also be necessary in certain cases, such as checking the position of a headset or speaker.
Regarding the application's volume control feature, I'm not entirely sure whether it is necessary, some addons already provide this functionality

@SaschaCowley
Copy link
Copy Markdown
Member

@LeonarddeR IIRC when I tried to track down the commit that introduced #17654 the earliest commit I could track it to was the refactor of sound split. It's also not present in 2024.4.2.

@SaschaCowley SaschaCowley merged commit 37949a8 into master Apr 4, 2025
@SaschaCowley SaschaCowley deleted the revAVA branch April 4, 2025 05:05
@github-actions github-actions Bot added this to the 2025.1 milestone Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

7 participants