From 2ee2e17dc404c02d94ae4271130ea8000aee9724 Mon Sep 17 00:00:00 2001 From: Ben Soares Date: Tue, 12 Sep 2023 00:16:44 +0100 Subject: [PATCH] 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. --- src/jalview/bin/Commands.java | 276 ++++++++++++++++++++++++++++++----------- 1 file changed, 202 insertions(+), 74 deletions(-) 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 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; + } } -- 1.7.10.2