Skip to content

Volume Delta Expansion#1062

Open
SteveMicroNova wants to merge 1 commit into
mainfrom
VolumeDeltaExpansion
Open

Volume Delta Expansion#1062
SteveMicroNova wants to merge 1 commit into
mainfrom
VolumeDeltaExpansion

Conversation

@SteveMicroNova
Copy link
Copy Markdown
Contributor

@SteveMicroNova SteveMicroNova commented Oct 24, 2025

What does this change intend to accomplish?

This PR aims to preserve the relative distance between zone volume levels when they would otherwise exceed the min or max vol_f

Demonstration

Checklist

  • Have you tested your changes and ensured they work?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • If applicable, have you updated the documentation/manual?
  • If applicable, have you updated the CHANGELOG?
  • Does your submission pass linting & tests? You can test on localhost using ./scripts/test
  • Have you written new tests for your core features/changes, as applicable?
  • If this is a UI change, have you tested it across multiple browser platforms on both desktop and mobile?

@SteveMicroNova SteveMicroNova linked an issue Oct 29, 2025 that may be closed by this pull request
@SteveMicroNova SteveMicroNova marked this pull request as ready for review October 29, 2025 16:41
Comment thread amplipi/models.py
@SteveMicroNova SteveMicroNova linked an issue Oct 29, 2025 that may be closed by this pull request
@SteveMicroNova
Copy link
Copy Markdown
Contributor Author

This has expanded to also include #1024 due to that being one extra line beyond what I'd already done to implement that for zones, though I then discovered it was an extra chunk to App.jsx to do the same for groups so there was some slight feature creep to this PR there.

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.25%. Comparing base (7499989) to head (6c049a5).
⚠️ Report is 80 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1062      +/-   ##
==========================================
- Coverage   50.67%   50.25%   -0.42%     
==========================================
  Files          40       41       +1     
  Lines        7154     7368     +214     
==========================================
+ Hits         3625     3703      +78     
- Misses       3529     3665     +136     
Flag Coverage Δ
unittests 50.25% <100.00%> (-0.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SteveMicroNova
Copy link
Copy Markdown
Contributor Author

This does not interfere with home assistant's functionality

Comment thread amplipi/ctrl.py
Comment thread amplipi/models.py Outdated
Comment thread amplipi/ctrl.py Outdated
Comment thread amplipi/ctrl.py Outdated
Comment thread amplipi/ctrl.py
Comment thread amplipi/ctrl.py Outdated
Comment thread amplipi/models.py Outdated
Copy link
Copy Markdown
Contributor

@linknum23 linknum23 left a comment

Choose a reason for hiding this comment

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

The changes here look pretty reasonable. As this is an API change we should add one or more tests (or update pre-existing ones) right?

Comment thread tests/test_rest.py
Comment thread amplipi/ctrl.py Outdated
Comment thread amplipi/ctrl.py Outdated
Comment thread amplipi/ctrl.py Outdated
Comment thread web/src/App.jsx
Comment thread web/src/App.jsx Outdated
Comment thread web/src/App.jsx Outdated
Comment thread web/src/App.jsx Outdated
@SteveMicroNova
Copy link
Copy Markdown
Contributor Author

There's some jittering in this yet, not due to this PR but exacerbated by it. The origin of this jitter is that we send volume change requests while sliding the slider but not all of the delta is always realized during that due to a race condition. A few solutions to this:

  • Only recognize on mouse up - This loses the ability to hear changes as you scroll
  • Convert the volume change call to be some sort of loop that goes while cumulativeDelta is not 0 as to not miss any user input - This seems complicated, not nearly as useful as python would make it. Potentially better to solve our queuing problem in the backend if this is the chosen path

There's also some potential that the jitter is due to javascript only having floats for decimal values, which would mean we have to fix this by rounding every single instance of a float vol everywhere to see this fixed

@SteveMicroNova
Copy link
Copy Markdown
Contributor Author

I'm going to refrain from rebasing #1063 and #1071 until we're happy with the volume slider functionality here, so those will not be worth reviewing until that is the case

Comment thread web/src/components/CardVolumeSlider/CardVolumeSlider.jsx
@SteveMicroNova SteveMicroNova mentioned this pull request Feb 11, 2026
5 tasks
@SteveMicroNova SteveMicroNova linked an issue Feb 24, 2026 that may be closed by this pull request
Comment thread amplipi/app.py
@SteveMicroNova SteveMicroNova force-pushed the VolumeDeltaExpansion branch 3 times, most recently from 013587a to 9efc33d Compare March 3, 2026 21:22
@SteveMicroNova SteveMicroNova force-pushed the VolumeDeltaExpansion branch from 9efc33d to 5d5e3f2 Compare May 11, 2026 18:20
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.

Add min/max volume buffers Set child mute status on the frontend

3 participants