TrackShifter handles removal and reinsertion of clips

src/tracks/playabletrack/wavetrack/ui/WaveTrackView.cpp

@@ -10,6 +10,8 @@ Paul Licameli split from TrackPanel.cpp
 
 #include "WaveTrackView.h"
 
+#include <unordered_set>
+
 #include "CutlineHandle.h"
 
 #include "../../../../Experimental.h"
@@ -1363,8 +1365,48 @@ public:
       return desiredOffset;
    }
 
+   Intervals Detach() override
+   {
+      for ( auto &interval: mMoving ) {
+         auto pData = static_cast<WaveTrack::IntervalData*>( interval.Extra() );
+         auto pClip = pData->GetClip().get();
+         // interval will still hold the clip, so ignore the return:
+         (void) mpTrack->RemoveAndReturnClip(pClip);
+         mMigrated.erase(pClip);
+      }
+      return std::move( mMoving );
+   }
+
+   bool Attach( Intervals intervals ) override
+   {
+      for (auto &interval : intervals) {
+         auto pData = static_cast<WaveTrack::IntervalData*>( interval.Extra() );
+         auto pClip = pData->GetClip();
+         if ( !mpTrack->AddClip( pClip ) )
+            return false;
+         mMigrated.insert( pClip.get() );
+         mMoving.emplace_back( std::move( interval ) );
+      }
+      return true;
+   }
+
+   bool FinishMigration() override
+   {
+      auto rate = mpTrack->GetRate();
+      for (auto pClip : mMigrated) {
+         // Now that user has dropped the clip into a different track,
+         // make sure the sample rate matches the destination track.
+         pClip->Resample(rate);
+         pClip->MarkChanged();
+      }
+      return true;
+   }
+
 private:
    std::shared_ptr<WaveTrack> mpTrack;
+
+   // Clips that may require resampling
+   std::unordered_set<WaveClip *> mMigrated;
 };
 
 using MakeWaveTrackShifter = MakeTrackShifter::Override<WaveTrack>;

src/tracks/ui/TimeShiftHandle.cpp

@@ -275,6 +275,21 @@ bool TrackShifter::CommonMayMigrateTo(Track &otherTrack)
    return false;
 }
 
+auto TrackShifter::Detach() -> Intervals
+{
+   return {};
+}
+
+bool TrackShifter::Attach( Intervals )
+{
+   return true;
+}
+
+bool TrackShifter::FinishMigration()
+{
+   return true;
+}
+
 void TrackShifter::InitIntervals()
 {
    mMoving.clear();
@@ -811,19 +826,20 @@ namespace {
       return ok;
    }
 
+   [[noreturn]] void MigrationFailure() {
+      // Tracks may be in an inconsistent state; throw to the application
+      // handler which restores consistency from undo history
+      throw SimpleMessageBoxException{
+         XO("Could not shift between tracks")};
+   }
+
    struct TemporaryClipRemover {
       TemporaryClipRemover( ClipMoveState &clipMoveState )
          : state( clipMoveState )
       {
          // Pluck the moving clips out of their tracks
-         for ( auto &trackClip : state.capturedClipArray ) {
+         for (auto &pair : state.shifters)
-            WaveClip *const pSrcClip = trackClip.clip;
+            detached[pair.first] = pair.second->Detach();
-            if (pSrcClip)
-               trackClip.holder =
-                  // Assume track is wave because it has a clip
-                  static_cast<WaveTrack*>(trackClip.track)->
-                     RemoveAndReturnClip(pSrcClip);
-         }
       }
 
       void Fail()
@@ -831,9 +847,11 @@ namespace {
          // Cause destructor to put all clips back where they came from
          for ( auto &trackClip : state.capturedClipArray )
             trackClip.dstTrack = static_cast<WaveTrack*>(trackClip.track);
+         failed = true;
       }
 
-      ~TemporaryClipRemover()
+      void Reinsert(
+         std::unordered_map< Track*, Track* > &correspondence )
       {
          // Complete (or roll back) the vertical move.
          // Put moving clips into their destination tracks
@@ -842,14 +860,23 @@ namespace {
             WaveClip *const pSrcClip = trackClip.clip;
             if (pSrcClip) {
                const auto dstTrack = trackClip.dstTrack;
-               // To do:  check and propagate the error!  Can't from a dtor
-               (void) dstTrack->AddClip(trackClip.holder);
                trackClip.track = dstTrack;
             }
          }
+
+         for (auto &pair : detached) {
+            auto pTrack = pair.first;
+            if (!failed && correspondence.count(pTrack))
+               pTrack = correspondence[pTrack];
+            auto &pShifter = state.shifters[pTrack];
+            if (!pShifter->Attach( std::move( pair.second ) ))
+               MigrationFailure();
+         }
       }
 
       ClipMoveState &state;
+      std::unordered_map<Track*, TrackShifter::Intervals> detached;
+      bool failed = false;
    };
 }
 
