Configurable volume floor and visualizer volume linking#237
Open
arro000 wants to merge 3 commits into
Open
Conversation
…ry and unlink volume from visualizer
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a configurable minimum volume floor (VolumeMin) and a visualizer-link toggle (VisVolumeLinked); config parsing, defaults, and clamping are updated; Player and Engine APIs/implementation expose and enforce the floor; visualizer sample scaling and UI volume normalization use the new settings; tests and docs updated. ChangesConfigurable Volume Minimum and Visualizer Linking
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config_test.go (1)
613-638: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert
SetVolumeMinbehavior inTestApplyPlayer.The mock captures
volumeMin, but the test never verifies it, so regressions inConfig.ApplyPlayercan pass unnoticed.✅ Suggested assertion
if p.volume != -10 { t.Errorf("volume = %f, want -10", p.volume) } + if p.volumeMin != cfg.VolumeMin { + t.Errorf("volumeMin = %f, want %f", p.volumeMin, cfg.VolumeMin) + } if p.speed != 1.5 { t.Errorf("speed = %f, want 1.5", p.speed) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/config_test.go` around lines 613 - 638, TestApplyPlayer doesn't assert the mockPlayer's captured volumeMin value, so regressions in Config.ApplyPlayer's handling of SetVolumeMin can go unnoticed; update TestApplyPlayer to verify the mockPlayer.volumeMin (the value set via SetVolumeMin) matches the expected minimum for the configured Volume (or the configured minimum logic), by adding an assertion after cfg.ApplyPlayer(p) that checks p.volumeMin equals the expected value, referencing TestApplyPlayer, mockPlayer, SetVolumeMin and Config.ApplyPlayer to locate the relevant code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/config_test.go`:
- Around line 103-111: Replace the single-case TestVolumeClampedToVolumeMin with
a table-driven test: create a slice of test cases (fields like name, initial
Volume, VolumeMin, expected after clamp), loop over them with t.Run(case.name,
func(t *testing.T) { cfg := defaultConfig(); cfg.VolumeMin = tc.VolumeMin;
cfg.Volume = tc.Volume; cfg.clamp(); if cfg.Volume != tc.expected {
t.Errorf(...) } }), and include at least the original scenario (Volume -40
clamped to -30) plus any additional edge cases you want; keep the test function
name TestVolumeClampedToVolumeMin and reference cfg.clamp() and
cfg.Volume/VolumeMin in the cases so the refactor is purely structural.
In `@config/config.go`:
- Around line 455-456: The case for "vis_volume_linked" currently coerces any
non-"true" token to false; change it to only update cfg.VisVolumeLinked when the
value is a valid boolean. In the "vis_volume_linked" branch, attempt to parse
val with strconv.ParseBool (or explicitly check for "true"/"false"), and only
assign to cfg.VisVolumeLinked when parsing succeeds; otherwise leave
cfg.VisVolumeLinked unchanged so the default is preserved.
In `@player/player_test.go`:
- Around line 62-69: Convert TestVolumeClampedToCustomMin into a table-driven
test: create a slice of test cases (struct with name, min, set, want) and
iterate with t.Run for each case; inside the loop call newTestPlayer(),
p.SetVolumeMin(tc.min), p.SetVolume(tc.set) and assert p.Volume() == tc.want.
Keep the original behavior for the existing values (min -30, set -40, want -30)
and reference the same functions/methods (TestVolumeClampedToCustomMin,
newTestPlayer, SetVolumeMin, SetVolume, Volume) so the test style matches the
rest of the suite.
In `@player/player.go`:
- Around line 569-571: The SetVolumeMin implementation must atomically clamp the
input (keep using max(min(db, 0), -90) and math.Float64bits) AND reconcile the
current volume so Volume() isn't left below the new floor: store the new min
into p.volMin, then load the current volume value from p.vol (use
math.Float64frombits on p.vol.Load()), and if the current volume is less than
the new min, perform an atomic compare-and-swap loop on p.vol to replace the old
bits with math.Float64bits(newMin) (retrying if CAS fails) so the current volume
is raised to the new floor immediately; reference SetVolumeMin, p.volMin, p.vol,
Volume(), and use math.Float64bits/Float64frombits and the atomic CAS on p.vol.
---
Outside diff comments:
In `@config/config_test.go`:
- Around line 613-638: TestApplyPlayer doesn't assert the mockPlayer's captured
volumeMin value, so regressions in Config.ApplyPlayer's handling of SetVolumeMin
can go unnoticed; update TestApplyPlayer to verify the mockPlayer.volumeMin (the
value set via SetVolumeMin) matches the expected minimum for the configured
Volume (or the configured minimum logic), by adding an assertion after
cfg.ApplyPlayer(p) that checks p.volumeMin equals the expected value,
referencing TestApplyPlayer, mockPlayer, SetVolumeMin and Config.ApplyPlayer to
locate the relevant code to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0968409b-f51e-408e-8b8e-e96c64d20ccb
📒 Files selected for processing (15)
config/config.goconfig/config_test.godocs/configuration.mdmain.goplayer/engine.goplayer/player.goplayer/player_test.goplayer/tap.gosite/index.htmlui/model/init.goui/model/model.goui/model/playback_test.goui/model/stream_seek_keys_test.goui/model/tick.goui/model/view.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the minimum volume floor from -30 dB to -50 dB and makes it configurable via
volume_mininconfig.toml. Also addsvis_volume_linkedto control whether visualizer bar height follows the current volume level.volume_min(default-50, range[-90, 0]): sets the dB floor for the volume control. The bar formula in the UI scales accordingly.vis_volume_linked(defaulttrue): whentrue, visualizer bars reflect the current volume (classic behavior). Set tofalseto decouple thevisualizer from volume — useful when listening at very low levels.
Internally, the audio tap is now placed before the volume streamer in the pipeline so the visualizer always captures pre-volume samples. When
vis_volume_linked = true, the volume gain is re-applied to the samples in the FFT analysis path instead.How to test
volume_min = -50(or omit — it's the default) and verify you can lower volume below -30 dB.volume_min = -70inconfig.toml, restart, and confirm the floor is now -70 dB.vis_volume_linked = true(default), confirm visualizer bars shrink as volume decreases.vis_volume_linked = false, lower volume to -50 dB, and confirm bars remain at full height.make check.Checklist
make checkpassesdocs/andsite/index.htmlupdated for user-facing changesSummary by CodeRabbit
New Features
Documentation