Skip to content

Clips: Stop AudioClipBase's constructor making undoable writes - #433

Merged
drowaudio merged 1 commit into
developfrom
bugfix/issue_415_audio_clip_base_undoable_fade_write
Sep 18, 2026
Merged

drowaudio merged 1 commit into
developfrom
bugfix/issue_415_audio_clip_base_undoable_fade_write

Conversation

@drowaudio

Copy link
Copy Markdown
Contributor

Summary

AudioClipBase's constructor wrote to the clip state through the Edit's UndoManager. A clip can be constructed whilst an undo is in progress, so those writes were being silently discarded. This makes the constructor's corrections and migrations non-undoable.

Root cause

Undoing a record, or undoing a clip deletion, re-adds the clip's ValueTree child inside the undo transaction. valueTreeChildAdded rebuilds the Clip, and the constructor's checkFadeLengthsForOverrun() then rescales fadeIn/fadeOut through a CachedValue bound to edit.getUndoManager().

That re-enters UndoManager::perform, which refuses to run recursively - it hits jassert (! isPerformingUndoRedo()) at juce_UndoManager.cpp:128 and drops the action. The correction never reaches the state.

Worse, CachedValue::setValue assigns its cached copy before calling ValueTree::setProperty, so after a refused perform the clip object reports the corrected fade whilst the state still holds the overrunning one. The two only diverge until the Edit is next loaded, at which point the bad fade comes back.

Fixed by making everything the constructor writes non-undoable:

  • checkFadeLengthsForOverrun() clamps fadeIn/fadeOut with a null UndoManager. This is derived geometry, not user data, and the user-facing paths (setFadeIn/setFadeOut) do their own clamping undoably as before.
  • The pan limit, the legacy timeStretch -> stretchMode migration and TimeStretcher::checkModeIsAvailable() likewise - all are corrections/migrations rather than edits.
  • The LOOPINFO child is created with a null UndoManager. ValueTree::getOrCreateChildWithName returns a detached tree when the appendChild perform is refused, which would have left LoopInfo writing into a tree that isn't in the Edit.

A comment on the constructor records the constraint so it doesn't regress.

Regression test

"Fade length correction isn't undoable" in tracktion_Clip.cpp, using the issue's fixture - a clip of length 0.00999999999999979 carrying a 0.02 fade-out:

  • Correcting the fades during construction isn't undoable - rebuilds the clip from state inside an open transaction, then undoes everything; the corrected fadeOut must survive. Fails on develop (0.02 vs Approx(0.01)).
  • Fades are corrected when a clip is rebuilt during an undo - deletes the clip in a transaction and undoes it, so the clip is reconstructed inside UndoManager::perform. Asserts on the fadeIn/fadeOut properties in the state, not the CachedValue accessors, which is what actually broke; also checks the undo history is still intact afterwards. Fails on develop (0.02 <= 0.01).

Both subcases fail on develop and pass with the fix.

Testing

Full TestRunner suite (Release, macOS): 370/370 doctest cases, 21267 assertions, green.

Fixes #415

🤖 Generated with Claude Code

 #415)

A clip can be constructed whilst an undo is in progress - undoing a record or a
clip deletion re-adds the clip's state, which rebuilds the Clip from
valueTreeChildAdded inside the undo transaction. Any write made through the
Edit's UndoManager at that point re-enters UndoManager::perform, which refuses
it, asserts, and silently discards the change. CachedValue still updates its
cached copy, so the object and the state then disagree until the Edit is
reloaded.

Made the constructor's corrections and migrations non-undoable:
 - checkFadeLengthsForOverrun() now clamps fadeIn/fadeOut with a null
   UndoManager; it's derived geometry, not user data, and setFadeIn/setFadeOut
   still do their own clamping on the user-facing paths
 - the pan limit, the legacy timeStretch migration and
   TimeStretcher::checkModeIsAvailable() likewise
 - the LOOPINFO child is created with a null UndoManager, so a refused
   perform() can't leave LoopInfo referring to a detached tree

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.83%. Comparing base (aa6301a) to head (3175421).

Files with missing lines Patch % Lines
...ion_engine/model/clips/tracktion_AudioClipBase.cpp 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #433      +/-   ##
===========================================
+ Coverage    59.81%   59.83%   +0.02%     
===========================================
  Files          568      568              
  Lines        79964    80002      +38     
  Branches     12381    12382       +1     
===========================================
+ Hits         47828    47869      +41     
+ Misses       32136    32133       -3     

☔ View full report in Codecov by Harness.
📢 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.

@drowaudio

Copy link
Copy Markdown
Contributor Author

Will this have any problems when undoing? For example, if a clip gets copy/pasted then undone then redone, will these properties get lost? Or will they just get reset again to the defaults in a non-undoable way?

@drowaudio

Copy link
Copy Markdown
Contributor Author

🤖 Good question - I tested that exact scenario rather than reasoning about it. The properties survive; they don't get lost, and they don't need re-deriving.

Paste → undo → redo, with the issue's fixture (length 0.00999…, stored fadeOut 0.02), adding the clip state to the track as an undoable action:

PASTE  fadeOut=0.01  loopinfo=1  clips=1
UNDO   clips=0       canRedo=1
REDO   fadeOut=0.01  fadeIn=0    loopinfo=1  elastiqueMode=0

Two independent reasons it holds:

  1. JUCE's AddOrRemoveChildAction keeps a reference to the child's SharedObject, not a snapshot. Undo detaches that same object and redo re-attaches it, so non-undoable mutations made to the subtree while it was attached ride along. The LOOPINFO child and the corrected fades are still there after the round trip.
  2. Even if that weren't true, redo reconstructs the clip (valueTreeChildAdded fires again), so the constructor re-derives exactly the same values. The correction is idempotent and depends only on length/fadeIn/fadeOut, and the timeStretchMode and LOOPINFO paths are likewise "create if absent" / "fix if unavailable".

So it's the second half of your alternative: they'd get reset again in a non-undoable way - except they don't even need to be, because they were never removed.

Worth being straight about the one case that is imperfect, though it's unchanged by this PR: nothing re-clamps the fades when a clip is trimmed shorter (checkFadeLengthsForOverrun is only called from the constructor and updateAutoCrossfades). So undoing back to a state where the stored fades overrun leaves them overrunning until the clip is next constructed. That was true before this change too - the difference is only that the correction now actually reaches the state instead of being silently discarded.

Happy to add the paste/undo/redo round trip as a third SUBCASE if you'd like it guarded permanently - say the word and I'll push it.

@drowaudio
drowaudio merged commit b325a9f into develop Sep 18, 2026
39 checks passed
@drowaudio
drowaudio deleted the bugfix/issue_415_audio_clip_base_undoable_fade_write branch September 18, 2026 14:57
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.

AudioClipBase's constructor performs an UNDOABLE write

1 participant