From: Ben Soares Date: Thu, 25 Oct 2018 15:36:41 +0000 (+0100) Subject: JAL-3141 Ostensibly all backups working for alignment and project saves. Need to... X-Git-Tag: Release_2_11_0~17^2~97^2~47 X-Git-Url: http://source.jalview.org/gitweb/?p=jalview.git;a=commitdiff_plain;h=95d4b3209ee3ab5f13232da00dc23153da86bc28 JAL-3141 Ostensibly all backups working for alignment and project saves. Need to add a preferences tab and some tests. --- diff --git a/resources/lang/Messages.properties b/resources/lang/Messages.properties index ae5b0e7..bcd63ae 100644 --- a/resources/lang/Messages.properties +++ b/resources/lang/Messages.properties @@ -1363,3 +1363,7 @@ label.most_bound_molecules = Most Bound Molecules label.most_polymer_residues = Most Polymer Residues label.cached_structures = Cached Structures label.free_text_search = Free Text Search +label.backupfiles_confirm_delete = Confirm delete +label.backupfiles_confirm_delete_old_files = Delete the following older version files? +label.backupfiles_confirm_save_file = Confirm save file +label.backupfiles_confirm_save_file_backupfiles_roll_wrong = Something possibly went wrong with the backups of this file, write the new file anyway? diff --git a/resources/lang/Messages_es.properties b/resources/lang/Messages_es.properties index 555977d..9498aaf 100644 --- a/resources/lang/Messages_es.properties +++ b/resources/lang/Messages_es.properties @@ -1364,3 +1364,7 @@ label.most_bound_molecules = M label.most_polymer_residues = Más Residuos de Polímeros label.cached_structures = Estructuras en Caché label.free_text_search = Búsqueda de texto libre +label.backupfiles_confirm_delete = Confirmar borrar +label.backupfiles_confirm_delete_old_files = ¿Borrar los siguientes archivos? +label.backupfiles_confirm_save_file = Confirmar guardar archivo +label.backupfiles_confirm_save_file_backupfiles_roll_wrong = Posiblemente algo está mal con los archivos de copia de seguridad. ¿Guardar el nuevo archivo? diff --git a/src/jalview/gui/AlignFrame.java b/src/jalview/gui/AlignFrame.java index 94b38ed..268139d 100644 --- a/src/jalview/gui/AlignFrame.java +++ b/src/jalview/gui/AlignFrame.java @@ -64,6 +64,7 @@ import jalview.gui.ColourMenuHelper.ColourChangeListener; import jalview.gui.ViewSelectionMenu.ViewSetProvider; import jalview.io.AlignmentProperties; import jalview.io.AnnotationFile; +import jalview.io.BackupFiles; import jalview.io.BioJsHTMLOutput; import jalview.io.DataSourceType; import jalview.io.FileFormat; @@ -1186,9 +1187,13 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, } else { + // create backupfiles object and get new temp filename destination + BackupFiles backupfiles = new BackupFiles(file); + try { - PrintWriter out = new PrintWriter(new FileWriter(file)); + PrintWriter out = new PrintWriter( + new FileWriter(backupfiles.getTempFilePath())); out.print(output); out.close(); @@ -1201,6 +1206,11 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, success = false; ex.printStackTrace(); } + + backupfiles.setWriteSuccess(success); + // do the backup file roll and rename the temp file to actual file + success = backupfiles.rollBackupsAndRenameTempFile(); + } } diff --git a/src/jalview/gui/Jalview2XML.java b/src/jalview/gui/Jalview2XML.java index 9285754..2c3d2d2 100644 --- a/src/jalview/gui/Jalview2XML.java +++ b/src/jalview/gui/Jalview2XML.java @@ -43,6 +43,7 @@ import jalview.datamodel.features.FeatureMatcherSet; import jalview.datamodel.features.FeatureMatcherSetI; import jalview.ext.varna.RnaModel; import jalview.gui.StructureViewer.ViewerType; +import jalview.io.BackupFiles; import jalview.io.DataSourceType; import jalview.io.FileFormat; import jalview.renderer.ResidueShaderI; @@ -52,7 +53,6 @@ import jalview.schemabinding.version2.Annotation; import jalview.schemabinding.version2.AnnotationColours; import jalview.schemabinding.version2.AnnotationElement; import jalview.schemabinding.version2.CalcIdParam; -import jalview.schemabinding.version2.Colour; import jalview.schemabinding.version2.CompoundMatcher; import jalview.schemabinding.version2.DBRef; import jalview.schemabinding.version2.Features; @@ -463,10 +463,16 @@ public class Jalview2XML FileOutputStream fos = null; try { - fos = new FileOutputStream(statefile); + + BackupFiles backupfiles = new BackupFiles(statefile); + fos = new FileOutputStream(backupfiles.getTempFile()); + JarOutputStream jout = new JarOutputStream(fos); saveState(jout); + backupfiles.setWriteSuccess(true); + backupfiles.rollBackupsAndRenameTempFile(); + } catch (Exception e) { // TODO: inform user of the problem - they need to know if their data was @@ -647,7 +653,11 @@ public class Jalview2XML { try { - FileOutputStream fos = new FileOutputStream(jarFile); + // create backupfiles object and get new temp filename destination + BackupFiles backupfiles = new BackupFiles(jarFile); + FileOutputStream fos = new FileOutputStream( + backupfiles.getTempFilePath()); + JarOutputStream jout = new JarOutputStream(fos); List frames = new ArrayList<>(); @@ -669,7 +679,12 @@ public class Jalview2XML } ; jout.close(); - return true; + boolean success = true; + + backupfiles.setWriteSuccess(success); + success = backupfiles.rollBackupsAndRenameTempFile(); + + return success; } catch (Exception ex) { errorMessage = "Couldn't Write alignment view to Jalview Archive - see error output for details"; diff --git a/src/jalview/io/BackupFilenameFilter.java b/src/jalview/io/BackupFilenameFilter.java index 8cf7cbf..568f1c9 100644 --- a/src/jalview/io/BackupFilenameFilter.java +++ b/src/jalview/io/BackupFilenameFilter.java @@ -2,6 +2,7 @@ package jalview.io; import java.io.File; import java.io.FilenameFilter; +import java.io.IOException; public class BackupFilenameFilter implements FilenameFilter { @@ -24,27 +25,34 @@ public class BackupFilenameFilter implements FilenameFilter } @Override - public boolean accept(File file, String filename) + public boolean accept(File dir, String filename) { - // CHECK THIS IS NOT ALWAYS THE PARENT DIR - if (file.isDirectory()) + boolean ret = false; + try { - return true; - } - else + File file = new File( + dir.getCanonicalPath() + File.separatorChar + filename); + if (file.isDirectory()) + { + // backup files aren't dirs! + return false; + } + } catch (IOException e) { - BackupFilenameParts bffp = new BackupFilenameParts(filename, base, - template, digits, extension); - return bffp.isBackupFile(); + System.out.println("IOException when checking file '" + filename + + "' is a backupfile"); } + + BackupFilenameParts bffp = new BackupFilenameParts(filename, base, + template, digits, extension); + ret = bffp.isBackupFile(); + return ret; } } class BackupFilenameParts { - File file; - String base; String templateStart; @@ -70,8 +78,15 @@ class BackupFilenameParts { this.isBackupFile = false; - if (!(filename.startsWith(base) && filename.endsWith(extension))) + // calculate minimum length of a backup filename + int minlength = base.length() + template.length() + - BackupFiles.NUM_PLACEHOLDER.length() + digits + + extension.length(); + + if (!(filename.startsWith(base) && filename.endsWith(extension) + && filename.length() >= minlength)) { + // non-starter return; } @@ -83,16 +98,17 @@ class BackupFilenameParts templateStart = template.substring(0, numcharstart); templateEnd = template.substring(numcharstart + BackupFiles.NUM_PLACEHOLDER.length()); } + int startLength = base.length() + templateStart.length(); int endLength = templateEnd.length() + extension.length(); - String numString = filename.substring(startLength, filename.length() - endLength + 1); + String numString = filename.substring(startLength, + filename.length() - endLength); if (filename.length() >= startLength + digits + endLength && filename.startsWith(base + templateStart) && filename.endsWith(templateEnd + extension) && numString.matches("[0-9]+")) { - this.file = file; this.base = base; this.templateStart = templateStart; this.num = Integer.parseInt(numString); diff --git a/src/jalview/io/BackupFiles.java b/src/jalview/io/BackupFiles.java index 39cd23e..ff23cbf 100644 --- a/src/jalview/io/BackupFiles.java +++ b/src/jalview/io/BackupFiles.java @@ -1,6 +1,9 @@ package jalview.io; import jalview.bin.Cache; +import jalview.gui.Desktop; +import jalview.gui.JvOptionPane; +import jalview.util.MessageManager; import java.io.File; import java.io.IOException; @@ -16,6 +19,7 @@ import java.util.TreeMap; * BACKUPFILES_ROLL_MAX - the maximum number of backupfiles to keep for any one alignment or project file. * BACKUPFILES_SUFFIX_DIGITS - the number of digits to insert replace %n with (e.g. BACKUPFILES_SUFFIX_DIGITS = 3 would make "001", "002", etc) * BACKUPFILES_REVERSE_ORDER - if true then "logfile" style numbering and file rolling will occur. If false then ever-increasing version numbering will occur, but old files will still be deleted if there are more than ROLL_MAX backup files. + * BACKUPFILES_CONFIRM_DELETE_OLD - if true then prompt/confirm with the user when deleting older backup/version files. */ public class BackupFiles @@ -36,6 +40,8 @@ public class BackupFiles public static String REVERSE_ORDER = NS + "_REVERSE_ORDER"; + public static String CONFIRM_DELETE_OLD = NS + "_CONFIRM_DELETE_OLD"; + private static String DEFAULT_TEMP_FILE = "jalview_temp_file_" + NS; // file - File object to be backed up and then updated (written over) @@ -43,7 +49,13 @@ public class BackupFiles // enabled - default flag as to whether to do the backup file roll (if not // defined in preferences) - private boolean enabled; + private static boolean enabled; + + // confirmDelete - default flag as to whether to confirm with the user before + // deleting old backup/version files + private static boolean confirmDelete; + + private static boolean classInit = false; // defaultSuffix - default template to use to append to basename of file private String suffix; @@ -61,48 +73,34 @@ public class BackupFiles // temp saved file to become new saved file private File tempFile; + // flag set to see if file save to temp file was successful + private boolean tempFileWriteSuccess; + public BackupFiles(String filename) { this(new File(filename)); } - // first time defaults for ENABLED, SUFFIX, ROLL_MAX, SUFFIX_DIGITS and - // REVERSE_ORDER + // first time defaults for SUFFIX, ROLL_MAX, SUFFIX_DIGITS and REVERSE_ORDER public BackupFiles(File file) { - this(file, true, "-v" + NUM_PLACEHOLDER, 4, 3, false); - } - - // set, get and rename temp file into place - public void setTempFile(File temp) - { - this.tempFile = temp; - } - - public File getTempFile() - { - return tempFile; - } - - public boolean renameTempFile() - { - return tempFile.renameTo(file); + this(file, "-v" + NUM_PLACEHOLDER, 4, 3, false); } - public BackupFiles(File file, boolean defaultEnabled, - String defaultSuffix, - int defaultMax, int defaultDigits, boolean defaultReverseOrder) + public BackupFiles(File file, + String defaultSuffix, int defaultMax, int defaultDigits, + boolean defaultReverseOrder) { + classInit(); this.file = file; - this.enabled = Cache.getDefault(ENABLED, defaultEnabled); this.suffix = Cache.getDefault(SUFFIX, defaultSuffix); this.max = Cache.getDefault(ROLL_MAX, defaultMax); this.digits = Cache.getDefault(SUFFIX_DIGITS, defaultDigits); this.reverseOrder = Cache.getDefault(REVERSE_ORDER, defaultReverseOrder); - + // create a temp file to save new data in - File temp; + File temp = null; try { if (file != null) @@ -114,7 +112,6 @@ public class BackupFiles else { temp = File.createTempFile(DEFAULT_TEMP_FILE, ".tmp"); - setTempFile(temp); } } catch (IOException e) { @@ -124,15 +121,91 @@ public class BackupFiles { System.out.println("Exception ctreating temp file for saving"); } + this.setTempFile(temp); + } + + public static void classInit() + { + if (!classInit) + { + setEnabled(Cache.getDefault(ENABLED, true)); + setConfirmDelete(Cache.getDefault(CONFIRM_DELETE_OLD, true)); + classInit = true; + } + } + + public static void setEnabled(boolean flag) + { + enabled = flag; + } + + public static boolean getEnabled() + { + classInit(); + return enabled; + } + + public static void setConfirmDelete(boolean flag) + { + confirmDelete = flag; + } + + public static boolean getConfirmDelete() + { + classInit(); + return confirmDelete; + } + + // set, get and rename temp file into place + public void setTempFile(File temp) + { + this.tempFile = temp; + } + + public File getTempFile() + { + return tempFile; + } + + public String getTempFilePath() + { + String path = null; + try + { + path = this.getTempFile().getCanonicalPath(); + } catch (IOException e) + { + System.out.println( + "IOException when getting Canonical Path of temp file '" + + this.getTempFile().getName() + "'"); + } + return path; + } + public boolean setWriteSuccess(boolean flag) + { + boolean old = this.tempFileWriteSuccess; + this.tempFileWriteSuccess = flag; + return old; + } + + public boolean getWriteSuccess() + { + return this.tempFileWriteSuccess; + } + + public boolean renameTempFile() + { + return tempFile.renameTo(file); } + // roll the backupfiles public boolean rollBackupFiles() { // file doesn't yet exist or backups are not enabled - if ((!file.exists()) || (!enabled) || (max < 1)) + if ((!file.exists()) || (!enabled) || (max < -1)) { // nothing to do return true; @@ -166,128 +239,223 @@ public class BackupFiles boolean ret = true; // Create/move backups up one - String numString = null; - File lastfile = null; - if (reverseOrder) + File[] oldFilesToDelete = null; + + // find existing backup files + BackupFilenameFilter bff = new BackupFilenameFilter(basename, suffix, + digits, + extension); + File[] backupFiles = dirFile.listFiles(bff); + int nextIndexNum = 0; + String confirmDeleteExtraInfo = null; + + if (backupFiles.length == 0) + { + // No other backup files. Just need to move existing file to backupfile_1 + nextIndexNum = 1; + } + else { - // backup style numbering - int tempMax = max; - // max == -1 means no limits - if (max == -1) + // sort the backup files (based on integer found in the suffix) using a + // precomputed Hashmap for speed + HashMap bfHashMap = new HashMap<>(); + for (int i = 0; i < backupFiles.length; i++) { - // do something cleverer here (possibly)! - tempMax = 10000; + File f = backupFiles[i]; + BackupFilenameParts bfp = new BackupFilenameParts(f, basename, suffix, digits, extension); + bfHashMap.put(bfp.indexNum(), f); } + TreeMap bfTreeMap = new TreeMap<>(); + bfTreeMap.putAll(bfHashMap); - for (int m = 0; m < tempMax; m++) + if (reverseOrder) { - int n = tempMax - m; - String backupfilename = dir + File.separatorChar - + BackupFilenameParts.getBackupFilename(n, basename, suffix, - digits, extension); - File backupfile_n = new File(backupfilename); - - if (!backupfile_n.exists()) + // backup style numbering + + File lastfile = null; + int tempMax = max; + // max == -1 means no limits + // look for first "gap" in backupFiles + // if tempMax is -1 at this stage just keep going until there's a gap... + // why do I feel a little uneasy about this loop?.. + for (int i = 1; tempMax < 0 || (max >= 0 && i <= max); i++) { - lastfile = backupfile_n; - continue; + if (!bfTreeMap.containsKey(i)) // first non-existent backupfile + { + tempMax = i; + } } - if (m == 0) - { // Move the max backup to /tmp instead of deleting (Just In - // Case) - String tmpfile = "tmp-" + backupfile_n.getName(); - try + for (int m = 0; m < tempMax; m++) + { + int n = tempMax - m; + String backupfilename = dir + File.separatorChar + + BackupFilenameParts.getBackupFilename(n, basename, + suffix, digits, extension); + File backupfile_n = new File(backupfilename); + + if (!backupfile_n.exists()) { - File tmpFile = File.createTempFile(tmpfile, ".tmp"); - ret = ret && backupfile_n.renameTo(tmpFile); - } catch (IOException e) + lastfile = backupfile_n; + continue; + } + + if (m == 0 && backupfile_n.exists()) { - System.out.println( - "Could not create temp file '" + tmpfile + ".tmp'"); + // move the largest (max) rolled file to a temp file and add to the delete list + try + { + File temp = File.createTempFile(backupfilename, ".tmp", + dirFile); + backupfile_n.renameTo(temp); + + oldFilesToDelete = new File[] { temp }; + confirmDeleteExtraInfo = "(was " + backupfile_n.getName() + + ")"; + } catch (IOException e) + { + System.out.println( + "IOException when creating temporary file for backupfilename"); + } } - } - else - { - // Just In Case - if (lastfile != null) + else { - ret = ret && backupfile_n.renameTo(lastfile); + // Just In Case + if (lastfile != null) + { + ret = ret && backupfile_n.renameTo(lastfile); + } } + + lastfile = backupfile_n; } - lastfile = backupfile_n; + // index to use for the latest backup + nextIndexNum = 1; } - - // now actually backup the important file! - ret = ret && file.renameTo(lastfile); - } - else - { - // version style numbering (with earliest file deletion if max files - // reached) - - // find existing backup files - BackupFilenameFilter bff = new BackupFilenameFilter(basename, suffix, - digits, - extension); - File[] backupFiles = dirFile.listFiles(bff); - int nextIndexNum; - - if (backupFiles.length == 0) + else { - nextIndexNum = 1; - } else { + // version style numbering (with earliest file deletion if max files + // reached) - // and sort them (based on integer found in the suffix) using a - // precomputed Hashmap for speed - HashMap bfHashMap = new HashMap(); - for (int i = 0; i < backupFiles.length; i++) - { - File f = backupFiles[i]; - BackupFilenameParts bfp = new BackupFilenameParts(f, basename, suffix, digits, extension); - bfHashMap.put(bfp.indexNum(), f); - } - TreeMap bfTreeMap = new TreeMap<>(); - bfTreeMap.putAll(bfHashMap); bfTreeMap.values().toArray(backupFiles); - + // max value of -1 means keep all backup files if (bfTreeMap.size() >= max && max != -1) { // need to delete some files to keep number of backups to designated // max - int numToDelete = bfTreeMap.size() - max; - File[] filesToDelete = Arrays.copyOfRange(backupFiles, 0, - numToDelete - 1); - - /****************************************** - * CONFIRM THESE DELETIONS WITH THE USER! * - ******************************************/ - for (int i = 0; i < filesToDelete.length; i++) - { - File toDelete = filesToDelete[i]; - toDelete.delete(); - } + int numToDelete = bfTreeMap.size() - max + 1; + oldFilesToDelete = Arrays.copyOfRange(backupFiles, 0, + numToDelete); } nextIndexNum = bfTreeMap.lastKey() + 1; - // Let's make the new backup file!! yay, got there at last! - String nextBackupFilename = dir + File.separatorChar - + BackupFilenameParts.getBackupFilename(nextIndexNum, - basename, suffix, digits, extension); - File nextBackupFile = new File(nextBackupFilename); - ret = ret && file.renameTo(nextBackupFile); } } + if (oldFilesToDelete != null && oldFilesToDelete.length > 0) + { + // delete old backup/version files + + boolean delete = false; + if (confirmDelete) + { + // Object[] confirmMessageArray = {}; + StringBuilder confirmMessage = new StringBuilder(); + confirmMessage.append(MessageManager + .getString("label.backupfiles_confirm_delete_old_files")); + for (File f : oldFilesToDelete) + { + confirmMessage.append("\n"); + confirmMessage.append(f.getName()); + } + if (confirmDeleteExtraInfo != null + && confirmDeleteExtraInfo.length() > 0) + { + confirmMessage.append("\n"); + confirmMessage.append(confirmDeleteExtraInfo); + } + int confirm = JvOptionPane.showConfirmDialog(Desktop.desktop, + confirmMessage.toString(), + MessageManager + .getString("label.backupfiles_confirm_delete"), + JvOptionPane.YES_NO_OPTION, JvOptionPane.WARNING_MESSAGE); + + delete = (confirm == JvOptionPane.YES_OPTION); + } + else + { + delete = true; + } + + if (delete) + { + for (int i = 0; i < oldFilesToDelete.length; i++) + { + File fileToDelete = oldFilesToDelete[i]; + fileToDelete.delete(); + // System.out.println("DELETING '" + fileToDelete.getName() + + // "'"); + } + } + + } + + // Let's make the new backup file!! yay, got there at last! + String latestBackupFilename = dir + File.separatorChar + + BackupFilenameParts.getBackupFilename(nextIndexNum, basename, + suffix, digits, extension); + File latestBackupFile = new File(latestBackupFilename); + ret = ret && file.renameTo(latestBackupFile); + return ret; } + public boolean rollBackupsAndRenameTempFile() + { + boolean write = this.getWriteSuccess(); + + boolean roll = false; + if (write) { + roll = this.rollBackupFiles(); + } else { + return false; + } + + /* + * Not sure that this confirmation is desirable. By this stage the new file is + * already written successfully, but something (e.g. disk full) has happened while + * trying to roll the backup files, and most likely the filename needed will already + * be vacant so renaming the temp file is nearly always correct! + */ + if (!roll) + { + int confirm = JvOptionPane.showConfirmDialog(Desktop.desktop, + MessageManager.getString( + "label.backupfiles_confirm_save_file_backupfiles_roll_wrong"), + MessageManager.getString("label.backupfiles_confirm_save_file"), + JvOptionPane.YES_NO_OPTION, JvOptionPane.WARNING_MESSAGE); + + if (confirm == JvOptionPane.YES_OPTION) + { + roll = true; + } + } + + boolean rename = false; + if (roll) + { + rename = this.renameTempFile(); + } + + return rename; + } + } diff --git a/src/jalview/io/JalviewFileChooser.java b/src/jalview/io/JalviewFileChooser.java index 90a3cc0..192948d 100755 --- a/src/jalview/io/JalviewFileChooser.java +++ b/src/jalview/io/JalviewFileChooser.java @@ -336,10 +336,13 @@ public class JalviewFileChooser extends JFileChooser setSelectedFile(ourselectedFile); } } + // TODO: ENSURE THAT FILES SAVED WITH A ':' IN THE NAME ARE REFUSED AND THE - // USER PROMPTED FOR A NEW FILENAME + // USER PROMPTED FOR A NEW FILENAME. + // DO NOT need to confirm file overwrite if using backup files (the files + // aren't being overwritten!) if ((ret == JalviewFileChooser.APPROVE_OPTION) - && ourselectedFile.exists()) + && ourselectedFile.exists() && (!BackupFiles.getEnabled())) { int confirm = JvOptionPane.showConfirmDialog(parent, MessageManager.getString("label.overwrite_existing_file"), @@ -351,7 +354,6 @@ public class JalviewFileChooser extends JFileChooser ret = JalviewFileChooser.CANCEL_OPTION; } - // not happening here now rollBackupFiles(ourselectedFile); } return ret;