Clips: Stop AudioClipBase's constructor making undoable writes

modules/tracktion_engine/model/clips/tracktion_AudioClipBase.cpp

@@ -205,10 +205,16 @@ class ProxyGeneratorJob  : public AudioProxyGenerator::GeneratorJob
 //==============================================================================
 AudioClipBase::AudioClipBase (const juce::ValueTree& v, EditItemID id, Type t, ClipOwner& targetParent)
     : Clip (v, targetParent, id, t),
-      loopInfo (edit.engine, state.getOrCreateChildWithName (IDs::LOOPINFO, getUndoManager()), getUndoManager()),
+      loopInfo (edit.engine, state.getOrCreateChildWithName (IDs::LOOPINFO, nullptr), getUndoManager()),
       pluginList (edit),
       lastProxy (edit.engine)
 {
+    // N.B. Nothing in this constructor may write to the state via the UndoManager.
+    // A clip can be constructed whilst an undo is in progress (undoing a record or a
+    // clip deletion re-adds the clip's state), and UndoManager::perform refuses to
+    // re-enter, so any undoable write made here is silently discarded.
+    // Corrections to derived geometry and old-Edit migrations are made with a null
+    // UndoManager instead.
     auto um = getUndoManager();
 
     level->dbGain.referTo (state, IDs::gain, um);
@@ -245,9 +251,9 @@ AudioClipBase::AudioClipBase (const juce::ValueTree& v, EditItemID id, Type t, C
 
     // Keep this in to handle old edits..
     if (state.getProperty (IDs::timeStretch))
-        timeStretchMode = juce::VariantConverter<TimeStretcher::Mode>::fromVar (state.getProperty (IDs::stretchMode));
+        timeStretchMode.setValue (juce::VariantConverter<TimeStretcher::Mode>::fromVar (state.getProperty (IDs::stretchMode)), nullptr);
 
-    timeStretchMode = TimeStretcher::checkModeIsAvailable (timeStretchMode);
+    timeStretchMode.setValue (TimeStretcher::checkModeIsAvailable (timeStretchMode), nullptr);
 
     autoPitch.referTo (state, IDs::autoPitch, um);
     autoPitchMode.referTo (state, IDs::autoPitchMode, um);
@@ -256,7 +262,7 @@ AudioClipBase::AudioClipBase (const juce::ValueTree& v, EditItemID id, Type t, C
     isReversed.referTo (state, IDs::isReversed, um);
     autoDetectBeats.referTo (state, IDs::autoDetectBeats, um);
 
-    level->pan = juce::jlimit (-1.0f, 1.0f, static_cast<float> (level->pan.get()));
+    level->pan.setValue (juce::jlimit (-1.0f, 1.0f, static_cast<float> (level->pan.get())), nullptr);
     checkFadeLengthsForOverrun();
 
     useClipLaunchQuantisation.referTo (state, IDs::useClipLaunchQuantisation, um);
@@ -750,8 +756,12 @@ void AudioClipBase::checkFadeLengthsForOverrun()
     if (fadeIn + fadeOut > len)
     {
         const double scale = len / (fadeIn + fadeOut);
-        fadeIn = fadeIn * scale;
-        fadeOut = fadeOut * scale;
+
+        // This is derived geometry, not user data, and it can be corrected whilst a clip is
+        // being constructed. If that happens during an undo, an undoable write would re-enter
+        // UndoManager::perform, which refuses it and silently discards the correction.
+        fadeIn.setValue (fadeIn * scale, nullptr);
+        fadeOut.setValue (fadeOut * scale, nullptr);
     }
 
     // also check the auto fades

modules/tracktion_engine/model/clips/tracktion_Clip.cpp

@@ -879,6 +879,67 @@ TEST_SUITE("tracktion_engine")
             CHECK (clip->getPosition().time == positionBeforeTrim.time);
         }
     }
+
+    TEST_CASE("Fade length correction isn't undoable")
+    {
+        auto& engine = *tracktion::engine::Engine::getEngines()[0];
+        auto edit = Edit::createSingleTrackEdit (engine);
+        auto track = getAudioTracks (*edit)[0];
+        auto& undoManager = edit->getUndoManager();
+
+        auto clip = dynamic_cast<AudioClipBase*> (insertNewClip (*track, TrackItem::Type::wave, { 2_tp, 3_tp }));
+        REQUIRE (clip != nullptr);
+
+        // A comp remnant can be left with fades that are longer than the clip itself
+        const auto length = TimeDuration::fromSeconds (0.00999999999999979);
+        auto clipState = clip->state;
+        clipState.setProperty (IDs::length, length.inSeconds(), nullptr);
+        clipState.setProperty (IDs::fadeIn, 0.0, nullptr);
+        clipState.setProperty (IDs::fadeOut, 0.02, nullptr);
+
+        SUBCASE ("Correcting the fades during construction isn't undoable")
+        {
+            track->state.removeChild (clipState, nullptr);
+            undoManager.clearUndoHistory();
+            undoManager.beginNewTransaction();
+
+            track->state.addChild (clipState, -1, nullptr);
+            const double correctedFadeOut = clipState[IDs::fadeOut];
+            CHECK (correctedFadeOut == doctest::Approx (length.inSeconds()));
+
+            // Undoing back past the clip's construction mustn't revert the correction
+            while (undoManager.canUndo())
+                undoManager.undo();
+
+            CHECK (double (clipState[IDs::fadeOut]) == doctest::Approx (correctedFadeOut));
+        }
+
+        SUBCASE ("Fades are corrected when a clip is rebuilt during an undo")
+        {
+            undoManager.clearUndoHistory();
+            undoManager.beginNewTransaction();
+            clip->removeFromParent();
+            REQUIRE (track->getClips().isEmpty());
+
+            undoManager.beginNewTransaction();
+            undoManager.undo();
+
+            REQUIRE (track->getClips().size() == 1);
+            auto rebuiltClip = dynamic_cast<AudioClipBase*> (track->getClips()[0]);
+            REQUIRE (rebuiltClip != nullptr);
+            CHECK (rebuiltClip->getFadeIn() + rebuiltClip->getFadeOut() <= length);
+
+            // The correction has to reach the state, not just the CachedValue, or it's
+            // lost the next time the Edit is loaded
+            const double fadeInInState  = rebuiltClip->state[IDs::fadeIn];
+            const double fadeOutInState = rebuiltClip->state[IDs::fadeOut];
+            CHECK (fadeInInState + fadeOutInState <= length.inSeconds());
+            CHECK (fadeOutInState == doctest::Approx (length.inSeconds()));
+
+            // The undo history must still be intact
+            CHECK (undoManager.canRedo());
+        }
+    }
 }
 
 } // namespace tracktion::inline engine