Merge pull request #909 from Paul-Licameli/Bug2764

Bug2764 Open Project... (under Scriptables in Extra menus) can corrupt a project

src/AudacityApp.cpp

@@ -813,25 +813,8 @@ bool AudacityApp::MRUOpen(const FilePath &fullPathStr) {
          if (ProjectFileManager::IsAlreadyOpen(fullPathStr))
             return false;
 
-         // DMM: If the project is dirty, that means it's been touched at
+         ( void ) ProjectManager::OpenProject( proj, fullPathStr,
-         // all, and it's not safe to open a NEW project directly in its
+               true /* addtohistory */, false /* reuseNonemptyProject */ );
-         // place.  Only if the project is brand-NEW clean and the user
-         // hasn't done any action at all is it safe for Open to take place
-         // inside the current project.
-         //
-         // If you try to Open a NEW project inside the current window when
-         // there are no tracks, but there's an Undo history, etc, then
-         // bad things can happen, including data files moving to the NEW
-         // project directory, etc.
-         if (proj && (
-            ProjectHistory::Get( *proj ).GetDirty() ||
-            !TrackList::Get( *proj ).empty()
-         ) )
-            proj = nullptr;
-         // This project is clean; it's never been touched.  Therefore
-         // all relevant member variables are in their initial state,
-         // and it's okay to open a NEW project inside this window.
-         ( void ) ProjectManager::OpenProject( proj, fullPathStr );
       }
       else {
          // File doesn't exist - remove file from history
@@ -1487,7 +1470,8 @@ bool AudacityApp::InitPart2()
       // Auto-recovery
       //
       bool didRecoverAnything = false;
-      if (!ShowAutoRecoveryDialogIfNeeded(&project, &didRecoverAnything))
+      // This call may reassign project (passed by reference)
+      if (!ShowAutoRecoveryDialogIfNeeded(project, &didRecoverAnything))
       {
          QuitAudacity(true);
       }
@@ -1495,7 +1479,7 @@ bool AudacityApp::InitPart2()
       //
       // Remainder of command line parsing, but only if we didn't recover
       //
-      if (!didRecoverAnything)
+      if (project && !didRecoverAnything)
       {
          if (parser->Found(wxT("t")))
          {

src/AutoRecoveryDialog.cpp

@@ -36,7 +36,7 @@ enum {
 class AutoRecoveryDialog final : public wxDialogWrapper
 {
 public:
-   AutoRecoveryDialog(AudacityProject *proj);
+   explicit AutoRecoveryDialog(AudacityProject *proj);
 
    bool HasRecoverables() const;
    FilePaths GetRecoverables();
@@ -417,29 +417,27 @@ void AutoRecoveryDialog::OnListKeyDown(wxKeyEvent &evt)
 ////////////////////////////////////////////////////////////////////////////
 
 static bool RecoverAllProjects(const FilePaths &files,
-                               AudacityProject **pproj)
+                               AudacityProject *&pproj)
 {
    // Open a project window for each auto save file
    wxString filename;
+   bool result = true;
 
    for (auto &file: files)
    {
       AudacityProject *proj = nullptr;
-      if (*pproj)
+      // Reuse any existing project window, which will be the empty project
-      {
+      // created at application startup
-         // Reuse existing project window
+      std::swap(proj, pproj);
-         proj = *pproj;
-         *pproj = NULL;
-      }
 
       // Open project.
-      if (ProjectManager::OpenProject(proj, file, false) == nullptr)
+      if (ProjectManager::OpenProject(proj, file, false, true) == nullptr)
       {
-         return false;
+         result = false;
       }
    }
 
-   return true;
+   return result;
 }
 
 static void DiscardAllProjects(const FilePaths &files)
@@ -449,7 +447,7 @@ static void DiscardAllProjects(const FilePaths &files)
       ProjectFileManager::DiscardAutosave(file);
 }
 
-bool ShowAutoRecoveryDialogIfNeeded(AudacityProject **pproj, bool *didRecoverAnything)
+bool ShowAutoRecoveryDialogIfNeeded(AudacityProject *&pproj, bool *didRecoverAnything)
 {
    if (didRecoverAnything)
    {
@@ -472,7 +470,7 @@ bool ShowAutoRecoveryDialogIfNeeded(AudacityProject **pproj, bool *didRecoverAny
    // This must be done before "dlg" is declared.
    wxEventLoopBase::GetActive()->YieldFor(wxEVT_CATEGORY_UI);
 
-   AutoRecoveryDialog dialog(*pproj);
+   AutoRecoveryDialog dialog(pproj);
 
    if (dialog.HasRecoverables())
    {

src/AutoRecoveryDialog.h

@@ -26,7 +26,7 @@ class AudacityProject;
 // The didRecoverAnything param is strictly for a return value.
 // Any value passed in is ignored.
 //
-bool ShowAutoRecoveryDialogIfNeeded(AudacityProject** pproj,
+bool ShowAutoRecoveryDialogIfNeeded(AudacityProject*& pproj,
                                     bool *didRecoverAnything);
 
 #endif

src/ProjectFileManager.cpp

@@ -862,17 +862,9 @@ bool ProjectFileManager::IsAlreadyOpen(const FilePath &projPathName)
    return false;
 }
 
-// FIXME:? TRAP_ERR This should return a result that is checked.
+AudacityProject *ProjectFileManager::OpenFile( const ProjectChooserFn &chooser,
-//    See comment in AudacityApp::MRUOpen().
+   const FilePath &fileNameArg, bool addtohistory)
-void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory)
 {
-   auto &project = mProject;
-   auto &history = ProjectHistory::Get( project );
-   auto &projectFileIO = ProjectFileIO::Get( project );
-   auto &tracks = TrackList::Get( project );
-   auto &trackPanel = TrackPanel::Get( project );
-   auto &window = ProjectWindow::Get( project );
-
    // On Win32, we may be given a short (DOS-compatible) file name on rare
    // occasions (e.g. stuff like "C:\PROGRA~1\AUDACI~1\PROJEC~1.AUP"). We
    // convert these to long file name first.
@@ -882,7 +874,7 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
                                       XO("Project resides on FAT formatted drive.\n"
                                          "Copy it to another drive to open it.")))
    {
-      return;
+      return nullptr;
    }
 
    // Make sure it isn't already open.
@@ -893,7 +885,7 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
    //    This was reported in http://bugzilla.audacityteam.org/show_bug.cgi?id=137#c17,
    //    but is not really part of that bug. Anyway, prevent it!
    if (IsAlreadyOpen(fileName))
-      return;
+      return nullptr;
 
    // Data loss may occur if users mistakenly try to open ".aup3.bak" files
    // left over from an unsuccessful save or by previous versions of Audacity.
@@ -905,8 +897,8 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
 "You are trying to open an automatically created backup file.\nDoing this may result in severe data loss.\n\nPlease open the actual Audacity project file instead."),
          XO("Warning - Backup File Detected"),
          wxOK | wxCENTRE,
-         &window);
+         nullptr);
-      return;
+      return nullptr;
    }
 
    if (!::wxFileExists(fileName)) {
@@ -914,10 +906,11 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
          XO("Could not open file: %s").Format( fileName ),
          XO("Error Opening File"),
          wxOK | wxCENTRE,
-         &window);
+         nullptr);
-      return;
+      return nullptr;
    }
 
