AmpliPi icon indicating copy to clipboard operation
AmpliPi copied to clipboard

Volume Delta Expansion

Open SteveMicroNova opened this issue 2 months ago • 3 comments

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

Checklist

  • [x] Have you tested your changes and ensured they work?
  • [x] 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?
  • [x] If applicable, have you updated the CHANGELOG?
  • [x] Does your submission pass linting & tests? You can test on localhost using ./scripts/test
  • [x] Have you written new tests for your core features/changes, as applicable?
  • [x] If this is a UI change, have you tested it across multiple browser platforms on both desktop and mobile?

SteveMicroNova avatar Oct 24 '25 13:10 SteveMicroNova

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.

SteveMicroNova avatar Oct 29 '25 18:10 SteveMicroNova

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

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 50.25%. Comparing base (7499989) to head (6c049a5). :warning: Report is 80 commits behind head on main. :exclamation: 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%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov-commenter avatar Oct 29 '25 20:10 codecov-commenter

This does not interfere with home assistant's functionality

SteveMicroNova avatar Nov 07 '25 16:11 SteveMicroNova

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 avatar Dec 12 '25 22:12 SteveMicroNova

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

SteveMicroNova avatar Dec 15 '25 19:12 SteveMicroNova