APVTS: Fixed some issues using an UndoManager with an AudioProcessorValueTreeState

BREAKING-CHANGES.txt

@@ -4,6 +4,27 @@ JUCE breaking changes
 Develop
 =======
 
+Change
+------
+The behaviour of an UndoManager used by an AudioProcessorValueTreeState has
+been improved.
+
+Possible Issues
+---------------
+If your plug-in contains an UndoManager used by an AudioProcessorValueTreeState
+and relies upon the old behaviour of the UndoManager then it is possible that
+the new behaviour is no longer appropriate for your use case.
+
+Workaround
+----------
+Use an external UndoManager to reproduce the old behaviour manually.
+
+Rationale
+---------
+This change fixes a few bugs in the behaviour of an UndoManager used by an
+AudioProcessorValueTreeState.
+
+
 Change
 ------
 JUCE no longer supports OS X deployment targets earlier than 10.7.

modules/juce_audio_processors/utilities/juce_AudioProcessorValueTreeState.cpp

@@ -49,8 +49,8 @@ struct AudioProcessorValueTreeState::Parameter   : public AudioProcessorParamete
           isDiscreteParam (discrete),
           isBooleanParam (boolean)
     {
+        value = defaultValue;
         state.addListener (this);
-        needsUpdate.set (1);
     }
 
     ~Parameter()
@@ -93,7 +93,7 @@ struct AudioProcessorValueTreeState::Parameter   : public AudioProcessorParamete
             listeners.call ([=] (AudioProcessorValueTreeState::Listener& l) { l.parameterChanged (paramID, value); });
             listenersNeedCalling = false;
 
-            needsUpdate.set (1);
+            needsUpdate = true;
         }
     }
 
@@ -119,12 +119,25 @@ struct AudioProcessorValueTreeState::Parameter   : public AudioProcessorParamete
 
     void copyValueToValueTree()
     {
-        if (state.isValid())
-            state.setPropertyExcludingListener (this, owner.valuePropertyID, value, owner.undoManager);
+        if (auto* valueProperty = state.getPropertyPointer (owner.valuePropertyID))
+        {
+            if ((float) *valueProperty != value)
+            {
+                ScopedValueSetter<bool> svs (ignoreParameterChangedCallbacks, true);
+                state.setProperty (owner.valuePropertyID, value, owner.undoManager);
+            }
+        }
+        else
+        {
+            state.setProperty (owner.valuePropertyID, value, nullptr);
+        }
     }
 
     void valueTreePropertyChanged (ValueTree&, const Identifier& property) override
     {
+        if (ignoreParameterChangedCallbacks)
+            return;
+
         if (property == owner.valuePropertyID)
             updateFromValueTree();
     }
@@ -163,9 +176,10 @@ struct AudioProcessorValueTreeState::Parameter   : public AudioProcessorParamete
     std::function<float (const String&)> textToValueFunction;
     NormalisableRange<float> range;
     float value, defaultValue;
-    Atomic<int> needsUpdate;
+    std::atomic<bool> needsUpdate { true };
     bool listenersNeedCalling;
     const bool isMetaParam, isAutomatableParam, isDiscreteParam, isBooleanParam;
+    bool ignoreParameterChangedCallbacks = false;
 
     JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (Parameter)
 };
@@ -218,7 +232,7 @@ Value AudioProcessorValueTreeState::getParameterAsValue (StringRef paramID) cons
     if (Parameter* p = Parameter::getParameterForID (processor, paramID))
         return p->state.getPropertyAsValue (valuePropertyID, undoManager);
 
-    return Value();
+    return {};
 }
 
 NormalisableRange<float> AudioProcessorValueTreeState::getParameterRange (StringRef paramID) const noexcept
@@ -246,6 +260,8 @@ ValueTree AudioProcessorValueTreeState::copyState()
 {
     ScopedLock lock (valueTreeChanging);
 
+    flushParameterValuesToValueTree();
+
     return state.createCopy();
 }
 
@@ -254,6 +270,9 @@ void AudioProcessorValueTreeState::replaceState (const ValueTree& newState)
     ScopedLock lock (valueTreeChanging);
 
     state = newState;
+
+    if (undoManager != nullptr)
+        undoManager->clearUndoHistory();
 }
 
 ValueTree AudioProcessorValueTreeState::getOrCreateChildValueTree (const String& paramID)
@@ -263,8 +282,8 @@ ValueTree AudioProcessorValueTreeState::getOrCreateChildValueTree (const String&
     if (! v.isValid())
     {
         v = ValueTree (valueType);
-        v.setProperty (idPropertyID, paramID, undoManager);
-        state.appendChild (v, undoManager);
+        v.setProperty (idPropertyID, paramID, nullptr);
+        state.appendChild (v, nullptr);
     }
 
     return v;
@@ -316,14 +335,16 @@ bool AudioProcessorValueTreeState::flushParameterValuesToValueTree()
 {
     ScopedLock lock (valueTreeChanging);
 
-    auto anythingUpdated = false;
+    bool anythingUpdated = false;
 
     for (auto* ap : processor.getParameters())
     {
         jassert (dynamic_cast<Parameter*> (ap) != nullptr);
         auto* p = static_cast<Parameter*> (ap);
 
-        if (p->needsUpdate.compareAndSetBool (0, 1))
+        bool needsUpdateTestValue = true;
+
+        if (p->needsUpdate.compare_exchange_strong (needsUpdateTestValue, false))
         {
             p->copyValueToValueTree();
             anythingUpdated = true;
@@ -395,7 +416,12 @@ struct AttachedControlBase  : public AudioProcessorValueTreeState::Listener,
     void beginParameterChange()
     {
         if (AudioProcessorParameter* p = state.getParameter (paramID))
+        {
+            if (state.undoManager != nullptr)
+                state.undoManager->beginNewTransaction();
+
             p->beginChangeGesture();
+        }
     }
 
     void endParameterChange()