+   // Following block covers cases other than a project file:
    {
       wxFFile ff(fileName, wxT("rb"));
 
@@ -934,8 +927,8 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
             XO("Could not open file: %s").Format( fileName ),
             XO("Error opening file"),
             wxOK | wxCENTRE,
-            &window);
+            nullptr);
-         return;
+         return nullptr;
       }
 
       char buf[7];
@@ -945,40 +938,60 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
             XO("File may be invalid or corrupted: \n%s").Format( fileName ),
             XO("Error Opening File or Project"),
             wxOK | wxCENTRE,
-            &window);
+            nullptr);
-         return;
+         return nullptr;
       }
 
       if (wxStrncmp(buf, "SQLite", 6) != 0)
       {
+         // Not a database
 #ifdef EXPERIMENTAL_DRAG_DROP_PLUG_INS
          // Is it a plug-in?
-         if (PluginManager::Get().DropFile(fileName))
+         if (PluginManager::Get().DropFile(fileName)) {
-         {
             MenuCreator::RebuildAllMenuBars();
+            // Plug-in installation happened, not really opening of a file,
+            // so return null
+            return nullptr;
          }
-         else
 #endif
 #ifdef USE_MIDI
-         if (FileNames::IsMidi(fileName))
+         if (FileNames::IsMidi(fileName)) {
-         {
+            auto &project = chooser(false);
-            DoImportMIDI(project, fileName);
+            // If this succeeds, indo history is incremented, and it also does
+            // ZoomAfterImport:
+            if(DoImportMIDI(project, fileName))
+               return &project;
+            return nullptr;
          }
-         else
 #endif
-         {
+         auto &project = chooser(false);
-            Import(fileName);
+         // Undo history is incremented inside this:
+         if (Get(project).Import(fileName)) {
+            // Undo history is incremented inside this:
+            // Bug 2743: Don't zoom with lof.
+            if (!fileName.AfterLast('.').IsSameAs(wxT("lof"), false))
+               ProjectWindow::Get(project).ZoomAfterImport(nullptr);
+            return &project;
          }
-         // Bug 2743: Don't zoom with lof.
+         return nullptr;
-         if (!fileName.AfterLast('.').IsSameAs(wxT("lof"), false))
-            window.ZoomAfterImport(nullptr);
-
-         return;
       }
    }
 
+   auto &project = chooser(true);
+   return Get(project).OpenProjectFile(fileName, addtohistory);
+}
+
+AudacityProject *ProjectFileManager::OpenProjectFile(
+   const FilePath &fileName, bool addtohistory)
+{
+   auto &project = mProject;
+   auto &history = ProjectHistory::Get( project );
+   auto &tracks = TrackList::Get( project );
+   auto &trackPanel = TrackPanel::Get( project );
+   auto &projectFileIO = ProjectFileIO::Get( project );
+   auto &window = ProjectWindow::Get( project );
+
    auto results = ReadProjectFile( fileName );
- 
    const bool bParseSuccess = results.parseSuccess;
    const auto &errorStr = results.errorString;
    const bool err = results.trackError;
@@ -1020,6 +1033,7 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
          // PushState calls AutoSave(), so no longer need to do so here.
          history.PushState(XO("Project was recovered"), XO("Recover"));
       }
+      return &project;
    }
    else {
       // Vaughan, 2011-10-30:
@@ -1044,6 +1058,8 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory
          XO("Error Opening Project"),
          errorStr,
          results.helpUrl);
