From: Ben Soares <b.soares@dundee.ac.uk>
Date: Mon, 11 Sep 2023 23:16:44 +0000 (+0100)
Subject: JAL-4284 JAL-4285 Deal with output files/images consistently with --overwrite check... 
X-Git-Tag: Release_2_11_4_0~174^2~32
X-Git-Url: http://source.jalview.org/gitweb/?a=commitdiff_plain;h=2ee2e17dc404c02d94ae4271130ea8000aee9724;p=jalview.git

JAL-4284 JAL-4285 Deal with output files/images consistently with --overwrite check.  Gather batch process errors together to output at the end of processing.
---

diff --git a/src/jalview/bin/Commands.java b/src/jalview/bin/Commands.java
index 8164182..cd72e60 100644
--- a/src/jalview/bin/Commands.java
+++ b/src/jalview/bin/Commands.java
@@ -72,6 +72,8 @@ public class Commands
 
   private boolean argsWereParsed = false;
 
+  private List<String> errors = new ArrayList<>();
+
   public Commands(ArgParser argparser, boolean headless)
   {
     this(Desktop.instance, argparser, headless);
@@ -138,9 +140,28 @@ public class Commands
       }
 
     }
+
+    // report errors
+    StringBuilder sb = new StringBuilder();
+    for (String error : errors)
+    {
+      sb.append("- " + error);
+      sb.append("\n");
+    }
+    if (Platform.isHeadless())
+    {
+      Console.debug("All errors from command line argument commands:\n"
+              + sb.toString());
+    }
+    else
+    {
+      // scrollable dialog box
+
+    }
+
     if (argParser.getBoolean(Arg.QUIT))
     {
-      Jalview.getInstance().quit();
+      Jalview.exit("Exiting due to " + Arg.QUIT.argString(), 0);
       return true;
     }
     // carry on with jalview.bin.Jalview
@@ -163,7 +184,11 @@ public class Commands
     boolean theseArgsWereParsed = false;
     ArgValuesMap avm = argParser.getLinkedArgs(id);
     if (avm == null)
+    {
       return true;
+    }
+
+    boolean isError = false;
 
     // set wrap scope here so it can be applied after structures are opened
     boolean wrap = false;
@@ -216,7 +241,8 @@ public class Commands
           {
             if (!(new File(openFile)).exists())
             {
-              Console.warn("Can't find file '" + openFile + "'");
+              addError("Can't find file '" + openFile + "'");
+              isError = true;
               continue;
             }
           }
@@ -231,7 +257,8 @@ public class Commands
           format = new IdentifyFile().identify(openFile, protocol);
         } catch (FileFormatException e1)
         {
-          Console.error("Unknown file format for '" + openFile + "'");
+          addError("Unknown file format for '" + openFile + "'");
+          isError = true;
           continue;
         }
 
@@ -249,21 +276,25 @@ public class Commands
           Console.debug(
                   "Opening '" + openFile + "' in new alignment frame");
           FileLoader fileLoader = new FileLoader(!headless);
-          boolean xception=false;
-          try {
+          boolean xception = false;
+          try
+          {
             af = fileLoader.LoadFileWaitTillLoaded(openFile, protocol,
                     format);
           } catch (Throwable thr)
           {
-            xception=true;
-            Console.error("Couldn't open '"+openFile+"' as "+format+" "+thr.getLocalizedMessage()+ " (Enable debug for full stack trace)");
-            Console.debug("Exception when opening '"+openFile+"'",thr);
-          }
-          finally
+            xception = true;
+            addError("Couldn't open '" + openFile + "' as " + format + " "
+                    + thr.getLocalizedMessage()
+                    + " (Enable debug for full stack trace)");
+            isError = true;
+            Console.debug("Exception when opening '" + openFile + "'", thr);
+          } finally
           {
-            if (af==null && !xception)
+            if (af == null && !xception)
             {
-              Console.info("Ignoring '"+openFile+"' - no alignment data found.");
+              addInfo("Ignoring '" + openFile
+                      + "' - no alignment data found.");
               continue;
             }
           }
