Refine BackupManager

CHANGELOG.md

@@ -11,10 +11,14 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 ### Added
 
+- In case a backup is found, the filename of the backup is shown.
+
 ### Changed
 
 - We improved the Citavi Importer to also import so called Knowledge-items into the field `comment` of the corresponding entry [#9025](https://github.com/JabRef/jabref/issues/9025)
 - We removed wrapping of string constants when writing to a `.bib` file.
+- We call backup files `.bak` and temporary writing files now `.sav`.
+- JabRef keeps 10 older versions of a `.bib` file in the [user data dir](https://github.com/harawata/appdirs#supported-directories) (instead of a single `.sav` (now: `.bak`) file in the directory of the `.bib` file)
 - We changed the button label from "Return to JabRef" to "Return to library" to better indicate the purpose of the action.
 
 ### Fixed

src/main/java/org/jabref/gui/JabRefGUI.java

@@ -77,8 +77,8 @@ private void openWindow(Stage mainStage) {
         if (guiPreferences.isWindowMaximised()) {
             mainStage.setMaximized(true);
         } else if ((Screen.getScreens().size() == 1) && isWindowPositionOutOfBounds()) {
-            // corrects the Window, if its outside of the mainscreen
-            LOGGER.debug("The Jabref Window is outside the Main Monitor\n");
+            // corrects the Window, if it is outside the mainscreen
+            LOGGER.debug("The Jabref window is outside the main screen\n");
             mainStage.setX(0);
             mainStage.setY(0);
             mainStage.setWidth(1024);

src/main/java/org/jabref/gui/dialogs/BackupUIManager.java

@@ -6,6 +6,8 @@
 import org.jabref.gui.util.DefaultTaskExecutor;
 import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.l10n.Localization;
+import org.jabref.logic.util.BackupFileType;
+import org.jabref.logic.util.io.FileUtil;
 
 /**
  * Stores all user dialogs related to {@link BackupManager}.
@@ -17,7 +19,10 @@ private BackupUIManager() {
 
     public static void showRestoreBackupDialog(DialogService dialogService, Path originalPath) {
         String content = new StringBuilder()
-                .append(Localization.lang("A backup file for '%0' was found.", originalPath.getFileName().toString()))
+                .append(Localization.lang("A backup file for '%0' was found at '%1'.",
+                        originalPath.getFileName().toString(),
+                        // We need to determine the path "manually" as the path does not get passed through when a diff is detected.
+                        FileUtil.getPathOfLatestExisingBackupFile(originalPath, BackupFileType.BACKUP).map(Path::toString).orElse(Localization.lang("File not found"))))
                 .append("\n")
                 .append(Localization.lang("This could indicate that JabRef did not shut down cleanly last time the file was used."))
                 .append("\n\n")

src/main/java/org/jabref/gui/externalfiles/FileExtensionViewModel.java

@@ -21,7 +21,7 @@ public class FileExtensionViewModel {
 
     FileExtensionViewModel(FileType fileType, FilePreferences filePreferences) {
         this.description = Localization.lang("%0 file", fileType.getName());
-        this.extensions = fileType.getExtensionsWithDot();
+        this.extensions = fileType.getExtensionsWithAsteriskAndDot();
         this.filePreferences = filePreferences;
     }
 

src/main/java/org/jabref/gui/util/FileFilterConverter.java

@@ -28,11 +28,11 @@ private FileFilterConverter() {
 
     public static FileChooser.ExtensionFilter toExtensionFilter(FileType fileType) {
         String description = Localization.lang("%0 file", fileType.toString());
-        return new FileChooser.ExtensionFilter(description, fileType.getExtensionsWithDot());
+        return new FileChooser.ExtensionFilter(description, fileType.getExtensionsWithAsteriskAndDot());
     }
 
     public static FileChooser.ExtensionFilter toExtensionFilter(String description, FileType fileType) {
-        return new FileChooser.ExtensionFilter(description, fileType.getExtensionsWithDot());
+        return new FileChooser.ExtensionFilter(description, fileType.getExtensionsWithAsteriskAndDot());
     }
 
     public static Optional<Importer> getImporter(FileChooser.ExtensionFilter extensionFilter, Collection<Importer> importers) {
@@ -46,7 +46,7 @@ public static Optional<Exporter> getExporter(FileChooser.ExtensionFilter extensi
     public static FileChooser.ExtensionFilter forAllImporters(SortedSet<Importer> importers) {
         List<FileType> fileTypes = importers.stream().map(Importer::getFileType).collect(Collectors.toList());
         List<String> flatExtensions = fileTypes.stream()
-                                               .flatMap(type -> type.getExtensionsWithDot().stream())
+                                               .flatMap(type -> type.getExtensionsWithAsteriskAndDot().stream())
                                                .collect(Collectors.toList());
 
         return new FileChooser.ExtensionFilter(Localization.lang("Available import formats"), flatExtensions);

src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java

@@ -1,22 +1,26 @@
 package org.jabref.logic.autosaveandbackup;
 
 import java.io.IOException;
+import java.io.Writer;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Optional;
+import java.util.Queue;
 import java.util.Set;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
 
 import org.jabref.logic.bibtex.InvalidFieldValueException;
-import org.jabref.logic.exporter.AtomicFileWriter;
 import org.jabref.logic.exporter.BibWriter;
 import org.jabref.logic.exporter.BibtexDatabaseWriter;
 import org.jabref.logic.exporter.SavePreferences;
+import org.jabref.logic.util.BackupFileType;
 import org.jabref.logic.util.CoarseChangeFilter;
 import org.jabref.logic.util.DelayTaskThrottler;
 import org.jabref.logic.util.io.FileUtil;
@@ -40,8 +44,7 @@ public class BackupManager {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(BackupManager.class);
 
-    // This differs from org.jabref.logic.exporter.AtomicFileOutputStream.BACKUP_EXTENSION, which is used for copying the .bib away before overwriting on save.
-    private static final String AUTOSAVE_FILE_EXTENSION = ".sav";
+    private static final int MAXIMUM_BACKUP_FILE_COUNT = 10;
 
     private static Set<BackupManager> runningInstances = new HashSet<>();
 
@@ -51,6 +54,11 @@ public class BackupManager {
     private final CoarseChangeFilter changeFilter;
     private final BibEntryTypesManager entryTypesManager;
 
+
+    // Contains a list of all backup paths
+    // During a write, the less recent backup file is deleted
+    private final Queue<Path> backupFilesQueue = new LinkedBlockingQueue<>();
+
     private BackupManager(BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, PreferencesService preferences) {
         this.bibDatabaseContext = bibDatabaseContext;
         this.entryTypesManager = entryTypesManager;
@@ -61,8 +69,18 @@ private BackupManager(BibDatabaseContext bibDatabaseContext, BibEntryTypesManage
         changeFilter.registerListener(this);
     }
 
-    static Path getBackupPath(Path originalPath) {
-        return FileUtil.addExtension(originalPath, AUTOSAVE_FILE_EXTENSION);
+    /**
+     * Determines the most recent backup file name
+     */
+    static Path getBackupPathForNewBackup(Path originalPath) {
+        return FileUtil.getPathForNewBackupFileAndCreateDirectory(originalPath, BackupFileType.BACKUP);
+    }
+
+    /**
+     * Determines the most recent existing backup file name
+     */
+    static Optional<Path> getLatestBackupPath(Path originalPath) {
+        return FileUtil.getPathOfLatestExisingBackupFile(originalPath, BackupFileType.BACKUP);
     }
 
     /**
@@ -99,13 +117,13 @@ public static void shutdown(BibDatabaseContext bibDatabaseContext) {
      * the user checks the output.
      */
     public static boolean backupFileDiffers(Path originalPath) {
-        Path backupPath = getBackupPath(originalPath);
-        if (!Files.exists(backupPath) || Files.isDirectory(backupPath)) {
+        Optional<Path> backupPath = getLatestBackupPath(originalPath);
+        if (backupPath.isEmpty()) {
             return false;
         }
 
         try {
-            return Files.mismatch(originalPath, backupPath) != -1L;
+            return Files.mismatch(originalPath, backupPath.get()) != -1L;
         } catch (IOException e) {
             LOGGER.debug("Could not compare original file and backup file.", e);
             // User has to investigate in this case
@@ -119,28 +137,50 @@ public static boolean backupFileDiffers(Path originalPath) {
      * @param originalPath Path to the file which should be equalized to the backup file.
      */
     public static void restoreBackup(Path originalPath) {
-        Path backupPath = getBackupPath(originalPath);
+        Optional<Path> backupPath = getLatestBackupPath(originalPath);
+        if (backupPath.isEmpty()) {
+            LOGGER.error("There is no backup file");
+            return;
+        }
         try {
-            Files.copy(backupPath, originalPath, StandardCopyOption.REPLACE_EXISTING);
+            Files.copy(backupPath.get(), originalPath, StandardCopyOption.REPLACE_EXISTING);
         } catch (IOException e) {
             LOGGER.error("Error while restoring the backup file.", e);
         }
     }
 
-    private Optional<Path> determineBackupPath() {
-        return bibDatabaseContext.getDatabasePath().map(BackupManager::getBackupPath);
+    private Optional<Path> determineBackupPathForNewBackup() {
+        return bibDatabaseContext.getDatabasePath().map(BackupManager::getBackupPathForNewBackup);
     }
 
+    /**
+     * This method is called as soon as the scheduler says: "Do the backup"
+     *
+     * <em>SIDE EFFECT: Deletes oldest backup file</em>
+     *
+     * @param backupPath the path where the library should be backed up to
+     */
     private void performBackup(Path backupPath) {
+        // We opted for "while" to delete backups in case there are more than 10
+        while (backupFilesQueue.size() >= MAXIMUM_BACKUP_FILE_COUNT) {
+            Path lessRecentBackupFile = backupFilesQueue.poll();
+            try {
+                Files.delete(lessRecentBackupFile);
+            } catch (IOException e) {
+                LOGGER.error("Could not delete backup file {}", lessRecentBackupFile, e);
+            }
+        }
+
         // code similar to org.jabref.gui.exporter.SaveDatabaseAction.saveDatabase
         GeneralPreferences generalPreferences = preferences.getGeneralPreferences();
         SavePreferences savePreferences = preferences.getSavePreferences()
                                                      .withMakeBackup(false);
         Charset encoding = bibDatabaseContext.getMetaData().getEncoding().orElse(StandardCharsets.UTF_8);
-        try (AtomicFileWriter fileWriter = new AtomicFileWriter(backupPath, encoding)) {
-            BibWriter bibWriter = new BibWriter(fileWriter, bibDatabaseContext.getDatabase().getNewLineSeparator());
+        try (Writer writer = Files.newBufferedWriter(backupPath, encoding)) {
+            BibWriter bibWriter = new BibWriter(writer, bibDatabaseContext.getDatabase().getNewLineSeparator());
             new BibtexDatabaseWriter(bibWriter, generalPreferences, savePreferences, entryTypesManager)
                     .saveDatabase(bibDatabaseContext);
+            backupFilesQueue.add(backupPath);
         } catch (IOException e) {
             logIfCritical(backupPath, e);
         }
@@ -167,7 +207,30 @@ public synchronized void listen(@SuppressWarnings("unused") BibDatabaseContextCh
     }
 
     private void startBackupTask() {
-        throttler.schedule(() -> determineBackupPath().ifPresent(this::performBackup));
+        fillQueue();
+
+        // We need to determine the backup path on each action, because the user might have saved the file to a different location
+        throttler.schedule(() -> determineBackupPathForNewBackup().ifPresent(this::performBackup));
+    }
+
+    private void fillQueue() {
+        Path backupDir = FileUtil.getAppDataBackupDir();
+        if (!Files.exists(backupDir)) {
+            return;
+        }
+        bibDatabaseContext.getDatabasePath().ifPresent(databasePath -> {
+            // code similar to {@link org.jabref.logic.util.io.FileUtil.getPathOfLatestExisingBackupFile}
+            final String prefix = FileUtil.getUniqueFilePrefix(databasePath) + "--" + databasePath.getFileName();
+            try {
+                List<Path> allSavFiles = Files.list(backupDir)
+                                              // just list the .sav belonging to the given targetFile
+                                              .filter(p -> p.getFileName().toString().startsWith(prefix))
+                                              .sorted().toList();
+                backupFilesQueue.addAll(allSavFiles);
+            } catch (IOException e) {
+                LOGGER.error("Could not determine most recent file", e);
+            }
+        });
     }
 
     /**
@@ -178,16 +241,5 @@ private void shutdown() {
         changeFilter.unregisterListener(this);
         changeFilter.shutdown();
         throttler.shutdown();
-        determineBackupPath().ifPresent(this::deleteBackupFile);
-    }
-
-    private void deleteBackupFile(Path backupPath) {
-        try {
-            if (Files.exists(backupPath) && !Files.isDirectory(backupPath)) {
-                Files.delete(backupPath);
-            }
-        } catch (IOException e) {
-            LOGGER.error("Error while deleting the backup file.", e);
-        }
     }
 }

src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

@@ -12,6 +12,7 @@
 import java.util.EnumSet;
 import java.util.Set;
 
+import org.jabref.logic.util.BackupFileType;
 import org.jabref.logic.util.io.FileUtil;
 
 import org.slf4j.Logger;
@@ -47,7 +48,7 @@ public class AtomicFileOutputStream extends FilterOutputStream {
     private static final Logger LOGGER = LoggerFactory.getLogger(AtomicFileOutputStream.class);
 
     private static final String TEMPORARY_EXTENSION = ".tmp";
-    private static final String BACKUP_EXTENSION = ".bak";
+    private static final String SAVE_EXTENSION = BackupFileType.SAVE.getExtensions().get(0);
 
     /**
      * The file we want to create/replace.
@@ -76,7 +77,7 @@ public AtomicFileOutputStream(Path path, boolean keepBackup) throws IOException
 
         this.targetFile = path;
         this.temporaryFile = getPathOfTemporaryFile(path);
-        this.backupFile = getPathOfBackupFile(path);
+        this.backupFile = getPathOfSaveBackupFile(path);
         this.keepBackup = keepBackup;
 
         try {
@@ -104,8 +105,8 @@ private static Path getPathOfTemporaryFile(Path targetFile) {
         return FileUtil.addExtension(targetFile, TEMPORARY_EXTENSION);
     }
 
-    private static Path getPathOfBackupFile(Path targetFile) {
-        return FileUtil.addExtension(targetFile, BACKUP_EXTENSION);
+    private static Path getPathOfSaveBackupFile(Path targetFile) {
+        return FileUtil.addExtension(targetFile, SAVE_EXTENSION);
     }
 
     /**

src/main/java/org/jabref/logic/util/BackupFileType.java

@@ -0,0 +1,32 @@
+package org.jabref.logic.util;
+
+import java.util.Collections;
+import java.util.List;
+
+public enum BackupFileType implements FileType {
+
+    // Used at BackupManager
+    BACKUP("Backup", "bak"),
+
+    // Used when writing the .bib file. See {@link org.jabref.logic.exporter.AtomicFileWriter}
+    // Used for copying the .bib away before overwriting on save.
+    SAVE("AutoSaveFile", "sav");
+
+    private final List<String> extensions;
+    private final String name;
+
+    BackupFileType(String name, String extension) {
+        this.extensions = Collections.singletonList(extension);
+        this.name = name;
+    }
+
+    @Override
+    public List<String> getExtensions() {
+        return extensions;
+    }
+
+    @Override
+    public String getName() {
+        return this.name;
+    }
+}

src/main/java/org/jabref/logic/util/FileType.java

@@ -8,7 +8,7 @@
  */
 public interface FileType {
 
-    default List<String> getExtensionsWithDot() {
+    default List<String> getExtensionsWithAsteriskAndDot() {
         return getExtensions().stream()
                               .map(extension -> "*." + extension)
                               .collect(Collectors.toList());