+
+      return nullptr;
    }
 }
 
@@ -1292,7 +1308,7 @@ bool ProjectFileManager::Import(
 
       history.PushState(XO("Imported '%s'").Format( fileName ), XO("Import"));
 
-      return false;
+      return true;
    }
 
    // PRL: Undo history is incremented inside this:

src/ProjectFileManager.h

@@ -11,6 +11,7 @@ Paul Licameli split from AudacityProject.h
 #ifndef __AUDACITY_PROJECT_FILE_MANAGER__
 #define __AUDACITY_PROJECT_FILE_MANAGER__
 
+#include <functional>
 #include <memory>
 #include <vector>
 
@@ -43,16 +44,6 @@ public:
    ProjectFileManager &operator=( const ProjectFileManager & ) PROHIBITED;
    ~ProjectFileManager();
 
-   struct ReadProjectResults
-   {
-      bool parseSuccess;
-      bool trackError;
-      const TranslatableString errorString;
-      wxString helpUrl;
-   };
-   ReadProjectResults ReadProjectFile(
-      const FilePath &fileName, bool discardAutosave = false );
-
    bool OpenProject();
    void CloseProject();
    bool OpenNewProject();
@@ -91,7 +82,18 @@ public:
 
    static bool IsAlreadyOpen(const FilePath &projPathName);
 
