PDFs are not stored next to .bib file if file directory is configured (…

…#9113) * Fix typos * Add comment * FileLinkPreferences now rely on FilePreferences * The fallback directory of the file folder now is the general file directory. Co-authored-by: Kevin Klein <kevin@0x002a.com> * Improve LOGGER call with {} * The global default directory for storing PDFs is now the subdirectory "JabRef" in the user's home Co-authored-by: Kevin Klein <kevin@0x002a.com> * Adapt tests to new behavior Co-authored-by: Kevin Klein <kevin@0x002a.com> * Refine comment * Try to fix checkstyle * Fix typo * Fix "illegal unicode escape" * Add architecture test and move to java desktop remove obsolete checks * Add missing class * remove check for empty file dirs * fix tests * Fix checkstye * fix copy paste error * Revert "remove check for empty file dirs" This reverts commit 279ae2e. * Fix tests Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Kevin Klein <kevin@0x002a.com> Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
JabRef · Sep 3, 2022 · 4685800 · 4685800
1 parent 858e942
commit 4685800
Show file tree

Hide file tree

Showing 12 changed files with 134 additions and 73 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We removed "last-search-date" from the SLR feature, because the last-search-date can be deducted from the git logs. [#9116](https://github.com/JabRef/jabref/pull/9116)
 - A user can now add arbitrary data into `study.yml`. JabRef just ignores this data. [#9124](https://github.com/JabRef/jabref/pull/9124)
 - We reworked the External Changes Resolver dialog. [#9021](https://github.com/JabRef/jabref/pull/9021)
+- The fallback directory of the file folder now is the general file directory. In case there was a directory configured for a library and this directory was not found, JabRef placed the PDF next to the .bib file and not into the general file directory.
+- The global default directory for storing PDFs is now the subdirectory "JabRef" in the user's home.
 - We reworked the Define study parameters dialog. [#9123](https://github.com/JabRef/jabref/pull/9123)
 
 ### Fixed
@@ -40,7 +42,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed a bug where spaces are trimmed when highlighting differences in the Entries merge dialog. [koppor#371](https://github.com/koppor/jabref/issues/371)
 - We fixed some visual glitches with the linked files editor field in the entry editor and increased its height. [#8823](https://github.com/JabRef/jabref/issues/8823)
 - We fixed several bugs regarding the manual and the autosave of library files that sometimes lead to exceptions or data loss. [#9067](https://github.com/JabRef/jabref/pull/9067), [#8448](https://github.com/JabRef/jabref/issues/8484), [#8746](https://github.com/JabRef/jabref/issues/8746), [#6684](https://github.com/JabRef/jabref/issues/6684), [#6644](https://github.com/JabRef/jabref/issues/6644), [#6102](https://github.com/JabRef/jabref/issues/6102), [#6002](https://github.com/JabRef/jabref/issues/6000)
-- We fixed an issue where applied save actions on saving the library file would lead to the dialog "The libary has been modified by another program" popping up [#4877](https://github.com/JabRef/jabref/issues/4877)
+- We fixed an issue where applied save actions on saving the library file would lead to the dialog "The library has been modified by another program" popping up [#4877](https://github.com/JabRef/jabref/issues/4877)
 - We fixed issues with save actions not correctly loaded when opening the library. [#9122](https://github.com/JabRef/jabref/pull/9122)
 - We fixed an issue where title case didn't capitalize words after en-dash characters. [#9068](https://github.com/JabRef/jabref/pull/9068)
 - We fixed an issue where JabRef would not exit when a connection to a LibreOffice document was established previously and the document is still open. [#9075](https://github.com/JabRef/jabref/issues/9075)

diff --git a/src/main/java/org/jabref/architecture/AllowedToUseSwing.java b/src/main/java/org/jabref/architecture/AllowedToUseSwing.java
@@ -0,0 +1,10 @@
+package org.jabref.architecture;
+
+/**
+ * Annotation to indicate that this logic class can access swing
+ */
+public @interface AllowedToUseSwing {
+
+    // The rationale
+    String value();
+}
diff --git a/src/main/java/org/jabref/gui/desktop/JabRefDesktop.java b/src/main/java/org/jabref/gui/desktop/JabRefDesktop.java
@@ -10,6 +10,9 @@
 import java.util.Optional;
 import java.util.regex.Pattern;
 
+import javax.swing.filechooser.FileSystemView;
+
+import org.jabref.architecture.AllowedToUseSwing;
 import org.jabref.gui.DialogService;
 import org.jabref.gui.Globals;
 import org.jabref.gui.desktop.os.DefaultDesktop;
@@ -37,6 +40,7 @@
  * TODO: Replace by http://docs.oracle.com/javase/7/docs/api/java/awt/Desktop.html
  * http://stackoverflow.com/questions/18004150/desktop-api-is-not-supported-on-the-current-platform
  */
+@AllowedToUseSwing("Needs access to swing for the user's os dependent file chooser path")
 public class JabRefDesktop {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(JabRefDesktop.class);
@@ -288,6 +292,21 @@ public static void openConsole(File file, PreferencesService preferencesService,
         }
     }
 
+    /**
+     * Get the user's default file chooser directory
+     *
+     * @return The path to the directory
+     */
+    public static String getDefaultFileChooserDirectory() {
+        // Property "user.home" might be "difficult" on Windows
+        // See https://stackoverflow.com/a/586917/873282 for a longer discussion
+        // The proposed solution is to use Swing's FileSystemView
+        // See https://stackoverflow.com/a/32914568/873282
+        // As of 2022, System.getProperty("user.home") returns c:\Users\USERNAME on Windows 10, whereas
+        // the FileSystemView returns C:\Users\USERNAME\Documents, which is the "better" directory
+        return FileSystemView.getFileSystemView().getDefaultDirectory().getPath();
+    }
+
     // TODO: Move to OS.java
     public static NativeDesktop getNativeDesktop() {
         if (OS.WINDOWS) {

diff --git a/src/main/java/org/jabref/gui/exporter/ExportToClipboardAction.java b/src/main/java/org/jabref/gui/exporter/ExportToClipboardAction.java
@@ -103,7 +103,7 @@ private ExportResult exportToClipboard(Exporter exporter) throws Exception {
         // Set the global variable for this database's file directory before exporting,
         // so formatters can resolve linked files correctly.
         // (This is an ugly hack!)
-        preferences.storeFileDirforDatabase(stateManager.getActiveDatabase()
+        preferences.storeFileDirForDatabase(stateManager.getActiveDatabase()
                                                         .map(db -> db.getFileDirectories(preferences.getFilePreferences()))
                                                         .orElse(List.of(preferences.getFilePreferences().getWorkingDirectory())));
 

diff --git a/src/main/java/org/jabref/logic/layout/format/FileLinkPreferences.java b/src/main/java/org/jabref/logic/layout/format/FileLinkPreferences.java
@@ -17,6 +17,15 @@ public String getMainFileDirectory() {
         return mainFileDirectory;
     }
 
+    /**
+     * The following field is used as a global variable during the export of a database.
+     * By setting this field to the path of the database's default file directory, formatters
+     * that should resolve external file paths can access this field. This is an ugly hack
+     * to solve the problem of formatters not having access to any context except for the
+     * string to be formatted and possible formatter arguments.
+     *
+     * See also {@link org.jabref.preferences.JabRefPreferences#fileDirForDatabase}
+     */
     public List<Path> getFileDirForDatabase() {
         return fileDirForDatabase;
     }

diff --git a/src/main/java/org/jabref/model/database/BibDatabaseContext.java b/src/main/java/org/jabref/model/database/BibDatabaseContext.java
@@ -115,15 +115,20 @@ public boolean isBiblatexMode() {
 
     /**
      * Look up the directories set up for this database.
-     * There can be up to three directory definitions for these files: the database's
-     * metadata can specify a general directory and/or a user-specific directory, or the preferences can specify one.
+     * There can be up to four directories definitions for these files:
+     * <ol>
+     * <li>next to the .bib file.</li>
+     * <li>the preferences can specify a default one.</li>
+     * <li>the database's metadata can specify a general directory.</li>
+     * <li>the database's metadata can specify a user-specific directory.</li>
+     * </ol>
      * <p>
      * The settings are prioritized in the following order, and the first defined setting is used:
      * <ol>
      *     <li>user-specific metadata directory</li>
      *     <li>general metadata directory</li>
-     *     <li>preferences directory</li>
-     *     <li>BIB file directory</li>
+     *     <li>BIB file directory (if configured in the preferences AND none of the two above directories are configured)</li>
+     *     <li>preferences directory (if .bib file directory should not be used according to the preferences)</li>
      * </ol>
      *
      * @param preferences The fileDirectory preferences
@@ -140,7 +145,7 @@ public List<Path> getFileDirectories(FilePreferences preferences) {
                 .ifPresent(metaDataDirectory -> fileDirs.add(getFileDirectoryPath(metaDataDirectory)));
 
         // 3. BIB file directory or Main file directory
-        if (preferences.shouldStoreFilesRelativeToBibFile()) {
+        if (fileDirs.isEmpty() && preferences.shouldStoreFilesRelativeToBibFile()) {
             getDatabasePath().ifPresent(dbPath -> {
                 Path parentPath = dbPath.getParent();
                 if (parentPath == null) {

diff --git a/src/main/java/org/jabref/preferences/FilePreferences.java b/src/main/java/org/jabref/preferences/FilePreferences.java
@@ -52,7 +52,7 @@ public FilePreferences(String user,
         this.externalFileTypes.addAll(externalFileTypes);
     }
 
-    public String getUser() { // Read only
+    public String getUser() {
         return user.getValue();
     }
 

diff --git a/src/main/java/org/jabref/preferences/JabRefPreferences.java b/src/main/java/org/jabref/preferences/JabRefPreferences.java
@@ -410,6 +410,7 @@ public class JabRefPreferences implements PreferencesService {
     // to solve the problem of formatters not having access to any context except for the
     // string to be formatted and possible formatter arguments.
     public List<Path> fileDirForDatabase;
+
     private final Preferences prefs;
 
     /**
@@ -1119,12 +1120,12 @@ public void importPreferences(Path file) throws JabRefException {
     @Override
     public FileLinkPreferences getFileLinkPreferences() {
         return new FileLinkPreferences(
-                get(MAIN_FILE_DIRECTORY), // REALLY HERE?
+                getFilePreferences().mainFileDirectoryProperty().get(),
                 fileDirForDatabase);
     }
 
     @Override
-    public void storeFileDirforDatabase(List<Path> dirs) {
+    public void storeFileDirForDatabase(List<Path> dirs) {
         this.fileDirForDatabase = dirs;
     }
 
@@ -1475,7 +1476,7 @@ private void updateEntryEditorTabList() {
         List<String> tabNames = getSeries(CUSTOM_TAB_NAME);
         List<String> tabFields = getSeries(CUSTOM_TAB_FIELDS);
 
-        if (tabNames.isEmpty() || tabNames.size() != tabFields.size()) {
+        if (tabNames.isEmpty() || (tabNames.size() != tabFields.size())) {
             // Nothing set, so we use the default values
             tabNames = getSeries(CUSTOM_TAB_NAME + "_def");
             tabFields = getSeries(CUSTOM_TAB_FIELDS + "_def");
@@ -2207,6 +2208,20 @@ public FieldWriterPreferences getFieldWriterPreferences() {
                 getFieldContentParserPreferences());
     }
 
+    /**
+     * Ensures that the main file directory is a non-empty String.
+     * The directory is <emph>NOT</emph> created, because creation of the directory is the task of the respective methods.
+     *
+     * @param originalDirectory the directory as configured
+     */
+    private String determineMainFileDirectory(String originalDirectory) {
+        if (!originalDirectory.isEmpty()) {
+            // A non-empty directory is kept
+            return originalDirectory;
+        }
+        return Path.of(JabRefDesktop.getDefaultFileChooserDirectory(), "JabRef").toString();
+    }
+
     @Override
     public FilePreferences getFilePreferences() {
         if (Objects.nonNull(filePreferences)) {
@@ -2215,7 +2230,7 @@ public FilePreferences getFilePreferences() {
 
         filePreferences = new FilePreferences(
                 getInternalPreferences().getUser(),
-                get(MAIN_FILE_DIRECTORY),
+                determineMainFileDirectory(get(MAIN_FILE_DIRECTORY)),
                 getBoolean(STORE_RELATIVE_TO_BIB),
                 get(IMPORT_FILENAMEPATTERN),
                 get(IMPORT_FILEDIRPATTERN),
@@ -2794,7 +2809,7 @@ private Set<CustomImporter> getCustomImportFormats() {
                     importers.add(new CustomImporter(importerString.get(3), importerString.get(2)));
                 }
             } catch (Exception e) {
-                LOGGER.warn("Could not load " + importerString.get(0) + " from preferences. Will ignore.", e);
+                LOGGER.warn("Could not load {} from preferences. Will ignore.", importerString.get(0), e);
             }
         }
 

diff --git a/src/main/java/org/jabref/preferences/PreferencesService.java b/src/main/java/org/jabref/preferences/PreferencesService.java
@@ -221,7 +221,7 @@ public interface PreferencesService {
 
     FileLinkPreferences getFileLinkPreferences();
 
-    void storeFileDirforDatabase(List<Path> dirs);
+    void storeFileDirForDatabase(List<Path> dirs);
 
     //*************************************************************************************************************
     // Import/Export preferences

diff --git a/src/test/java/org/jabref/architecture/MainArchitectureTests.java b/src/test/java/org/jabref/architecture/MainArchitectureTests.java
@@ -17,7 +17,6 @@ class MainArchitectureTests {
     public static final String CLASS_ORG_JABREF_GLOBALS = "org.jabref.gui.Globals";
     private static final String PACKAGE_JAVAX_SWING = "javax.swing..";
     private static final String PACKAGE_JAVA_AWT = "java.awt..";
-    private static final String PACKAGE_JAVA_FX = "javafx..";
     private static final String PACKAGE_ORG_JABREF_GUI = "org.jabref.gui..";
     private static final String PACKAGE_ORG_JABREF_LOGIC = "org.jabref.logic..";
     private static final String PACKAGE_ORG_JABREF_MODEL = "org.jabref.model..";
@@ -33,34 +32,17 @@ public static void doNotUseApacheCommonsLang3(JavaClasses classes) {
     @ArchTest
     public static void doNotUseSwing(JavaClasses classes) {
         // This checks for all all Swing packages, but not the UndoManager
-        noClasses().should().accessClassesThat().resideInAnyPackage("javax.swing",
-                "javax.swing.border..",
-                "javax.swing.colorchooser..",
-                "javax.swing.event..",
-                "javax.swing.filechooser..",
-                "javax.swing.plaf..",
-                "javax.swing.table..",
-                "javax.swing.text..",
-                "javax.swing.tree.."
-        ).check(classes);
-    }
-
-    @ArchTest
-    public static void doNotUseJGoodies(JavaClasses classes) {
-        noClasses().should().accessClassesThat().resideInAPackage("com.jgoodies..")
-                   .check(classes);
-    }
-
-    @ArchTest
-    public static void doNotUseGlazedLists(JavaClasses classes) {
-        noClasses().should().accessClassesThat().resideInAPackage("ca.odell.glazedlists..")
-                   .check(classes);
-    }
-
-    @ArchTest
-    public static void doNotUseGlyphsDirectly(JavaClasses classes) {
-        noClasses().that().resideOutsideOfPackage("org.jabref.gui.icon")
-                   .should().accessClassesThat().resideInAnyPackage("de.jensd.fx.glyphs", "de.jensd.fx.glyphs.materialdesignicons")
+        noClasses().that().areNotAnnotatedWith(AllowedToUseSwing.class)
+                   .should().accessClassesThat()
+                   .resideInAnyPackage("javax.swing",
+                                       "javax.swing.border..",
+                                       "javax.swing.colorchooser..",
+                                       "javax.swing.event..",
+                                       "javax.swing.filechooser..",
+                                       "javax.swing.plaf..",
+                                       "javax.swing.table..",
+                                       "javax.swing.text..",
+                                       "javax.swing.tree..")
                    .check(classes);
     }
 
@@ -91,22 +73,22 @@ public static void doNotUsePaths(JavaClasses classes) {
     // Fails currently
     public static void respectLayeredArchitecture(JavaClasses classes) {
         layeredArchitecture()
-                .layer("Gui").definedBy(PACKAGE_ORG_JABREF_GUI)
-                .layer("Logic").definedBy(PACKAGE_ORG_JABREF_LOGIC)
-                .layer("Model").definedBy(PACKAGE_ORG_JABREF_MODEL)
-                .layer("Cli").definedBy(PACKAGE_ORG_JABREF_CLI)
-                .layer("Migrations").definedBy("org.jabref.migrations..") // TODO: Move to logic
-                .layer("Preferences").definedBy("org.jabref.preferences..")
-                .layer("Styletester").definedBy("org.jabref.styletester..")
-
-                .whereLayer("Gui").mayOnlyBeAccessedByLayers("Preferences", "Cli") // TODO: Remove preferences here
-                .whereLayer("Logic").mayOnlyBeAccessedByLayers("Gui", "Cli", "Model", "Migrations", "Preferences")
-                .whereLayer("Model").mayOnlyBeAccessedByLayers("Gui", "Logic", "Migrations", "Cli", "Preferences")
-                .whereLayer("Cli").mayNotBeAccessedByAnyLayer()
-                .whereLayer("Migrations").mayOnlyBeAccessedByLayers("Logic")
-                .whereLayer("Preferences").mayOnlyBeAccessedByLayers("Gui", "Logic", "Migrations", "Styletester", "Cli") // TODO: Remove logic here
-
-                .check(classes);
+                             .layer("Gui").definedBy(PACKAGE_ORG_JABREF_GUI)
+                             .layer("Logic").definedBy(PACKAGE_ORG_JABREF_LOGIC)
+                             .layer("Model").definedBy(PACKAGE_ORG_JABREF_MODEL)
+                             .layer("Cli").definedBy(PACKAGE_ORG_JABREF_CLI)
+                             .layer("Migrations").definedBy("org.jabref.migrations..") // TODO: Move to logic
+                             .layer("Preferences").definedBy("org.jabref.preferences..")
+                             .layer("Styletester").definedBy("org.jabref.styletester..")
+
+                             .whereLayer("Gui").mayOnlyBeAccessedByLayers("Preferences", "Cli") // TODO: Remove preferences here
+                             .whereLayer("Logic").mayOnlyBeAccessedByLayers("Gui", "Cli", "Model", "Migrations", "Preferences")
+                             .whereLayer("Model").mayOnlyBeAccessedByLayers("Gui", "Logic", "Migrations", "Cli", "Preferences")
+                             .whereLayer("Cli").mayNotBeAccessedByAnyLayer()
+                             .whereLayer("Migrations").mayOnlyBeAccessedByLayers("Logic")
+                             .whereLayer("Preferences").mayOnlyBeAccessedByLayers("Gui", "Logic", "Migrations", "Styletester", "Cli") // TODO: Remove logic here
+
+                             .check(classes);
     }
 
     @ArchTest

diff --git a/src/test/java/org/jabref/logic/cleanup/CleanupWorkerTest.java b/src/test/java/org/jabref/logic/cleanup/CleanupWorkerTest.java
@@ -1,6 +1,5 @@
 package org.jabref.logic.cleanup;
 
-import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -48,19 +47,21 @@ class CleanupWorkerTest {
     private final CleanupPreset emptyPreset = new CleanupPreset(EnumSet.noneOf(CleanupPreset.CleanupStep.class));
     private CleanupWorker worker;
 
-    // Ensure that the folder stays the same for all tests. By default @TempDir creates a new folder for each usage
+    // Ensure that the folder stays the same for all tests.
+    // By default, @TempDir creates a new folder for each usage
     private Path bibFolder;
 
+    // Currently, this directory is created below the bibFolder
+    private Path pdfPath;
+
     @BeforeEach
     void setUp(@TempDir Path bibFolder) throws IOException {
-
         this.bibFolder = bibFolder;
-        Path path = bibFolder.resolve("ARandomlyNamedFolder");
-        Files.createDirectory(path);
-        File pdfFolder = path.toFile();
+        pdfPath = bibFolder.resolve("ARandomlyNamedFolder");
+        Files.createDirectory(pdfPath);
 
         MetaData metaData = new MetaData();
-        metaData.setDefaultFileDirectory(pdfFolder.getAbsolutePath());
+        metaData.setDefaultFileDirectory(pdfPath.toAbsolutePath().toString());
         BibDatabaseContext context = new BibDatabaseContext(new BibDatabase(), metaData);
         Files.createFile(bibFolder.resolve("test.bib"));
         context.setDatabasePath(bibFolder.resolve("test.bib"));
@@ -239,7 +240,7 @@ void cleanupMoveFilesMovesFileFromSubfolder(@TempDir Path bibFolder) throws IOEx
     void cleanupRelativePathsConvertAbsoluteToRelativePath() throws IOException {
         CleanupPreset preset = new CleanupPreset(CleanupPreset.CleanupStep.MAKE_PATHS_RELATIVE);
 
-        Path path = bibFolder.resolve("AnotherRandomlyNamedFile");
+        Path path = pdfPath.resolve("AnotherRandomlyNamedFile");
         Files.createFile(path);
         BibEntry entry = new BibEntry();
         LinkedFile fileField = new LinkedFile("", path.toAbsolutePath(), "");
@@ -254,10 +255,10 @@ void cleanupRelativePathsConvertAbsoluteToRelativePath() throws IOException {
     void cleanupRenamePdfRenamesRelativeFile() throws IOException {
         CleanupPreset preset = new CleanupPreset(CleanupPreset.CleanupStep.RENAME_PDF);
 
-        Path path = bibFolder.resolve("AnotherRandomlyNamedFile.tmp");
+        Path path = pdfPath.resolve("AnotherRandomlyNamedFile.tmp");
         Files.createFile(path);
-        BibEntry entry = new BibEntry();
-        entry.setCitationKey("Toot");
+        BibEntry entry = new BibEntry()
+                .withCitationKey("Toot");
         LinkedFile fileField = new LinkedFile("", path.toAbsolutePath(), "");
         entry.setField(StandardField.FILE, FileFieldWriter.getStringRepresentation(fileField));