@@ -279,8 +310,7 @@ public class Commands
 
             if (cs == null && !"None".equals(colour))
             {
-              Console.warn(
-                      "Couldn't parse '" + colour + "' as a colourscheme.");
+              addWarn("Couldn't parse '" + colour + "' as a colourscheme.");
             }
             else
             {
@@ -344,7 +374,8 @@ public class Commands
                       "examples/testdata/uniref50_test_tree", treefile);
             } catch (IOException e)
             {
-              Console.warn("Couldn't add tree " + treefile, e);
+              addError("Couldn't add tree " + treefile, e);
+              isError = true;
             }
           }
 
@@ -414,7 +445,7 @@ public class Commands
         }
         else
         {
-          Console.warn("No more files to open");
+          Console.info("No more files to open");
         }
       }
       if (progressBarSet && desktop != null)
@@ -444,10 +475,8 @@ public class Commands
 
           if (seq == null)
           {
-            Console.warn("Could not find sequence for argument "
+            addWarn("Could not find sequence for argument "
                     + Arg.STRUCTURE.argString() + "=" + val);
-            // you probably want to continue here, not break
-            // break;
             continue;
           }
           File structureFile = null;
@@ -480,14 +509,14 @@ public class Commands
 
           if (structureFile == null)
           {
-            Console.warn("Not provided structure file with '" + val + "'");
+            addWarn("Not provided structure file with '" + val + "'");
             continue;
           }
 
           if (!structureFile.exists())
           {
-            Console.warn("Structure file '"
-                    + structureFile.getAbsoluteFile() + "' not found.");
+            addWarn("Structure file '" + structureFile.getAbsoluteFile()
+                    + "' not found.");
             continue;
           }
 
@@ -519,7 +548,7 @@ public class Commands
             } catch (IOException e)
             {
               paeFilepath = paeFile.getAbsolutePath();
-              Console.warn("Problem with the PAE file path: '"
+              addWarn("Problem with the PAE file path: '"
                       + paeFile.getPath() + "'");
             }
           }
@@ -562,7 +591,7 @@ public class Commands
                 if (it.hasNext())
                   sb.append(", ");
               }
-              Console.warn(sb.toString());
+              addWarn(sb.toString());
             }
           }
 
@@ -579,7 +608,8 @@ public class Commands
 
           if (sv == null)
           {
-            Console.error("Failed to import and open structure view.");
+            addError("Failed to import and open structure view for file '"
+                    + structureFile + "'.");
             continue;
           }
           try
@@ -597,13 +627,15 @@ public class Commands
             }
             if (tries == 0 && sv.isBusy())
             {
-              Console.warn(
-                      "Gave up waiting for structure viewer to load. Something may have gone wrong.");
+              addWarn("Gave up waiting for structure viewer to load file '"
+                      + structureFile
+                      + "'. Something may have gone wrong.");
             }
           } catch (Exception x)
           {
-            Console.warn("Exception whilst waiting for structure viewer "
+            addError("Exception whilst waiting for structure viewer "
                     + structureFilepath, x);
+            isError = true;
           }
 
           // add StructureViewer to svMap list
@@ -652,7 +684,7 @@ public class Commands
                       typeS.toUpperCase(Locale.ROOT));
             } catch (IllegalArgumentException e)
             {
-              Console.warn("Do not know image format '" + typeS
+              addWarn("Do not know image format '" + typeS
                       + "', using PNG");
               imageType = TYPE.PNG;
             }
@@ -669,6 +701,18 @@ public class Commands
                 AppJmol jmol = (AppJmol) sview;
                 try
                 {
+                  whatNext wn = this.checksBeforeWritingToFile(avm, subVals,
+                          false, structureImageFilename, "structure image");
+                  if (wn == whatNext.ERROR)
+                  {
+                    isError = true;
+                    continue;
+                  }
+                  else if (wn == whatNext.CONTINUE)
+                  {
+                    continue;
+                  }
+
                   Console.debug("Rendering image to " + structureImageFile);
                   jmol.makePDBImage(structureImageFile, imageType, renderer,
                           userBis);
@@ -677,16 +721,18 @@ public class Commands
 
                 } catch (ImageOutputException ioexc)
                 {
-                  Console.warn("Unexpected error whilst exporting image to "
+                  addError("Unexpected error whilst exporting image to "
                           + structureImageFile, ioexc);
+                  isError = true;
+                  continue;
                 }
 
               }
               break;
             default:
-              Console.warn("Cannot export image for structure viewer "
+              addWarn("Cannot export image for structure viewer "
                       + sv.getViewerType() + " yet");
-              break;
+              continue;
             }
           }
         }
@@ -719,7 +765,7 @@ public class Commands
     }
     */
 
-    return theseArgsWereParsed;
+    return theseArgsWereParsed && !isError;
   }
 
   protected void processGroovyScript(String id)
@@ -729,7 +775,7 @@ public class Commands
 
     if (af == null)
     {
-      Console.warn("Did not have an alignment window for id=" + id);
+      addWarn("Did not have an alignment window for id=" + id);
       return;
     }
 
@@ -753,10 +799,11 @@ public class Commands
 
     if (af == null)
     {
-      Console.warn("Did not have an alignment window for id=" + id);
+      addWarn("Did not have an alignment window for id=" + id);
       return false;
     }
 
+    boolean isError = false;
     if (avm.containsArg(Arg.IMAGE))
     {
       for (ArgValue av : avm.getArgValueList(Arg.IMAGE))
@@ -797,6 +844,19 @@ public class Commands
         Cache.setProperty("EXPORT_EMBBED_BIOJSON", "false");
 
         Console.info("Writing " + file);
+
+        whatNext wn = this.checksBeforeWritingToFile(avm, subVal, false,
+                fileName, "image");
+        if (wn == whatNext.ERROR)
+        {
+          isError = true;
+          continue;
+        }
+        else if (wn == whatNext.CONTINUE)
+        {
+          continue;
+        }
+
         try
         {
           switch (type)
@@ -844,17 +904,19 @@ public class Commands
             break;
 
           default:
-            Console.warn(Arg.IMAGE.argString() + " type '" + type
+            addWarn(Arg.IMAGE.argString() + " type '" + type
                     + "' not known. Ignoring");
             break;
           }
         } catch (Exception ioex)
         {
-          Console.warn("Unexpected error during export", ioex);
+          addError("Unexpected error during export to '" + fileName + "'",
+                  ioex);
+          isError = true;
         }
       }
     }
-    return true;
+    return !isError;
   }
 
   protected boolean processOutput(String id)
@@ -864,10 +926,12 @@ public class Commands
 
     if (af == null)
     {
-      Console.warn("Did not have an alignment window for id=" + id);
+      addWarn("Did not have an alignment window for id=" + id);
       return false;
     }
 
+    boolean isError = false;
+
     if (avm.containsArg(Arg.OUTPUT))
     {
       for (ArgValue av : avm.getArgValueList(Arg.OUTPUT))
@@ -877,24 +941,6 @@ public class Commands
         String fileName = subVals.getContent();
         boolean stdout = ArgParser.STDOUTFILENAME.equals(fileName);
         File file = new File(fileName);
-        boolean overwrite = ArgParser.getFromSubValArgOrPref(avm,
-                Arg.OVERWRITE, subVals, null, "OVERWRITE_OUTPUT", false);
-        // backups. Use the Arg.BACKUPS or subval "backups" setting first,
-        // otherwise if headless assume false, if not headless use the user
-        // preference with default true.
-        boolean backups = ArgParser.getFromSubValArgOrPref(avm, Arg.BACKUPS,
-                subVals, null,
-                Platform.isHeadless() ? null : BackupFiles.ENABLED,
-                !Platform.isHeadless());
-
-        // if backups is not true then --overwrite must be specified
-        if (file.exists() && !(overwrite || backups || stdout))
-        {
-          Console.error("Won't overwrite file '" + fileName + "' without "
-                  + Arg.OVERWRITE.argString() + " or "
-                  + Arg.BACKUPS.argString() + " set");
-          return false;
-        }
 
         String name = af.getName();
         String format = ArgParser.getValueFromSubValOrArg(avm, av,
@@ -944,41 +990,49 @@ public class Commands
               validSB.append(")");
             }
 
-            Jalview.exit("No valid format specified for "
+            addError("No valid format specified for "
                     + Arg.OUTPUT.argString() + ". Valid formats are "
-                    + validSB.toString() + ".", 1);
-            // this return really shouldn't happen
-            return false;
+                    + validSB.toString() + ".");
+            continue;
           }
         }
 
-        String savedBackupsPreference = Cache
-                .getDefault(BackupFiles.ENABLED, null);
-        Console.debug("Setting backups to " + backups);
-        Cache.applicationProperties.put(BackupFiles.ENABLED,
-                Boolean.toString(backups));
+        whatNext wn = this.checksBeforeWritingToFile(avm, subVals, true,
+                fileName, ff.getName());
+        if (wn == whatNext.ERROR)
+        {
+          isError = true;
+          continue;
+        }
+        else if (wn == whatNext.CONTINUE)
+        {
+          continue;
+        }
+
+        boolean backups = ArgParser.getFromSubValArgOrPref(avm, Arg.BACKUPS,
+                subVals, null,
+                Platform.isHeadless() ? null : BackupFiles.ENABLED,
+                !Platform.isHeadless());
 
         Console.info("Writing " + fileName);
 
-        af.saveAlignment(fileName, ff, stdout);
-        Console.debug("Returning backups to " + savedBackupsPreference);
-        if (savedBackupsPreference != null)
-          Cache.applicationProperties.put(BackupFiles.ENABLED,
-                  savedBackupsPreference);
+        af.saveAlignment(fileName, ff, stdout, backups);
         if (af.isSaveAlignmentSuccessful())
         {
           Console.debug("Written alignment '" + name + "' in "
-                  + ff.getName() + " format to " + file);
+                  + ff.getName() + " format to '" + file + "'");
         }
         else
         {
-          Console.warn("Error writing file " + file + " in " + ff.getName()
+          addError("Error writing file '" + file + "' in " + ff.getName()
                   + " format!");
+          isError = true;
+          continue;
         }
 
       }
     }
-    return true;
+    return !isError;
   }
 
   private SequenceI getSpecifiedSequence(AlignFrame af, ArgValuesMap avm,
@@ -1044,4 +1098,78 @@ public class Commands
     }
     return svs;
   }
+
+  private void addInfo(String errorMessage)
+  {
+    Console.info(errorMessage);
+    errors.add(errorMessage);
+  }
+
+  private void addWarn(String errorMessage)
+  {
+    Console.warn(errorMessage);
+    errors.add(errorMessage);
+  }
+
+  private void addError(String errorMessage)
+  {
+    addError(errorMessage, null);
+  }
+
+  private void addError(String errorMessage, Exception e)
+  {
+    Console.error(errorMessage, e);
+    errors.add(errorMessage);
+  }
+
+  private enum whatNext
+  {
+    OKAY, CONTINUE, ERROR;
+  }
+
+  private whatNext checksBeforeWritingToFile(ArgValuesMap avm,
+          SubVals subVal, boolean includeBackups, String filename,
+          String adjective)
+  {
+    File file = new File(filename);
+
+    boolean overwrite = ArgParser.getFromSubValArgOrPref(avm, Arg.OVERWRITE,
+            subVal, null, "OVERWRITE_OUTPUT", false);
+    boolean stdout = false;
+    boolean backups = false;
+    if (includeBackups)
+    {
+      stdout = ArgParser.STDOUTFILENAME.equals(filename);
+      // backups. Use the Arg.BACKUPS or subval "backups" setting first,
+      // otherwise if headless assume false, if not headless use the user
+      // preference with default true.
+      backups = ArgParser.getFromSubValArgOrPref(avm, Arg.BACKUPS, subVal,
+              null, Platform.isHeadless() ? null : BackupFiles.ENABLED,
+              !Platform.isHeadless());
+    }
+
+    if (file.exists() && !(overwrite || backups || stdout))
+    {
+      addWarn("Won't overwrite file '" + filename + "' without "
+              + Arg.OVERWRITE.argString()
+              + (includeBackups ? " or " + Arg.BACKUPS.argString() : "")
+              + " set");
+      return whatNext.CONTINUE;
+    }
+
+    boolean mkdirs = ArgParser.getFromSubValArgOrPref(avm, Arg.MKDIRS,
+            subVal, null, "MKDIRS_OUTPUT", false);
+
+    if (!FileUtils.checkParentDir(file, mkdirs))
+    {
+      addError("Directory '"
+              + FileUtils.getParentDir(file).getAbsolutePath()
+              + "' does not exist for " + adjective + " file '" + filename
+              + "'."
+              + (mkdirs ? "" : "  Try using " + Arg.MKDIRS.argString()));
+      return whatNext.ERROR;
+    }
+
+    return whatNext.OKAY;
+  }
 }