-   void OpenFile(const FilePath &fileName, bool addtohistory = true);
+   //! A function that returns a project to use for opening a file; argument is true if opening a project file
+   using ProjectChooserFn = std::function<AudacityProject&(bool)>;
+
+   /*!
+    Opens files of many kinds.  In case of import (sound, MIDI, or .aup), the undo history is pushed.
+    @param chooser told whether opening a project file; decides which project to open into
+    @param fileName the name and contents are examined to decide a type and open appropriately
+    @param addtohistory whether to add .aup3 files to the MRU list (but always done for imports)
+    @return if something was successfully opened, the project containing it; else null
+    */
+   static AudacityProject *OpenFile( const ProjectChooserFn &chooser,
+      const FilePath &fileName, bool addtohistory = true);
 
    bool Import(const FilePath &fileName,
                bool addToHistory = true);
@@ -105,6 +107,24 @@ public:
    void SetMenuClose(bool value) { mMenuClose = value; }
 
 private:
+   /*!
+    @param fileName a path assumed to exist and contain an .aup3 project
+    @param addtohistory whether to add the file to the MRU list
+    @return if something was successfully opened, the project containing it; else null
+    */
+   AudacityProject *OpenProjectFile(
+      const FilePath &fileName, bool addtohistory);
+
+   struct ReadProjectResults
+   {
+      bool parseSuccess;
+      bool trackError;
+      const TranslatableString errorString;
+      wxString helpUrl;
+   };
+   ReadProjectResults ReadProjectFile(
+      const FilePath &fileName, bool discardAutosave = false );
+
    bool DoSave(const FilePath & fileName, bool fromSaveAs);
 
    AudacityProject &mProject;

src/ProjectManager.cpp

@@ -838,17 +838,18 @@ void ProjectManager::OnCloseWindow(wxCloseEvent & event)
 
 // PRL: I preserve this handler function for an event that was never sent, but
 // I don't know the intention.
-
 void ProjectManager::OnOpenAudioFile(wxCommandEvent & event)
 {
-   auto &project = mProject;
-   auto &window = GetProjectFrame( project );
    const wxString &cmd = event.GetString();
-
+   if (!cmd.empty()) {
-   if (!cmd.empty())
+      ProjectChooser chooser{ &mProject, true };
-      ProjectFileManager::Get( mProject ).OpenFile(cmd);
+      if (auto project = ProjectFileManager::OpenFile(
-
+            std::ref(chooser), cmd)) {
-   window.RequestUserAttention();
+         auto &window = GetProjectFrame( *project );
+         window.RequestUserAttention();
+         chooser.Commit();
+      }
+   }
 }
 
 // static method, can be called outside of a project
@@ -868,68 +869,93 @@ void ProjectManager::OpenFiles(AudacityProject *proj)
       Importer::SetLastOpenType({});
    } );
 
-   for (size_t ff = 0; ff < selectedFiles.size(); ff++) {
+   for (const auto &fileName : selectedFiles) {
-      const wxString &fileName = selectedFiles[ff];
-
       // Make sure it isn't already open.
       if (ProjectFileManager::IsAlreadyOpen(fileName))
          continue; // Skip ones that are already open.
 
-      // DMM: If the project is dirty, that means it's been touched at
+      proj = OpenProject( proj, fileName,
-      // all, and it's not safe to open a NEW project directly in its
+         true /* addtohistory */, false /* reuseNonemptyProject */ );
-      // place.  Only if the project is brand-NEW clean and the user
-      // hasn't done any action at all is it safe for Open to take place
-      // inside the current project.
-      //
-      // If you try to Open a NEW project inside the current window when
-      // there are no tracks, but there's an Undo history, etc, then
-      // bad things can happen, including data files moving to the NEW
-      // project directory, etc.
-      if ( proj && (
-         ProjectHistory::Get( *proj ).GetDirty() ||
-         !TrackList::Get( *proj ).empty()
-      ) )
-         proj = nullptr;
-
-      // This project is clean; it's never been touched.  Therefore
-      // all relevant member variables are in their initial state,
-      // and it's okay to open a NEW project inside this window.
-      proj = OpenProject( proj, fileName );
    }
 }
 
-AudacityProject *ProjectManager::OpenProject(
+bool ProjectManager::SafeToOpenProjectInto(AudacityProject &proj)
-   AudacityProject *pProject, const FilePath &fileNameArg, bool addtohistory)
 {
-   bool success = false;
+   // DMM: If the project is dirty, that means it's been touched at
-   AudacityProject *pNewProject = nullptr;
+   // all, and it's not safe to open a fresh project directly in its
-   if ( ! pProject )
+   // place.  Only if the project is brandnew clean and the user
-      pProject = pNewProject = New();
+   // hasn't done any action at all is it safe for Open to take place
-   auto cleanup = finally( [&] {
+   // inside the current project.
-      if ( pNewProject )
+   //
-         GetProjectFrame( *pNewProject ).Close(true);
+   // If you try to Open a fresh project inside the current window when
-      else if ( !success )
+   // there are no tracks, but there's an Undo history, etc, then
+   // bad things can happen, including orphan blocks, or tracks
+   // referring to non-existent blocks
+   if (
+      ProjectHistory::Get( proj ).GetDirty() ||
+      !TrackList::Get( proj ).empty()
+   )
+      return false;
+   // This project is clean; it's never been touched.  Therefore
+   // all relevant member variables are in their initial state,
+   // and it's okay to open a NEW project inside its window.
+   return true;
+}
+
+ProjectManager::ProjectChooser::~ProjectChooser()
+{
+   if (mpUsedProject) {
+      if (mpUsedProject == mpGivenProject) {
          // Ensure that it happens here: don't wait for the application level
          // exception handler, because the exception may be intercepted
-         ProjectHistory::Get(*pProject).RollbackState();
+         ProjectHistory::Get(*mpGivenProject).RollbackState();
-      // Any exception now continues propagating
+         // Any exception now continues propagating
-   } );
+      }
-   ProjectFileManager::Get( *pProject ).OpenFile( fileNameArg, addtohistory );
+      else
-
+         GetProjectFrame( *mpUsedProject ).Close(true);
-   // The above didn't throw, so change what finally will do
-   success = true;
-   pNewProject = nullptr;
-
-   auto &projectFileIO = ProjectFileIO::Get( *pProject );
-   if( projectFileIO.IsRecovered() ) {
-      auto &window = ProjectWindow::Get( *pProject );
-      window.Zoom( window.GetZoomOfToFit() );
-      // "Project was recovered" replaces "Create new project" in Undo History.
-      auto &undoManager = UndoManager::Get( *pProject );
-      undoManager.RemoveStates(0, 1);
    }
+}
 
-   return pProject;
+AudacityProject &
+ProjectManager::ProjectChooser::operator() ( bool openingProjectFile )
+{
+   if (mpGivenProject) {
+      // Always check before opening a project file (for safety);
+      // May check even when opening other files
+      // (to preserve old behavior; as with the File > Open command specifying
+      // multiple files of whatever types, so that each gets its own window)
+      bool checkReuse = (openingProjectFile || !mReuseNonemptyProject);
+      if (!checkReuse || SafeToOpenProjectInto(*mpGivenProject))
+         return *(mpUsedProject = mpGivenProject);
+   }
+   return *(mpUsedProject = New());
+}
+
+void ProjectManager::ProjectChooser::Commit()
+{
+   mpUsedProject = nullptr;
+}
+
+AudacityProject *ProjectManager::OpenProject(
+   AudacityProject *pGivenProject, const FilePath &fileNameArg,
+   bool addtohistory, bool reuseNonemptyProject)
+{
+   ProjectManager::ProjectChooser chooser{ pGivenProject, reuseNonemptyProject };
+   if (auto pProject = ProjectFileManager::OpenFile(
+      std::ref(chooser), fileNameArg, addtohistory )) {
+      chooser.Commit();
+
+      auto &projectFileIO = ProjectFileIO::Get( *pProject );
+      if( projectFileIO.IsRecovered() ) {
+         auto &window = ProjectWindow::Get( *pProject );
+         window.Zoom( window.GetZoomOfToFit() );
+         // "Project was recovered" replaces "Create new project" in Undo History.
+         auto &undoManager = UndoManager::Get( *pProject );
+         undoManager.RemoveStates(0, 1);
+      }
+      return pProject;
+   }
+   return nullptr;
 }
 
 // This is done to empty out the tracks, but without creating a new project.

src/ProjectManager.h

@@ -45,12 +45,56 @@ public:
    // reason remains in this class, not in ProjectFileManager
    static void OpenFiles(AudacityProject *proj);
 
-   // Return the given project if that is not NULL, else create a project.
+   //! False when it is unsafe to overwrite proj with contents of an .aup3 file
-   // Then open the given project path.
+   static bool SafeToOpenProjectInto(AudacityProject &proj);
-   // But if an exception escapes this function, create no NEW project.
+
+   //! Callable object that supplies the `chooser` argument of ProjectFileManager::OpenFile
+   /*!
+    Its operator(), called lower down in ProjectFileManager, decides which project to put new file data into,
+    using file type information deduced there.  It may have the side effect of creating a project.
+
+    At the higher level where it is constructed, it provides conditional RAII.
+    One indicates there that the file opening succeeded by calling Commit().  But if that is never
+    called, creation of projects, or changes to a preexisting project, are undone.
+    */
+   class ProjectChooser {
+   public:
+       /*!
+       @param pProject if not null, an existing project to reuse if possible
+       @param reuseNonemptyProject if true, may reuse the given project when nonempty,
+       but only if importing (not for a project file)
+       */
+      ProjectChooser( AudacityProject *pProject, bool reuseNonemptyProject )
+         : mpGivenProject{ pProject }
+         , mReuseNonemptyProject{ reuseNonemptyProject }
+      {}
+      //! Don't copy.  Use std::ref to pass it to ProjectFileManager
+      ProjectChooser( const ProjectChooser& ) PROHIBITED;
+      //! Destroy any fresh project, or rollback the existing project, unless committed
+      ~ProjectChooser();
+      //! May create a fresh project
+      AudacityProject &operator() ( bool openingProjectFile );
+      //! Commit the creation of any fresh project or changes to the existing project
+      void Commit();
+
+   private:
+      AudacityProject *mpGivenProject;
+      AudacityProject *mpUsedProject = nullptr;
+      bool mReuseNonemptyProject;
+   };
+
+   //! Open a file into an AudacityProject, returning the project, or nullptr for failure
+   /*!
+    If an exception escapes this function, no projects are created.
+    @param pGivenProject if not null, a project that may be reused
+    @param fileNameArg path to the file to open; not always an Audacity project file, may be an import
+    @param addtohistory whether to add .aup3 files to the MRU list (but always done for imports)
+    @param reuseNonemptyProject if true, may reuse the given project when nonempty,
+       but only if importing (not for a project file)
+    */
    static AudacityProject *OpenProject(
-      AudacityProject *pProject,
+      AudacityProject *pGivenProject,
-      const FilePath &fileNameArg, bool addtohistory = true);
+      const FilePath &fileNameArg, bool addtohistory, bool reuseNonemptyProject);
 
    void ResetProjectToEmpty();
 

src/commands/OpenSaveCommands.cpp

@@ -59,13 +59,16 @@ bool OpenProjectCommand::Apply(const CommandContext & context){
    auto oldFileName = projectFileIO.GetFileName();
    if(mFileName.empty())
    {
+      // This path queries the user for files to open
       auto project = &context.project;
       ProjectManager::OpenFiles(project);
    }
    else
    {
-      ProjectFileManager::Get( context.project )
+      ProjectManager::ProjectChooser chooser{ &context.project, true };
-         .OpenFile(mFileName, mbAddToHistory);
+      if(ProjectFileManager::OpenFile(
+         std::ref(chooser), mFileName, mbAddToHistory))
+         chooser.Commit();
    }
    const auto &newFileName = projectFileIO.GetFileName();
 

src/import/ImportLOF.cpp

@@ -410,7 +410,8 @@ void LOFImportFileHandle::lofOpenFiles(wxString* ln)
           * audio file. TODO: Some sort of message here? */
 
 #endif // USE_MIDI
-         mProject = ProjectManager::OpenProject( mProject, targetfile );
+         mProject = ProjectManager::OpenProject( mProject, targetfile,
+            true /* addtohistory */, true /* reuseNonemptyProject */ );
 
       // Set tok to right after filename
       temptok2.SetString(targettoken);