Bug1348, partial: Fix bad undo after cancelling Vamp or FindClipping analyzer

src/effects/Effect.cpp

@@ -40,6 +40,7 @@ greater use in future.
 #include "audacity/ConfigInterface.h"
 
 #include "../AudioIO.h"
+#include "../LabelTrack.h"
 #include "../Mix.h"
 #include "../Prefs.h"
 #include "../Project.h"
@@ -2131,6 +2132,88 @@ void Effect::AddToOutputTracks(Track *t)
    mOMap.Add(t);
 }
 
+Effect::AddedAnalysisTrack::AddedAnalysisTrack(Effect *pEffect, const wxString &name)
+   : mpEffect(pEffect)
+{
+   std::unique_ptr < LabelTrack > pTrack{ pEffect->mFactory->NewLabelTrack() };
+   mpTrack = pTrack.get();
+   if (!name.empty())
+      pTrack->SetName(name);
+   pEffect->mTracks->Add(pTrack.release());
+}
+
+Effect::AddedAnalysisTrack::AddedAnalysisTrack(AddedAnalysisTrack &&that)
+{
+   mpEffect = that.mpEffect;
+   mpTrack = that.mpTrack;
+   that.Commit();
+}
+
+void Effect::AddedAnalysisTrack::Commit()
+{
+   mpEffect = nullptr;
+}
+
+Effect::AddedAnalysisTrack::~AddedAnalysisTrack()
+{
+   if (mpEffect) {
+      // not committed -- DELETE the label track
+      mpEffect->mTracks->Remove(mpTrack, true);
+   }
+}
+
+auto Effect::AddAnalysisTrack(const wxString &name) -> AddedAnalysisTrack
+{
+   return {this, name};
+}
+
+Effect::ModifiedAnalysisTrack::ModifiedAnalysisTrack
+   (Effect *pEffect, const LabelTrack *pOrigTrack, const wxString &name)
+   : mpEffect(pEffect)
+   , mpOrigTrack(pOrigTrack)
+{
+   Track *newTrack{};
+
+   // copy LabelTrack here, so it can be undone on cancel
+   pOrigTrack->Copy(pOrigTrack->GetStartTime(), pOrigTrack->GetEndTime(), &newTrack);
+
+   mpTrack = static_cast<LabelTrack*>(newTrack);
+
+   // Why doesn't LabelTrack::Copy complete the job? :
+   mpTrack->SetOffset(pOrigTrack->GetStartTime());
+   if (!name.empty())
+      mpTrack->SetName(name);
+
+   pEffect->mTracks->Replace(mpOrigTrack, mpTrack, false);
+}
+
+Effect::ModifiedAnalysisTrack::ModifiedAnalysisTrack(ModifiedAnalysisTrack &&that)
+{
+   mpEffect = that.mpEffect;
+   mpTrack = that.mpTrack;
+   mpOrigTrack = that.mpOrigTrack;
+   that.Commit();
+}
+
+void Effect::ModifiedAnalysisTrack::Commit()
+{
+   mpEffect = nullptr;
+}
+
+Effect::ModifiedAnalysisTrack::~ModifiedAnalysisTrack()
+{
+   if (mpEffect) {
+      // not committed -- DELETE the label track
+      mpEffect->mTracks->Replace(mpTrack, mpOrigTrack, true);
+   }
+}
+
+auto Effect::ModifyAnalysisTrack
+   (const LabelTrack *pOrigTrack, const wxString &name) -> ModifiedAnalysisTrack
+{
+   return{ this, pOrigTrack, name };
+}
+
 // If bGoodResult, replace mTracks tracks with successfully processed mOutputTracks copies.
 // Else clear and DELETE mOutputTracks copies.
 void Effect::ReplaceProcessedTracks(const bool bGoodResult)

src/effects/Effect.h

@@ -40,6 +40,7 @@ class ShuttleGui;
 #define BUILTIN_EFFECT_PREFIX wxT("Built-in Effect: ")
 
 class AudacityProject;
+class LabelTrack;
 class SelectedRegion;
 class TimeWarper;
 class EffectUIHost;
@@ -350,6 +351,69 @@ protected:
    void CopyInputTracks(); // trackType = Track::Wave
    void CopyInputTracks(int trackType);
 
+   // For the use of analyzers, which don't need to make output wave tracks,
+   // but may need to add label tracks.
+   class AddedAnalysisTrack {
+      friend Effect;
+      AddedAnalysisTrack(Effect *pEffect, const wxString &name);
+      AddedAnalysisTrack(const AddedAnalysisTrack&) PROHIBITED;
+
+   public:
+
+      AddedAnalysisTrack() {}
+
+      // So you can have a vector of them
+      AddedAnalysisTrack(AddedAnalysisTrack &&that);
+
+      LabelTrack *get() const { return mpTrack; }
+
+      // Call this to indicate successful completion of the analyzer.
+      void Commit();
+
+      // Destructor undoes the addition of the analysis track if not committed.
+      ~AddedAnalysisTrack();
+
+   private:
+      Effect *mpEffect{};
+      LabelTrack *mpTrack{};
+   };
+
+   // Set name to given value if that is not empty, else use default name
+   AddedAnalysisTrack AddAnalysisTrack(const wxString &name = wxString());
+
+   // For the use of analyzers, which don't need to make output wave tracks,
+   // but may need to modify label tracks.
+   class ModifiedAnalysisTrack {
+      friend Effect;
+      ModifiedAnalysisTrack
+         (Effect *pEffect, const LabelTrack *pOrigTrack, const wxString &name);
+      ModifiedAnalysisTrack(const ModifiedAnalysisTrack&) PROHIBITED;
+
+   public:
+
+      ModifiedAnalysisTrack();
+
+      // So you can have a vector of them
+      ModifiedAnalysisTrack(ModifiedAnalysisTrack &&that);
+
+      LabelTrack *get() const { return mpTrack; }
+
+      // Call this to indicate successful completion of the analyzer.
+      void Commit();
+
+      // Destructor undoes the modification of the analysis track if not committed.
+      ~ModifiedAnalysisTrack();
+
+   private:
+      Effect *mpEffect{};
+      LabelTrack *mpTrack{};
+      const LabelTrack *mpOrigTrack{};
+   };
+
+   // Set name to given value if that is not empty, else use default name
+   ModifiedAnalysisTrack ModifyAnalysisTrack
+      (const LabelTrack *pOrigTrack, const wxString &name = wxString());
+
    // If bGoodResult, replace mWaveTracks tracks in mTracks with successfully processed
    // mOutputTracks copies, get rid of old mWaveTracks, and set mWaveTracks to mOutputTracks.
    // Else clear and DELETE mOutputTracks copies.

src/effects/FindClipping.cpp