@@ -879,11 +906,13 @@ bool TimeShiftHandle::DoSlideVertical
    if (!ok) {
       // Failure, even with using tolerance.
       remover.Fail();
+      remover.Reinsert( correspondence );
       return false;
    }
 
    // Make the offset permanent; start from a "clean slate"
    state.mMouseClickX = xx;
+   remover.Reinsert( correspondence );
    return true;
 }
 
@@ -1013,24 +1042,9 @@ UIHandle::Result TimeShiftHandle::Release
    if ( !mDidSlideVertically && mClipMoveState.hSlideAmount == 0 )
       return result;
 
-   for ( auto &trackClip : mClipMoveState.capturedClipArray )
+   for ( auto &pair : mClipMoveState.shifters )
-   {
+      if ( !pair.second->FinishMigration() )
-      WaveClip* pWaveClip = trackClip.clip;
+         MigrationFailure();
-      // Note that per AddClipsToCaptured(Track *t, double t0, double t1),
-      // in the non-WaveTrack case, the code adds a NULL clip to capturedClipArray,
-      // so we have to check for that any time we're going to deref it.
-      // Previous code did not check it here, and that caused bug 367 crash.
-      if (pWaveClip &&
-         trackClip.track != trackClip.origTrack)
-      {
-         // Now that user has dropped the clip into a different track,
-         // make sure the sample rate matches the destination track.
-         // Assume the clip was dropped in a wave track
-         pWaveClip->Resample
-            (static_cast<WaveTrack*>(trackClip.track)->GetRate());
-         pWaveClip->MarkChanged();
-      }
-   }
    
    TranslatableString msg;
    bool consolidate;

src/tracks/ui/TimeShiftHandle.h

@@ -79,6 +79,26 @@ public:
    /*! Default implementation returns false */
    virtual bool MayMigrateTo( Track &otherTrack );
 
+   //! Remove all moving intervals from the track, if possible
+   /*! Default implementation does nothing */
+   virtual Intervals Detach();
+
+   //! Put moving intervals into the track, which may have migrated from another
+   /*! @return success
+   
+       In case of failure, track states are unspecified
+    
+       Default implementation does nothing and returns true */
+   virtual bool Attach( Intervals intervals );
+
+   //! When dragging is done, do (once) the final steps of migration (which may be expensive)
+   /*! @return success
+   
+       In case of failure, track states are unspecified
+    
+       Default implementation does nothing and returns true */
+   virtual bool FinishMigration();
+
 protected:
    /*! Unfix any of the intervals that intersect the given one; may be useful to override `SelectInterval()` */
    void CommonSelectInterval( const TrackInterval &interval );
@@ -133,7 +153,6 @@ public:
 
    // These fields are used only during time-shift dragging
    WaveTrack *dstTrack;
-   std::shared_ptr<WaveClip> holder;
 };
 
 using TrackClipArray = std::vector <TrackClip>;