Reorganize commit/rollback of SetProject, prior to fixing bug1979

src/DirManager.cpp

@@ -474,59 +474,147 @@ void DirManager::CleanDir(
    RecursivelyRemove(dirPathArray, count, countFiles, flags | kCleanDirs, msg);
 }
 
-bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const bool bCreate)
+namespace {
-{
+   struct PathRestorer {
-   bool moving = true;
+      PathRestorer(
-   wxString oldPath = this->projPath;
+         bool &commitFlag, wxString &path, wxString &name, wxString &full )
-   wxString oldName = this->projName;
+         : committed( commitFlag )
-   wxString oldFull = projFull;
-   wxString oldLoc = projFull;
-   if (oldLoc == wxT(""))
-      oldLoc = mytemp;
 
+         , projPath( path )
+         , projName( name )
+         , projFull( full )
+
+         , oldPath( path )
+         , oldName( name )
+         , oldFull( full )
+      {}
+      
+      ~PathRestorer()
+      {
+         if (!committed)
+            projFull = oldFull, projName = oldName, projPath = oldPath;
+      }
+
+      bool &committed;
+      wxString &projPath, &projName, &projFull;
+      const wxString oldPath, oldName, oldFull;
+   };
+
+   struct DirCleaner {
+      DirCleaner( bool &commitFlag, const wxString &path )
+         : committed( commitFlag )
+         , fullPath( path )
+      {}
+      ~DirCleaner()
+      {
+         if (!committed)
+            DirManager::CleanDir(
+               fullPath,
+               wxEmptyString,
+               wxEmptyString,
+               _("Cleaning up after failed save"),
+               kCleanTopDirToo);
+      }
+
+      bool &committed;
+      wxString fullPath;
+   };
+}
+
+struct DirManager::ProjectSetter::Impl
+{
+   Impl(
+      DirManager &dm,
+      wxString& newProjPath, const wxString& newProjName, const bool bCreate );
+
+   void Commit();
+
+   DirManager &dirManager;
+   bool committed{ false };
+
+   // RAII object
+   // Save old state of paths in case of failure
+   PathRestorer pathRestorer{
+      committed,
+      dirManager.projPath, dirManager.projName, dirManager.projFull };
+
+   // Another RAII object
+   // Be prepared to un-create directory on failure
+   Maybe<DirCleaner> dirCleaner;
+   
+   // State variables to carry over into Commit()
+   // Remember old path to be cleaned up in case of successful move
+   wxString oldFull{ dirManager.projFull };
+   wxArrayString newPaths;
+   size_t trueTotal{ 0 };
+   bool moving{ true };
+
+   // Make this true only after successful construction
+   bool ok{ false };
+};
+
+DirManager::ProjectSetter::ProjectSetter(
+   DirManager &dirManager,
+   wxString& newProjPath, const wxString& newProjName, const bool bCreate )
+   : mpImpl{
+      std::make_unique<Impl>( dirManager, newProjPath, newProjName, bCreate )
+   }
+{
+   
+}
+
+DirManager::ProjectSetter::~ProjectSetter()
+{
+}
+
+bool DirManager::ProjectSetter::Ok()
+{
+   return mpImpl->ok;
+}
+
+
+void DirManager::ProjectSetter::Commit()
+{
+   mpImpl->Commit();
+}
+
+DirManager::ProjectSetter::Impl::Impl(
+   DirManager &dm,
+   wxString& newProjPath, const wxString& newProjName, const bool bCreate )
+: dirManager{ dm }
+{
+   // Choose new paths
    if (newProjPath == wxT(""))
       newProjPath = ::wxGetCwd();
 
-   this->projPath = newProjPath;
+   dirManager.projPath = newProjPath;
-   this->projName = newProjName;
+   dirManager.projName = newProjName;
    if (newProjPath.Last() == wxFILE_SEP_PATH)
-      this->projFull = newProjPath + newProjName;
+      dirManager.projFull = newProjPath + newProjName;
    else
-      this->projFull = newProjPath + wxFILE_SEP_PATH + newProjName;
+      dirManager.projFull = newProjPath + wxFILE_SEP_PATH + newProjName;
-
-   wxString cleanupLoc1=oldLoc;
-   wxString cleanupLoc2=projFull;
-
-   bool created = false;
 
+   // Verify new paths, maybe creating a directory
    if (bCreate) {
-      if (!wxDirExists(projFull)) {
+      if (!wxDirExists(dirManager.projFull) &&
-         if (!wxMkdir(projFull)) {
+          !wxMkdir(dirManager.projFull))
-            this->projFull = oldFull;
+         return;
-            this->projPath = oldPath;
-            this->projName = oldName;
-            return false;
-         }
-         else
-            created = true;
-      }
 
       #ifdef __UNIX__
-      chmod(OSFILENAME(projFull), 0775);
+      chmod(OSFILENAME(dirManager.projFull), 0775);
       #endif
 
       #ifdef __WXMAC__
-      chmod(OSFILENAME(projFull), 0775);
+      chmod(OSFILENAME(dirManager.projFull), 0775);
       #endif
 
-   } else {
-      if (!wxDirExists(projFull)) {
-         this->projFull = oldFull;
-         this->projPath = oldPath;
-         this->projName = oldName;
-         return false;
-      }
    }
+   else if (!wxDirExists(dirManager.projFull))
+      return;
+
+   // Be prepared to un-create directory on failure
+   if (bCreate)
+      dirCleaner.create( committed, dirManager.projFull );
 
    /* Move all files into this NEW directory.  Files which are
       "locked" get copied instead of moved.  (This happens when
@@ -534,111 +622,86 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const
       saved version of the old project must not be moved,
       otherwise the old project would not be safe.) */
 
-   int trueTotal = 0;
+   moving = true;
+   trueTotal = 0;
 
    {
-      /*i18n-hint: This title appears on a dialog that indicates the progress in doing something.*/
+      /* i18n-hint: This title appears on a dialog that indicates the progress
+         in doing something.*/
       ProgressDialog progress(_("Progress"),
          _("Saving project data files"));
 
-      int total = mBlockFileHash.size();
+      int total = dirManager.mBlockFileHash.size();
+
+      for (const auto &pair : dirManager.mBlockFileHash) {
+         if( progress.Update(newPaths.size(), total) != ProgressResult::Success )
+            return;
 
-      bool success = true;
-      int count = 0;
-      wxArrayString newPaths;
-      for (const auto &pair : mBlockFileHash)
-      {
          wxString newPath;
-         BlockFilePtr b = pair.second.lock();
+         if (auto b = pair.second.lock()) {
-         if (b) {
+            moving = moving && !b->IsLocked();
-            // FIXME: TRAP_ERR
+            auto result = dirManager.CopyToNewProjectDirectory( &*b );
-            // JKC: The 'success' variable and recovery strategy looks 
+            if (!result.first)
-            // broken/bogus to me.  Would need to be using &= to catch 
+               return;
-            // failure in one of the copies/moves.  Besides which,
+            newPath = result.second;
-            // our temporary files are going to be deleted when we exit 
+            ++trueTotal;
-            // anyway, if saving from temporary to named project.
-
-            if( progress.Update(count, total) != ProgressResult::Success )
-               success = false;
-            else {
-               moving = moving && !b->IsLocked();
-               auto result = CopyToNewProjectDirectory( &*b );
-               success = result.first;
-               newPath = result.second;
-            }
-
-            if (success) {
-               count++;
-            }
-            else
-               break;
          }
-
          newPaths.push_back( newPath );
       }
+   }
 
-      // in case there are any nulls
+   ok = true;
-      trueTotal = count;
+}
 
-      if (success) {
+void DirManager::ProjectSetter::Impl::Commit()
-         auto size = newPaths.size();
+{
-         wxASSERT( size == mBlockFileHash.size() );
+   wxASSERT( ok );
 
-         // Commit changes to filenames in the BlockFile objects, and removal
+   // We have built all of the new file tree.
-         // of files at old paths, ONLY NOW!  This must be nothrow.
+   // So cancel the destructor actions of the RAII objects.
+   committed = true;
 
-         // This copy-then-delete procedure is needed to make it safe to
+   auto size = newPaths.size();
-         // attempt save to another storage device, but fail.
+   wxASSERT( size == dirManager.mBlockFileHash.size() );
 
-         // It has the consequence that saving a project from one part of
+   // Commit changes to filenames in the BlockFile objects, and removal
-         // the device to another will not succeed unless there is sufficient
+   // of files at old paths, ONLY NOW!  This must be nothrow.
-         // space to hold originals and copies at the same time.  Perhaps the
-         // extra cautions are not needed in that case, and the old procedure
-         // of renaming first, and reversing the renamings in case of failure,
-         // could still work safely.
 
-         // But I don't know whether wxWidgets gives us a reliable means to
+   // This copy-then-delete procedure is needed to make it safe to
-         // distinguish that case.
+   // attempt save to another storage device, but fail.
 
-         // I will err on the side of safety and simplicity and follow the
+   // It has the consequence that saving a project from one part of
-         // same procedure in all cases.
+   // the device to another will not succeed unless there is sufficient
+   // space to hold originals and copies at the same time.  Perhaps the
+   // extra cautions are not needed in that case, and the old procedure
+   // of renaming first, and reversing the renamings in case of failure,
+   // could still work safely.
 
-         size_t ii = 0;
+   // But I don't know whether wxWidgets gives us a reliable means to
-         for (const auto &pair : mBlockFileHash)
+   // distinguish that case.
-         {
-            BlockFilePtr b = pair.second.lock();
 
-            if (b) {
+   // I will err on the side of safety and simplicity and follow the
-               if (moving || !b->IsLocked()) {
+   // same procedure in all cases.
-                  auto result = b->GetFileName();
-                  oldPath = result.name.GetFullPath();
-                  if (!oldPath.empty())
-                     wxRemoveFile( oldPath );
-               }
 
-               if (ii < size)
+   size_t ii = 0;
-                  b->SetFileName(
+   for (const auto &pair : dirManager.mBlockFileHash)
-                     wxFileNameWrapper{ wxFileName{ newPaths[ii] } } );
+   {
-            }
+      BlockFilePtr b = pair.second.lock();
 
-            ++ii;
+      if (b) {
+         if (moving || !b->IsLocked()) {
+            auto result = b->GetFileName();
+            auto oldPath = result.name.GetFullPath();
+            if (!oldPath.empty())
+               wxRemoveFile( oldPath );
          }
-      }
-      else {
-         this->projFull = oldFull;
-         this->projPath = oldPath;
-         this->projName = oldName;
 
-         if (created)
+         if (ii < size)
-            CleanDir(
+            b->SetFileName(
-               cleanupLoc2,
+               wxFileNameWrapper{ wxFileName{ newPaths[ii] } } );
-               wxEmptyString,
-               wxEmptyString,
-               _("Cleaning up after failed save"),
-               kCleanTopDirToo);
-
-         return false;
       }
+
+      ++ii;
    }
 
    // Some subtlety; SetProject is used both to move a temp project
@@ -661,6 +724,7 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const
       // that we didn't put there, but that Finder may insert into the folders,
       // and mercilessly remove them, in addition to removing the directories.
 
+      auto cleanupLoc1 = oldFull.empty() ? dirManager.mytemp : oldFull;
       CleanDir(
          cleanupLoc1, 
          wxEmptyString, // EmptyString => ALL directories.
@@ -673,6 +737,15 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const
       //Dont know if this will make the project dirty, but I doubt it. (mchinen)
       //      count += RecursivelyEnumerate(cleanupLoc2, dirlist, wxEmptyString, false, true);
    }
+}
+
+bool DirManager::SetProject(
+   wxString& newProjPath, const wxString& newProjName, const bool bCreate)
+{
+   ProjectSetter setter{ *this, newProjPath, newProjName, bCreate };
+   if (!setter.Ok())
+      return false;
+   setter.Commit();
    return true;
 }
 

src/DirManager.h

@@ -59,10 +59,31 @@ class PROFILE_DLL_API DirManager final : public XMLTagHandler {
 
    static void SetTempDir(const wxString &_temp) { globaltemp = _temp; }
 
+   class ProjectSetter
+   {
+   public:
+      ProjectSetter(
+         DirManager &dirManager,
+         wxString& newProjPath,  // assigns it if empty
+         const wxString& newProjName, const bool bCreate);
+      ~ProjectSetter();
+
+      bool Ok();
+      void Commit();
+
+   private:
+      struct Impl;
+      std::unique_ptr<Impl> mpImpl;
+   };
+
    // Returns true on success.
    // If SetProject is told NOT to create the directory
    // but it doesn't already exist, SetProject fails and returns false.
-   bool SetProject(wxString& newProjPath, wxString& newProjName, const bool bCreate);
+   // This function simply creates a ProjectSetter and commits it if successful.
+   // Using ProjectSetter directly allows separation of those steps.
+   bool SetProject(
+      wxString& newProjPath, // assigns it if empty
+      const wxString& newProjName, const bool bCreate);
 
    wxString GetProjectDataDir();
    wxString GetProjectName();

src/Project.cpp

@@ -3959,11 +3959,13 @@ bool AudacityProject::DoSave (const bool fromSaveAs,
    if (!success)
       return false;
 
+   Maybe<DirManager::ProjectSetter> pSetter;
+
    if (fromSaveAs && !bWantSaveCopy) {
       // We are about to move files from the current directory to
       // the NEW directory.  We need to make sure files that belonged
       // to the last saved project don't get erased, so we "lock" them, so that
-      // SetProject() copies instead of moves the files.
+      // ProjectSetter's constructor copies instead of moves the files.
       // (Otherwise the NEW project would be fine, but the old one would
       // be empty of all of its files.)
 
@@ -3977,19 +3979,17 @@ bool AudacityProject::DoSave (const bool fromSaveAs,
 
       // This renames the project directory, and moves or copies
       // all of our block files over.
-      success = mDirManager->SetProject(projPath, projName, true);
+      pSetter.create( *mDirManager, projPath, projName, true );
 
-      if (!success)
+      if (!pSetter->Ok())
          return false;
    }
 
    // Commit the writing of the .aup only now, after we know that the _data
    // folder also saved with no problems.
-   // Error recovery in case this fails might not be correct -- there is no
+   // It is very unlikely that errors will happen:
-   // provision to undo the effects of SetProject -- but it is very unlikely
+   // only renaming and removing of files, not writes that might exhaust space.
-   // that this will happen:  only renaming and removing of files happens,
+   // So DO give a second dialog in case the unusual happens.
-   // not writes that might exhaust space.  So DO give a second dialog in
-   // case the unusual happens.
    success = success && GuardedCall< bool >( [&] {
          saveFile.PostCommit();
          return true;
@@ -4000,6 +4000,9 @@ bool AudacityProject::DoSave (const bool fromSaveAs,
 
    // SAVE HAS SUCCEEDED -- following are further no-fail commit operations.
 
+   if (pSetter)
+      pSetter->Commit();
+
    if ( !bWantSaveCopy )
    {
       // Now that we have saved the file, we can DELETE the auto-saved version