Skip to content

VolumeAndPan: Made vol/pan parameters undoable - #165

Open
drowaudio wants to merge 1 commit into
developfrom
feature/undoable_vol_pan
Open

VolumeAndPan: Made vol/pan parameters undoable#165
drowaudio wants to merge 1 commit into
developfrom
feature/undoable_vol_pan

Conversation

@drowaudio

Copy link
Copy Markdown
Contributor

No description provided.

@codecov

codecov Bot commented Jun 7, 2023

Copy link
Copy Markdown

Codecov Report

Merging #165 (4d370da) into develop (4d370da) will not change coverage.
The diff coverage is n/a.

❗ Current head 4d370da differs from pull request most recent head 51a1d45. Consider uploading reports for the commit 51a1d45 to get more accurate results

@@           Coverage Diff            @@
##           develop     #165   +/-   ##
========================================
  Coverage    35.50%   35.50%           
========================================
  Files          465      465           
  Lines        51993    51993           
========================================
  Hits         18458    18458           
  Misses       33535    33535           

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jun 7, 2023

Copy link
Copy Markdown

Unit Test Results

       3 files  ±0         3 suites  ±0   0s ⏱️ ±0s
   173 tests +2     173 ✔️ +2  0 💤 ±0  0 ❌ ±0 
6 198 runs  +6  6 198 ✔️ +6  0 💤 ±0  0 ❌ ±0 

Results for commit 51a1d45. ± Comparison against base commit 8f46d18.

@github-actions

github-actions Bot commented Jun 7, 2023

Copy link
Copy Markdown

Performance Test Results

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 51a1d45. ± Comparison against base commit 8f46d18.

@drowaudio drowaudio added the claude-local Opt this PR in for the local Claude PR-concierge session label Aug 26, 2026
@drowaudio

Copy link
Copy Markdown
Contributor Author

Implementation plan

Worktree set up on feature/undoable_vol_pan. No linked issue on this PR (empty body, no closing references), so this plan is derived from the diff itself. Nothing implemented yet - waiting for a go-ahead before touching the branch.

Current state

The branch is a single commit (51a1d45) from June 2023, now 669 commits behind develop and conflicting (mergeStateStatus: DIRTY). develop still has the original attachToCurrentValue/detachFromCurrentValue design, so the substance of this PR has not landed by another route - it just needs rebasing onto a file that has moved a lot.

What the change does

Today volParam/panParam are attached to the volume/pan CachedValues, so the parameter object owns the value and pushes it into the ValueTree. That bypasses the UndoManager transaction boundaries, so vol/pan moves are not undoable.

The PR inverts the ownership:

  • Drops the attach/detach pair; the constructor seeds the parameters once with setParameter (volume, dontSendNotification).
  • setSliderPos/setPan now write the CachedValue (volume = ..., pan = ...), which goes through the UndoManager the CachedValue was referTo'd with.
  • A new valueTreePropertyChanged override watches IDs::volume/IDs::pan on state, calls forceUpdateOfCachedValue() and pushes the value back into the parameter with sendNotification. Undo/redo mutates the tree, the callback fires, the parameter follows.

This is coherent with the accessors on develop: getSliderPos()/getPan() already read volParam->getCurrentValue(), so the parameter stays the read source and the CachedValue becomes the persisted, undoable store.

Rebase conflicts to resolve

  1. tracktion_VolumeAndPan.test.cpp - the real conflict. develop already has a file at this path, written as doctest TEST_SUITE/TEST_CASE (channel-widening and pan tests). The PR's version is a juce::UnitTest subclass with a static instance. Resolution: keep develop's file and port the PR's undo/redo coverage into it as two new TEST_CASEs ("undo/redo of volume and pan" and "state reloads after save"), using CHECK/REQUIRE and test_utilities::createTestEdit, rather than reintroducing the old juce::UnitTest style.
  2. tracktion_engine_plugins.cpp - the #include ".../tracktion_VolumeAndPan.test.cpp" line the PR adds is already on develop (line 74). Drop that hunk entirely.
  3. tracktion_TestConfig.h - ENGINE_UNIT_TESTS_VOLPANPLUGIN is already defined on develop (line 71). No change needed.
  4. tracktion_VolumeAndPan.h/.cpp - the surrounding code has changed (smoothed gains, RealTimeSpinLock vca lock, smoothingRampTimeSeconds), so the hunks will not apply cleanly, but none of that interacts with the attach/detach mechanism. Reapply by hand.

Proposed steps

  1. Rebase feature/undoable_vol_pan onto origin/develop, taking the resolutions above.
  2. Reapply the four source edits (constructor seed, destructor detach removal, setSliderPos/setPan writing the CachedValue, valueTreePropertyChanged override) against current develop.
  3. Drop the now-dead updateFromAttachedValue() loop at the end of restorePluginStateFromValueTree (line ~347 on develop) - with nothing attached it is a no-op, and the new valueTreePropertyChanged covers the restore path.
  4. Port the undo test cases into develop's doctest file.
  5. Add a BREAKING-CHANGES.md entry for the header change (see below).
  6. Build TestRunner and run -tc="*VolumeAndPan*" plus the plugin and automation suites.

Things worth a decision before I start

  • volume/pan moved from public to private in the header. I grepped the engine and the Waveform app: nothing reads those members directly (everything goes through getVolumeDb/setPan/getSliderPos), so it is safe in-tree, but it is still a public API break for other users of the module. Do you want them kept public, or private plus a BREAKING-CHANGES.md entry?
  • Automation interaction. With the attachment gone, an automation curve writing to volParam no longer writes back to the ValueTree. That is presumably intended (automation should not fill the undo history), but it changes what gets persisted when an edit is saved mid-automation. Worth confirming that is the behaviour you want.
  • Undo granularity. Each setSliderPos call now creates undo data. A fader drag calls it per mouse-move, so without coalescing a single drag could produce a long chain of undo steps. The test sidesteps this with um.setMaxNumberOfStoredUnits (30000, 30). Should the drag path open one transaction per gesture?

Say the word and I will implement it on this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-local Opt this PR in for the local Claude PR-concierge session

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant