Bug 137 (P3) - Unreliable project re-opening with orphaned and missing blockfile errors

At bottom of http://bugzilla.audacityteam.org/show_bug.cgi?id=137#c17: 

> I noticed that although when we open the same project a second time with File >
> Open or File > Recent Files we show an error "is already open in another
> window", there is no block on executing the .aup from your file manager and
> opening as many copies of the project as you like. I had not done that when
> Audacity moved the files around in error the first time, but can we block this
> anyway?

This is a different bug from Bug 137, repeatable, and I think lower priority. But I went ahead and fixed it with this commit.

Also fixed previously unnoted bug where AudacityApp::MRUOpen() returned true when it actually failed to open the file. Also removed AudacityApp::OnMRUProject() cruft.

Unfortunately, it still creates a new, empty project window on Win Explorer open whereas Open and Recent Files commands do not. I think that constitutes a new, separate P5 bug. 

Overall, this is another aspect of what I was talking about in http://bugzilla.audacityteam.org/show_bug.cgi?id=322#c26. Opening files vs projects got conflated for convenience, but this code is hacked in some regards, rather than being a good design, and that's why this type of bug shows up.

src/AudacityApp.cpp

@@ -701,7 +701,7 @@ void AudacityApp::OnMacOpenFile(wxCommandEvent & event)
    while (ofqueue.GetCount()) {
       wxString name(ofqueue[0]);
       ofqueue.RemoveAt(0);
-      MRUOpen(name);
+      MRUOpen(name); // FIX-ME: Check the return result?
    }
 }
 #endif //__WXMAC__
@@ -735,7 +735,7 @@ BEGIN_EVENT_TABLE(AudacityApp, wxApp)
    EVT_APP_COMMAND(wxID_ANY, AudacityApp::OnReceiveCommand)
 END_EVENT_TABLE()
 
-// Backend for OnMRUFile and OnMRUProject
+// backend for OnMRUFile 
 bool AudacityApp::MRUOpen(wxString fullPathStr) {
    // Most of the checks below are copied from AudacityProject::OpenFiles.
    // - some rationalisation might be possible.
@@ -749,18 +749,12 @@ bool AudacityApp::MRUOpen(wxString fullPathStr) {
       {
          gPrefs->Write(wxT("/DefaultOpenPath"), wxPathOnly(fullPathStr));
          
-         // Make sure it isn't already open
+         // Make sure it isn't already open.
-         wxFileName newFileName(fullPathStr);
+         // Test here even though AudacityProject::OpenFile() also now checks, because 
-         size_t numProjects = gAudacityProjects.Count();
+         // that method does not return the bad result. 
-         for (size_t i = 0; i < numProjects; i++) {
+         // That itself may be a FIX-ME.
-            if (newFileName.SameAs(gAudacityProjects[i]->GetFileName())) {
+         if (AudacityProject::IsAlreadyOpen(fullPathStr))
-               wxMessageBox(wxString::Format(_("%s is already open in another window."),
+            return false;
-                  newFileName.GetName().c_str()),
-                  _("Error opening project"),
-                  wxOK | wxCENTRE);
-               return(true);
-            }
-         }
          
          // DMM: If the project is dirty, that means it's been touched at
          // all, and it's not safe to open a new project directly in its
@@ -795,40 +789,22 @@ void AudacityApp::OnMRUClear(wxCommandEvent& event)
    mRecentFiles->Clear();
 }
 
+//vvv Basically, anything from Recent Files is treated as a .aup, until proven otherwise, 
+// then it tries to Import(). Very questionable handling, imo. 
+// Better, for example, to check the file type early on.
 void AudacityApp::OnMRUFile(wxCommandEvent& event) {
    int n = event.GetId() - ID_RECENT_FIRST;
    wxString fullPathStr = mRecentFiles->GetHistoryFile(n);
 
-   bool opened = MRUOpen(fullPathStr);
+   // Try to open only if not already open. 
-   if(!opened) {
+   // Test IsAlreadyOpen() here even though AudacityProject::MRUOpen() also now checks, 
+   // because we don't want to RemoveFileFromHistory() just because it already exists,
+   // and AudacityApp::OnMacOpenFile() calls MRUOpen() directly.
+   // that method does not return the bad result. 
+   if (!AudacityProject::IsAlreadyOpen(fullPathStr) && !MRUOpen(fullPathStr))
       mRecentFiles->RemoveFileFromHistory(n);
-   }
 }
 
-#if 0
-//FIX-ME: Was this OnMRUProject lost in an edit??  Should we have it back?
-//vvv I think it was removed on purpose, but I don't know why it's still here. 
-// Basically, anything from Recent Files is treated as a .aup, until proven otherwise, 
-// then it tries to Import(). Very questionable handling, imo. 
-// Better, for example, to check the file type early on.
-// 
-void AudacityApp::OnMRUProject(wxCommandEvent& event) {
-   AudacityProject *proj = GetActiveProject();
-
-   int n = event.GetId() - 6050;//FIX-ME: Use correct ID name.
-   wxString fullPathStr = proj->GetRecentProjects()->GetHistoryFile(n);
-
-   bool opened = MRUOpen(fullPathStr);
-   if(!opened) {
-      proj->GetRecentProjects()->RemoveFileFromHistory(n);
-      gPrefs->SetPath("/RecentProjects");
-      proj->GetRecentProjects()->Save(*gPrefs);
-      gPrefs->SetPath("..");
-   }
-}
-#endif
-
-
 void AudacityApp::InitLang( const wxString & lang )
 {
    if( mLocale )

src/Project.cpp

@@ -2160,6 +2160,26 @@ wxArrayString AudacityProject::ShowOpenDialog(wxString extraformat, wxString ext
    return selected;
 }
 
+// static method, can be called outside of a project
+bool AudacityProject::IsAlreadyOpen(const wxString projPathName)
+{
+   wxFileName newProjPathName(projPathName);
+   size_t numProjects = gAudacityProjects.Count();
+   for (size_t i = 0; i < numProjects; i++) 
+   {
+      if (newProjPathName.SameAs(gAudacityProjects[i]->mFileName)) 
+      {
+         wxString errMsg = 
+            wxString::Format(_("%s is already open in another window."),
+                              newProjPathName.GetName().c_str());
+         wxLogError(errMsg);
+         wxMessageBox(errMsg, _("Error opening project"), wxOK | wxCENTRE);
+         return true;
+      }
+   }
+   return false;
+}
+
 // static method, can be called outside of a project
 void AudacityProject::OpenFiles(AudacityProject *proj)
 {
@@ -2182,25 +2202,10 @@ void AudacityProject::OpenFiles(AudacityProject *proj)
 
    for (size_t ff = 0; ff < selectedFiles.GetCount(); ff++) {
       wxString fileName = selectedFiles[ff];
-      wxFileName newFileName(fileName);
-      bool skip = false;
 
-      // Make sure it isn't already open
+      // Make sure it isn't already open.
-      size_t numProjects = gAudacityProjects.Count();
+      if (AudacityProject::IsAlreadyOpen(fileName))
-      for (size_t i = 0; i < numProjects; i++) {
+         continue; // Skip ones that are already open.
-         if (newFileName.SameAs(gAudacityProjects[i]->mFileName)) {
-            wxMessageBox(wxString::Format(_("%s is already open in another window."),
-                                          newFileName.GetName().c_str()),
-                         _("Error opening project"),
-                         wxOK | wxCENTRE);
-            skip = true;
-            break;
-         }
-      }
-
-      if (skip) {
-         continue;
-      }
 
       gPrefs->Write(wxT("/DefaultOpenPath"), wxPathOnly(fileName));
       
@@ -2250,7 +2255,8 @@ bool AudacityProject::WarnOfLegacyFile( )
 }
 
 
-
+// FIX-ME? This should return a result that is checked. 
+//    See comment in AudacityApp::MRUOpen().
 void AudacityProject::OpenFile(wxString fileName, bool addtohistory)
 {
    // On Win32, we may be given a short (DOS-compatible) file name on rare
@@ -2259,6 +2265,17 @@ void AudacityProject::OpenFile(wxString fileName, bool addtohistory)
    fileName = PlatformCompatibility::ConvertSlashInFileName(
       PlatformCompatibility::GetLongFileName(fileName));
 
+   // Make sure it isn't already open.
+   // Vaughan, 2011-03-25: This was done previously in AudacityProject::OpenFiles() 
+   //    and AudacityApp::MRUOpen(), but if you open an aup file by double-clicking it 
+   //    from, e.g., Win Explorer, it would bypass those, get to here with no check, 
+   //    then open a new project from the same data with no warning. 
+   //    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 (AudacityProject::IsAlreadyOpen(fileName))
+      return;
+
+
    // Data loss may occur if users mistakenly try to open ".aup.bak" files
    // left over from an unsuccessful save or by previous versions of Audacity.
    // So we always refuse to open such files.

src/Project.h

@@ -189,6 +189,7 @@ class AUDACITY_DLL_API AudacityProject:  public wxFrame,
     */
    static wxArrayString ShowOpenDialog(wxString extraformat = wxEmptyString,
          wxString extrafilter = wxEmptyString);
+   static bool IsAlreadyOpen(const wxString projPathName);
    static void OpenFiles(AudacityProject *proj);
    void OpenFile(wxString fileName, bool addtohistory = true);
    bool WarnOfLegacyFile( );