From 0db0c6e0d55c177a4d3acccf23960e049ff10da6 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Thu, 4 Oct 2018 13:17:21 -0400 Subject: [PATCH 1/3] Reorganize some commit logic in DirManager::SetProject --- src/DirManager.cpp | 233 +++++++++++++++++++++++---------------------- src/DirManager.h | 4 +- 2 files changed, 123 insertions(+), 114 deletions(-) diff --git a/src/DirManager.cpp b/src/DirManager.cpp index 41900141a..c1b518dcc 100644 --- a/src/DirManager.cpp +++ b/src/DirManager.cpp @@ -474,16 +474,65 @@ void DirManager::CleanDir( RecursivelyRemove(dirPathArray, count, countFiles, flags | kCleanDirs, msg); } -bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const bool bCreate) -{ - bool moving = true; - wxString oldPath = this->projPath; - wxString oldName = this->projName; - wxString oldFull = projFull; - wxString oldLoc = projFull; - if (oldLoc == wxT("")) - oldLoc = mytemp; +namespace { + struct PathRestorer { + PathRestorer( + bool &commitFlag, wxString &path, wxString &name, wxString &full ) + : committed( commitFlag ) + , 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; + }; +} + +bool DirManager::SetProject( + wxString& newProjPath, const wxString& newProjName, const bool bCreate) +{ + bool committed = false; + + // Save old state of paths in case of failure + PathRestorer pathRestorer{ committed, projPath, projName, projFull }; + + // Remember old path to be cleaned up in case of successful move + const auto oldFull = projFull; + + // Choose new paths if (newProjPath == wxT("")) newProjPath = ::wxGetCwd(); @@ -494,22 +543,11 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const else this->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 (!wxMkdir(projFull)) { - this->projFull = oldFull; - this->projPath = oldPath; - this->projName = oldName; - return false; - } - else - created = true; - } + if (!wxDirExists(projFull) && + !wxMkdir(projFull)) + return false; #ifdef __UNIX__ chmod(OSFILENAME(projFull), 0775); @@ -519,14 +557,14 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const chmod(OSFILENAME(projFull), 0775); #endif - } else { - if (!wxDirExists(projFull)) { - this->projFull = oldFull; - this->projPath = oldPath; - this->projName = oldName; - return false; - } } + else if (!wxDirExists(projFull)) + return false; + + // Be prepared to un-create directory on failure + Maybe dirCleaner; + if (bCreate) + dirCleaner.create( committed, projFull ); /* Move all files into this NEW directory. Files which are "locked" get copied instead of moved. (This happens when @@ -534,111 +572,79 @@ 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; + bool moving = true; + size_t trueTotal = 0; + wxArrayString newPaths; { - /*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(); - bool success = true; - int count = 0; - wxArrayString newPaths; - for (const auto &pair : mBlockFileHash) - { + for (const auto &pair : mBlockFileHash) { + if( progress.Update(newPaths.size(), total) != ProgressResult::Success ) + return false; + wxString newPath; - BlockFilePtr b = pair.second.lock(); - if (b) { - // FIXME: TRAP_ERR - // JKC: The 'success' variable and recovery strategy looks - // broken/bogus to me. Would need to be using &= to catch - // failure in one of the copies/moves. Besides which, - // our temporary files are going to be deleted when we exit - // 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; + if (auto b = pair.second.lock()) { + moving = moving && !b->IsLocked(); + auto result = CopyToNewProjectDirectory( &*b ); + if (!result.first) + return false; + newPath = result.second; + ++trueTotal; } - newPaths.push_back( newPath ); } + } - // in case there are any nulls - trueTotal = count; + // We have built all of the new file tree. + committed = true; - if (success) { - auto size = newPaths.size(); - wxASSERT( size == mBlockFileHash.size() ); + auto size = newPaths.size(); + wxASSERT( size == mBlockFileHash.size() ); - // Commit changes to filenames in the BlockFile objects, and removal - // of files at old paths, ONLY NOW! This must be nothrow. + // Commit changes to filenames in the BlockFile objects, and removal + // of files at old paths, ONLY NOW! This must be nothrow. - // This copy-then-delete procedure is needed to make it safe to - // attempt save to another storage device, but fail. + // This copy-then-delete procedure is needed to make it safe to + // attempt save to another storage device, but fail. - // It has the consequence that saving a project from one part of - // 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. + // It has the consequence that saving a project from one part of + // 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. - // But I don't know whether wxWidgets gives us a reliable means to - // distinguish that case. + // But I don't know whether wxWidgets gives us a reliable means to + // distinguish that case. - // I will err on the side of safety and simplicity and follow the - // same procedure in all cases. + // I will err on the side of safety and simplicity and follow the + // same procedure in all cases. - size_t ii = 0; - for (const auto &pair : mBlockFileHash) - { - BlockFilePtr b = pair.second.lock(); + size_t ii = 0; + for (const auto &pair : mBlockFileHash) + { + BlockFilePtr b = pair.second.lock(); - if (b) { - if (moving || !b->IsLocked()) { - auto result = b->GetFileName(); - oldPath = result.name.GetFullPath(); - if (!oldPath.empty()) - wxRemoveFile( oldPath ); - } - - if (ii < size) - b->SetFileName( - wxFileNameWrapper{ wxFileName{ newPaths[ii] } } ); - } - - ++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) - CleanDir( - cleanupLoc2, - wxEmptyString, - wxEmptyString, - _("Cleaning up after failed save"), - kCleanTopDirToo); - - return false; + if (ii < size) + b->SetFileName( + wxFileNameWrapper{ wxFileName{ newPaths[ii] } } ); } + + ++ii; } // Some subtlety; SetProject is used both to move a temp project @@ -661,6 +667,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() ? mytemp : oldFull; CleanDir( cleanupLoc1, wxEmptyString, // EmptyString => ALL directories. diff --git a/src/DirManager.h b/src/DirManager.h index 67fbed234..98a915356 100644 --- a/src/DirManager.h +++ b/src/DirManager.h @@ -62,7 +62,9 @@ class PROFILE_DLL_API DirManager final : public XMLTagHandler { // 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); + bool SetProject( + wxString& newProjPath, // assigns it if empty + const wxString& newProjName, const bool bCreate); wxString GetProjectDataDir(); wxString GetProjectName(); From ec506ab52b2038ee57e715123f459108fe8884cc Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sun, 7 Oct 2018 16:50:11 -0400 Subject: [PATCH 2/3] Define DirManager::ProjectSetter so that commit steps can be deferred --- src/DirManager.cpp | 124 ++++++++++++++++++++++++++++++++++----------- src/DirManager.h | 19 +++++++ 2 files changed, 114 insertions(+), 29 deletions(-) diff --git a/src/DirManager.cpp b/src/DirManager.cpp index c1b518dcc..d5d16bfd2 100644 --- a/src/DirManager.cpp +++ b/src/DirManager.cpp @@ -521,50 +521,100 @@ namespace { }; } -bool DirManager::SetProject( - wxString& newProjPath, const wxString& newProjName, const bool bCreate) +struct DirManager::ProjectSetter::Impl { - bool committed = false; + 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, projPath, projName, projFull }; + PathRestorer pathRestorer{ + committed, + dirManager.projPath, dirManager.projName, dirManager.projFull }; + // Another RAII object + // Be prepared to un-create directory on failure + Maybe dirCleaner; + + // State variables to carry over into Commit() // Remember old path to be cleaned up in case of successful move - const auto oldFull = projFull; + 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( 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; - this->projName = newProjName; + dirManager.projPath = newProjPath; + 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; // Verify new paths, maybe creating a directory if (bCreate) { - if (!wxDirExists(projFull) && - !wxMkdir(projFull)) - return false; + if (!wxDirExists(dirManager.projFull) && + !wxMkdir(dirManager.projFull)) + return; #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)) - return false; + else if (!wxDirExists(dirManager.projFull)) + return; // Be prepared to un-create directory on failure - Maybe dirCleaner; if (bCreate) - dirCleaner.create( committed, projFull ); + dirCleaner.create( committed, dirManager.projFull ); /* Move all files into this NEW directory. Files which are "locked" get copied instead of moved. (This happens when @@ -572,9 +622,8 @@ bool DirManager::SetProject( saved version of the old project must not be moved, otherwise the old project would not be safe.) */ - bool moving = true; - size_t trueTotal = 0; - wxArrayString newPaths; + moving = true; + trueTotal = 0; { /* i18n-hint: This title appears on a dialog that indicates the progress @@ -582,18 +631,18 @@ bool DirManager::SetProject( ProgressDialog progress(_("Progress"), _("Saving project data files")); - int total = mBlockFileHash.size(); + int total = dirManager.mBlockFileHash.size(); - for (const auto &pair : mBlockFileHash) { + for (const auto &pair : dirManager.mBlockFileHash) { if( progress.Update(newPaths.size(), total) != ProgressResult::Success ) - return false; + return; wxString newPath; if (auto b = pair.second.lock()) { moving = moving && !b->IsLocked(); - auto result = CopyToNewProjectDirectory( &*b ); + auto result = dirManager.CopyToNewProjectDirectory( &*b ); if (!result.first) - return false; + return; newPath = result.second; ++trueTotal; } @@ -601,11 +650,19 @@ bool DirManager::SetProject( } } + ok = true; +} + +void DirManager::ProjectSetter::Impl::Commit() +{ + wxASSERT( ok ); + // We have built all of the new file tree. + // So cancel the destructor actions of the RAII objects. committed = true; auto size = newPaths.size(); - wxASSERT( size == mBlockFileHash.size() ); + wxASSERT( size == dirManager.mBlockFileHash.size() ); // Commit changes to filenames in the BlockFile objects, and removal // of files at old paths, ONLY NOW! This must be nothrow. @@ -627,7 +684,7 @@ bool DirManager::SetProject( // same procedure in all cases. size_t ii = 0; - for (const auto &pair : mBlockFileHash) + for (const auto &pair : dirManager.mBlockFileHash) { BlockFilePtr b = pair.second.lock(); @@ -667,7 +724,7 @@ bool DirManager::SetProject( // 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() ? mytemp : oldFull; + auto cleanupLoc1 = oldFull.empty() ? dirManager.mytemp : oldFull; CleanDir( cleanupLoc1, wxEmptyString, // EmptyString => ALL directories. @@ -680,6 +737,15 @@ bool DirManager::SetProject( //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; } diff --git a/src/DirManager.h b/src/DirManager.h index 98a915356..2091a8eb2 100644 --- a/src/DirManager.h +++ b/src/DirManager.h @@ -59,9 +59,28 @@ 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 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. + // 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); From e87e15f489f33dfda31da397f0ee856e72157e4d Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sun, 7 Oct 2018 16:50:49 -0400 Subject: [PATCH 3/3] Proper deferring of DirManager's commit steps when saving-as --- src/Project.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/Project.cpp b/src/Project.cpp index 637dc7274..f51378f41 100644 --- a/src/Project.cpp +++ b/src/Project.cpp @@ -3959,11 +3959,13 @@ bool AudacityProject::DoSave (const bool fromSaveAs, if (!success) return false; + Maybe 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 - // provision to undo the effects of SetProject -- but it is very unlikely - // that this will happen: only renaming and removing of files happens, - // not writes that might exhaust space. So DO give a second dialog in - // case the unusual happens. + // It is very unlikely that errors will happen: + // only renaming and removing of files, 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