Dont delete sample blocks prematurely (#648)

* Revert "AUP3: Don't delete sample blocks prematurely"

This reverts commit c1884349d595a2fb889c90f209ea4af3d1c70e1d.

* "Don't delete sample blocks prematurely" fixed otherwise...

... and very simply.

Problem was that, only for an interactive effect (like Bass and Treble), the
save point was created, rolled back, created again, then committed.

But (unlike with the non-savepoint commands, even if savepoint is outermost),
rolling back a savepoint really just rewinds it without removing it -- therefore
the second savepoint was inner, but the first (outer) was never committed, so
some changes failed to persist.

Solution:  add a commit after rollback of savepoint to implement destructor
of AutoCommitTransaction.

The reversion of c188434 also leaves AutoCommitTransaction as a better RAII
style operation.  Rollback changes by default -- keep changes only if
success is explicitly indicated.

* Rename AutoCommitTransaction as TransactionScope...

... More appropriately, since it's now the rollback that is automatic but
the commit that must be explicit

src/ProjectFileIO.cpp

@@ -2444,7 +2444,7 @@ int ProjectFileIO::get_varint(const unsigned char *ptr, int64_t *out)
    return 9;
 }
 
-AutoCommitTransaction::AutoCommitTransaction(ProjectFileIO &projectFileIO,
+TransactionScope::TransactionScope(ProjectFileIO &projectFileIO,
                                              const char *name)
 :  mIO(projectFileIO),
    mName(name)
@@ -2455,11 +2455,15 @@ AutoCommitTransaction::AutoCommitTransaction(ProjectFileIO &projectFileIO,
       throw SimpleMessageBoxException( XO("Database error") );
 }
 
-AutoCommitTransaction::~AutoCommitTransaction()
+TransactionScope::~TransactionScope()
 {
    if (mInTrans)
    {
-      if (!mIO.TransactionCommit(mName))
+      // Rollback AND REMOVE the transaction
+      // -- must do both; rolling back a savepoint only rewinds it
+      // without removing it, unlike the ROLLBACK command
+      if (!(mIO.TransactionRollback(mName) &&
+            mIO.TransactionCommit(mName) ) )
       {
          // Do not throw from a destructor!
          // This has to be a no-fail cleanup that does the best that it can.
@@ -2467,13 +2471,13 @@ AutoCommitTransaction::~AutoCommitTransaction()
    }
 }
 
-bool AutoCommitTransaction::Rollback()
+bool TransactionScope::Commit()
 {
    if ( !mInTrans )
       // Misuse of this class
       THROW_INCONSISTENCY_EXCEPTION;
 
-   mInTrans = !mIO.TransactionRollback(mName);
+   mInTrans = !mIO.TransactionCommit(mName);
 
    return mInTrans;
 }

src/ProjectFileIO.h

@@ -24,7 +24,7 @@ struct sqlite3_stmt;
 struct sqlite3_value;
 
 class AudacityProject;
-class AutoCommitTransaction;
+class TransactionScope;
 class DBConnection;
 class ProjectSerializer;
 class SqliteSampleBlock;
@@ -240,14 +240,16 @@ private:
 };
 
 // Make a savepoint (a transaction, possibly nested) with the given name;
+// roll it back at destruction time, unless an explicit Commit() happened first.
+// Commit() must not be called again after one successful call.
 // An exception is thrown from the constructor if the transaction cannot open.
-class AutoCommitTransaction
+class TransactionScope
 {
 public:
-   AutoCommitTransaction(ProjectFileIO &projectFileIO, const char *name);
-   ~AutoCommitTransaction();
+   TransactionScope(ProjectFileIO &projectFileIO, const char *name);
+   ~TransactionScope();
 
-   bool Rollback();
+   bool Commit();
 
 private:
    ProjectFileIO &mIO;

src/ProjectManager.cpp

@@ -740,7 +740,7 @@ void ProjectManager::OnCloseWindow(wxCloseEvent & event)
    projectFileIO.SetBypass();
 
    {
-      AutoCommitTransaction trans(projectFileIO, "Shutdown");
+      TransactionScope trans(projectFileIO, "Shutdown");
 
       // This can reduce reference counts of sample blocks in the project's
       // tracks.
@@ -748,6 +748,8 @@ void ProjectManager::OnCloseWindow(wxCloseEvent & event)
 
       // Delete all the tracks to free up memory
       tracks.Clear();
+
+      trans.Commit();
    }
 
    // We're all done with the project file, so close it now

src/effects/Effect.cpp

@@ -1215,7 +1215,7 @@ bool Effect::DoEffect(double projectRate,
    // This is for performance purposes only, no additional recovery implied
    auto &pProject = *const_cast<AudacityProject*>(FindProject()); // how to remove this const_cast?
    auto &pIO = ProjectFileIO::Get(pProject);
-   AutoCommitTransaction trans(pIO, "Effect");
+   TransactionScope trans(pIO, "Effect");
 
    // Update track/group counts
    CountWaveTracks();
@@ -1240,6 +1240,9 @@ bool Effect::DoEffect(double projectRate,
          // LastUsedDuration may have been modified by Preview.
          SetDuration(oldDuration);
       }
+      else
+         trans.Commit();
+
       End();
       ReplaceProcessedTracks( false );
    } );