@@ -93,26 +93,24 @@ bool EffectFindClipping::SetAutomationParameters(EffectAutomationParameters & pa
 
 bool EffectFindClipping::Process()
 {
-   LabelTrack *l = NULL;
+   Maybe<AddedAnalysisTrack> addedTrack;
+   Maybe<ModifiedAnalysisTrack> modifiedTrack;
    Track *original = NULL;
+   const wxString name{ _("Clipping") };
 
+   LabelTrack *lt = NULL;
    TrackListOfKindIterator iter(Track::Label, mTracks);
    for (Track *t = iter.First(); t; t = iter.Next()) {
-      if (t->GetName() == wxT("Clipping")) {
-         l = (LabelTrack *) t;
-         // copy LabelTrack here, so it can be undone on cancel
-         l->Copy(l->GetStartTime(), l->GetEndTime(), &original);
-         original->SetOffset(l->GetStartTime());
-         original->SetName(wxT("Clipping"));
+      if (t->GetName() == name) {
+         lt = (LabelTrack *)t;
          break;
       }
    }
 
-   if (!l) {
-      l = mFactory->NewLabelTrack();
-      l->SetName(_("Clipping"));
-      mTracks->Add((Track *) l);
-   }
+   if (!lt)
+      addedTrack.create(AddAnalysisTrack(name)), lt = addedTrack->get();
+   else
+      modifiedTrack.create(ModifyAnalysisTrack(lt, name)), lt = modifiedTrack->get();
 
    int count = 0;
 
@@ -130,12 +128,7 @@ bool EffectFindClipping::Process()
          sampleCount end = t->TimeToLongSamples(t1);
          sampleCount len = (sampleCount)(end - start);
 
-         if (!ProcessOne(l, count, t, start, len)) {
-            //put it back how it was
-            mTracks->Remove((Track *) l);
-            if(original) {
-               mTracks->Add((Track *) original);
-            }
+         if (!ProcessOne(lt, count, t, start, len)) {
             return false;
          }
       }
@@ -144,12 +137,17 @@ bool EffectFindClipping::Process()
       t = (WaveTrack *) waves.Next();
    }
 
+   // No cancellation, so commit the addition of the track.
+   if (addedTrack)
+      addedTrack->Commit();
+   if (modifiedTrack)
+      modifiedTrack->Commit();
    return true;
 }
 
-bool EffectFindClipping::ProcessOne(LabelTrack * l,
+bool EffectFindClipping::ProcessOne(LabelTrack * lt,
                                     int count,
-                                    WaveTrack * t,
+                                    const WaveTrack * wt,
                                     sampleCount start,
                                     sampleCount len)
 {
@@ -180,14 +178,14 @@ bool EffectFindClipping::ProcessOne(LabelTrack * l,
 
          block = s + blockSize > len ? len - s : blockSize;
 
-         t->Get((samplePtr)buffer, floatSample, start + s, block);
+         wt->Get((samplePtr)buffer, floatSample, start + s, block);
          ptr = buffer;
       }
 
       float v = fabs(*ptr++);
       if (v >= MAX_AUDIO) {
          if (startrun == 0) {
-            startTime = t->LongSamplesToTime(start + s);
+            startTime = wt->LongSamplesToTime(start + s);
             samps = 0;
          }
          else {
@@ -202,8 +200,8 @@ bool EffectFindClipping::ProcessOne(LabelTrack * l,
             samps++;
 
             if (stoprun >= mStop) {
-               l->AddLabel(SelectedRegion(startTime,
-                                          t->LongSamplesToTime(start + s - mStop)),
+               lt->AddLabel(SelectedRegion(startTime,
+                                          wt->LongSamplesToTime(start + s - mStop)),
                            wxString::Format(wxT("%lld of %lld"), (long long) startrun, (long long) (samps - mStop)));
                startrun = 0;
                stoprun = 0;

src/effects/FindClipping.h

@@ -52,7 +52,7 @@ public:
 private:
    // EffectFindCliping implementation
 
-   bool ProcessOne(LabelTrack *l, int count, WaveTrack * t,
+   bool ProcessOne(LabelTrack *lt, int count, const WaveTrack * wt,
                    sampleCount start, sampleCount len);
 
 private:

src/effects/vamp/VampEffect.cpp

@@ -419,6 +419,8 @@ bool VampEffect::Process()
       multiple = true;
    }
 
+   std::vector<Effect::AddedAnalysisTrack> addedTracks;
+
    while (left)
    {
       sampleCount lstart, rstart = 0;
@@ -484,20 +486,13 @@ bool VampEffect::Process()
          }
       }
 
-      LabelTrack *ltrack = mFactory->NewLabelTrack();
-
-      if (!multiple)
-      {
-         ltrack->SetName(GetName());
-      }
-      else
-      {
-         ltrack->SetName(wxString::Format(wxT("%s: %s"),
-                                          left->GetName().c_str(),
-                                          GetName().c_str()));
-      }
-
-      mTracks->Add(ltrack);
+      addedTracks.push_back(AddAnalysisTrack(
+         multiple
+         ? wxString::Format(wxT("%s: %s"),
+            left->GetName().c_str(), GetName().c_str())
+         : GetName()
+      ));
+      LabelTrack *ltrack = addedTracks.back().get();
 
       float **data = new float *[channels]; // ANSWER-ME: Vigilant Sentry marks this as memory leak, var "data" not deleted.
       for (int c = 0; c < channels; ++c)
@@ -576,6 +571,10 @@ bool VampEffect::Process()
       left = (WaveTrack *)iter.Next();
    }
 
+   // All completed without cancellation, so commit the addition of tracks now
+   for (auto &addedTrack : addedTracks)
+      addedTrack.Commit();
+
    return true;
 }