Merge pull request #3509 from JabRef/fix3472

koppor · web-flow · commit f86e24daa0a5 · 2017-12-23T15:06:09.000+01:00
Fixes #3472: Files starting with BibTeX key of a similar entry are no longer found by mistake
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -56,6 +56,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
  - We fixed an issue where the same Java Look and Feel would be listed more than once in the Preferences. [#3391](https://github.com/JabRef/jabref/issues/3391)
  - We fixed an issue where errors in citation styles triggered an exception when opening the preferences dialog. [#3389](https://github.com/JabRef/jabref/issues/3389)
  - We fixed an issue where entries were displayed twice after insertion into a shared database. [#3164](https://github.com/JabRef/jabref/issues/3164)
+ - We improved the auto link algorithm so that files starting with a similar key are no longer found (e.g, `Einstein1902` vs `Einstein1902a`). [#3472](https://github.com/JabRef/jabref/issues/3472) 
  - We fixed an issue where special fields (such as `printed`) could not be cleared when syncing special fields via the keywords. [#3432](https://github.com/JabRef/jabref/issues/3432)
  - We fixed an issue where the tooltip of the global search bar showed html tags instead of formatting the text. [#3381](https://github.com/JabRef/jabref/issues/3381)
  - We fixed an issue where timestamps were not updated for changed entries. [#2810](https://github.com/JabRef/jabref/issues/2810)
diff --git a/src/main/java/org/jabref/gui/entryeditor/EntryEditorTab.java b/src/main/java/org/jabref/gui/entryeditor/EntryEditorTab.java
@@ -2,6 +2,7 @@
 
 import javafx.scene.control.Tab;
 
+import org.jabref.gui.util.DefaultTaskExecutor;
 import org.jabref.model.entry.BibEntry;
 
 public abstract class EntryEditorTab extends Tab {
@@ -32,7 +33,7 @@ protected void handleFocus() {
     public void notifyAboutFocus(BibEntry entry) {
         if (!entry.equals(currentEntry)) {
             currentEntry = entry;
-            bindToEntry(entry);
+            DefaultTaskExecutor.runInJavaFXThread(() -> bindToEntry(entry));
         }
         handleFocus();
     }
diff --git a/src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java b/src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java
@@ -27,22 +27,33 @@
 public class AutoSetFileLinksUtil {
 
     private static final Log LOGGER = LogFactory.getLog(AutoSetLinks.class);
+    private List<Path> directories;
+    private AutoLinkPreferences autoLinkPreferences;
+    private ExternalFileTypes externalFileTypes;
 
-    public List<LinkedFile> findassociatedNotLinkedFiles(BibEntry entry, BibDatabaseContext databaseContext, FileDirectoryPreferences fileDirPrefs, AutoLinkPreferences autoLinkPrefs, ExternalFileTypes externalFileTypes) {
+    public AutoSetFileLinksUtil(BibDatabaseContext databaseContext, FileDirectoryPreferences fileDirectoryPreferences, AutoLinkPreferences autoLinkPreferences, ExternalFileTypes externalFileTypes) {
+        this(databaseContext.getFileDirectoriesAsPaths(fileDirectoryPreferences), autoLinkPreferences, externalFileTypes);
+    }
+
+    public AutoSetFileLinksUtil(List<Path> directories, AutoLinkPreferences autoLinkPreferences, ExternalFileTypes externalFileTypes) {
+        this.directories = directories;
+        this.autoLinkPreferences = autoLinkPreferences;
+        this.externalFileTypes = externalFileTypes;
+    }
+
+    public List<LinkedFile> findAssociatedNotLinkedFiles(BibEntry entry) {
         List<LinkedFile> linkedFiles = new ArrayList<>();
 
-        List<Path> dirs = databaseContext.getFileDirectoriesAsPaths(fileDirPrefs);
         List<String> extensions = externalFileTypes.getExternalFileTypeSelection().stream().map(ExternalFileType::getExtension).collect(Collectors.toList());
 
-        // Run the search operation:
-        FileFinder fileFinder = FileFinders.constructFromConfiguration(autoLinkPrefs);
-        List<Path> result = fileFinder.findAssociatedFiles(entry, dirs, extensions);
-
-        // Iterate over the entries:
+        // Run the search operation
+        FileFinder fileFinder = FileFinders.constructFromConfiguration(autoLinkPreferences);
+        List<Path> result = fileFinder.findAssociatedFiles(entry, directories, extensions);
 
+        // Collect the found files that are not yet linked
         for (Path foundFile : result) {
-            boolean existingSameFile = entry.getFiles().stream()
-                    .map(file -> file.findIn(dirs))
+            boolean fileAlreadyLinked = entry.getFiles().stream()
+                    .map(file -> file.findIn(directories))
                     .anyMatch(file -> {
                         try {
                             return file.isPresent() && Files.isSameFile(file.get(), foundFile);
@@ -51,15 +62,13 @@ public List<LinkedFile> findassociatedNotLinkedFiles(BibEntry entry, BibDatabase
                         }
                         return false;
                     });
-            if (!existingSameFile) {
-
+            if (!fileAlreadyLinked) {
                 Optional<ExternalFileType> type = FileHelper.getFileExtension(foundFile)
                         .map(externalFileTypes::getExternalFileTypeByExt)
                         .orElse(Optional.of(new UnknownExternalFileType("")));
 
                 String strType = type.isPresent() ? type.get().getName() : "";
-                String relativeFilePath = FileUtil.shortenFileName(foundFile, dirs)
-                        .toString();
+                String relativeFilePath = FileUtil.shortenFileName(foundFile, directories).toString();
                 LinkedFile linkedFile = new LinkedFile("", relativeFilePath, strType);
                 linkedFiles.add(linkedFile);
             }
diff --git a/src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java b/src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java
@@ -82,11 +82,11 @@ public static Runnable autoSetLinks(final List<BibEntry> entries, final NamedCom
 
         Runnable r = () -> {
             boolean foundAny = false;
-            AutoSetFileLinksUtil util = new AutoSetFileLinksUtil();
+            AutoSetFileLinksUtil util = new AutoSetFileLinksUtil(databaseContext, Globals.prefs.getFileDirectoryPreferences(), Globals.prefs.getAutoLinkPreferences(), ExternalFileTypes.getInstance());
 
             for (BibEntry entry : entries) {
 
-                List<LinkedFile> linkedFiles = util.findassociatedNotLinkedFiles(entry, databaseContext, Globals.prefs.getFileDirectoryPreferences(), Globals.prefs.getAutoLinkPreferences(), ExternalFileTypes.getInstance());
+                List<LinkedFile> linkedFiles = util.findAssociatedNotLinkedFiles(entry);
 
                 if (ce != null) {
                     for (LinkedFile linkedFile : linkedFiles) {
diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java
@@ -158,9 +158,8 @@ public void bindToEntry(BibEntry entry) {
     private List<LinkedFileViewModel> findAssociatedNotLinkedFiles(BibEntry entry) {
         List<LinkedFileViewModel> result = new ArrayList<>();
 
-        AutoSetFileLinksUtil util = new AutoSetFileLinksUtil();
-        List<LinkedFile> linkedFiles = util.findassociatedNotLinkedFiles(entry, databaseContext, Globals.prefs.getFileDirectoryPreferences(), Globals.prefs.getAutoLinkPreferences(), ExternalFileTypes.getInstance());
-
+        AutoSetFileLinksUtil util = new AutoSetFileLinksUtil(databaseContext, Globals.prefs.getFileDirectoryPreferences(), Globals.prefs.getAutoLinkPreferences(), ExternalFileTypes.getInstance());
+        List<LinkedFile> linkedFiles = util.findAssociatedNotLinkedFiles(entry);
         for (LinkedFile linkedFile : linkedFiles) {
             LinkedFileViewModel newLinkedFile = new LinkedFileViewModel(linkedFile, entry, databaseContext);
             newLinkedFile.markAsAutomaticallyFound();
diff --git a/src/main/java/org/jabref/logic/bibtexkeypattern/BibtexKeyPatternUtil.java b/src/main/java/org/jabref/logic/bibtexkeypattern/BibtexKeyPatternUtil.java
@@ -19,10 +19,9 @@
  * This is the utility class of the LabelPattern package.
  */
 public class BibtexKeyPatternUtil extends BracketedPattern {
-    private static final Log LOGGER = LogFactory.getLog(BibtexKeyPatternUtil.class);
-
     // All single characters that we can use for extending a key to make it unique:
-    private static final String CHARS = "abcdefghijklmnopqrstuvwxyz";
+    public static final String CHARS = "abcdefghijklmnopqrstuvwxyz";
+    private static final Log LOGGER = LogFactory.getLog(BibtexKeyPatternUtil.class);
 
     private BibtexKeyPatternUtil() {
     }
diff --git a/src/main/java/org/jabref/logic/net/URLDownload.java b/src/main/java/org/jabref/logic/net/URLDownload.java
@@ -260,7 +260,7 @@ public Path toTemporaryFile() throws IOException {
 
         // Take everything after the last '/' as name + extension
         String fileNameWithExtension = sourcePath.substring(sourcePath.lastIndexOf('/') + 1);
-        String fileName = FileUtil.getFileName(fileNameWithExtension);
+        String fileName = FileUtil.getBaseName(fileNameWithExtension);
         String extension = "." + FileHelper.getFileExtension(fileNameWithExtension).orElse("tmp");
 
         // Create temporary file and download to it
diff --git a/src/main/java/org/jabref/logic/util/io/CiteKeyBasedFileFinder.java b/src/main/java/org/jabref/logic/util/io/CiteKeyBasedFileFinder.java
@@ -5,18 +5,19 @@
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.BiPredicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import org.jabref.logic.bibtexkeypattern.BibtexKeyPatternUtil;
 import org.jabref.model.entry.BibEntry;
+import org.jabref.model.strings.StringUtil;
 import org.jabref.model.util.FileHelper;
 
 import org.apache.commons.logging.Log;
@@ -32,54 +33,55 @@ class CiteKeyBasedFileFinder implements FileFinder {
     }
 
     @Override
-    public Map<BibEntry, List<Path>> findAssociatedFiles(List<BibEntry> entries, List<Path> directories, List<String> extensions) {
+    public List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, List<String> extensions) {
         Objects.requireNonNull(directories);
-        Objects.requireNonNull(entries);
+        Objects.requireNonNull(entry);
 
-        Map<BibEntry, List<Path>> result = new HashMap<>();
+        Optional<String> citeKeyOptional = entry.getCiteKeyOptional();
+        if (StringUtil.isBlank(citeKeyOptional)) {
+            return Collections.emptyList();
+        }
+        String citeKey = citeKeyOptional.get();
+
+        List<Path> result = new ArrayList<>();
 
         // First scan directories
         Set<Path> filesWithExtension = findFilesByExtension(directories, extensions);
 
-        // Initialize Result-Set
-        for (BibEntry entry : entries) {
-            result.put(entry, new ArrayList<>());
-        }
-
         // Now look for keys
-        nextFile:
         for (Path file : filesWithExtension) {
             String name = file.getFileName().toString();
-            int dot = name.lastIndexOf('.');
-            // First, look for exact matches:
-            for (BibEntry entry : entries) {
-                Optional<String> citeKey = entry.getCiteKeyOptional();
-                if ((citeKey.isPresent()) && !citeKey.get().isEmpty() && (dot > 0)
-                        && name.substring(0, dot).equals(citeKey.get())) {
-                    result.get(entry).add(file);
-                    continue nextFile;
-                }
+            String nameWithoutExtension = FileUtil.getBaseName(name);
+
+            // First, look for exact matches
+            if (nameWithoutExtension.equals(citeKey)) {
+                result.add(file);
+                continue;
             }
-            // If we get here, we did not find any exact matches. If non-exact
-            // matches are allowed, try to find one:
-            if (!exactKeyOnly) {
-                for (BibEntry entry : entries) {
-                    Optional<String> citeKey = entry.getCiteKeyOptional();
-                    if ((citeKey.isPresent()) && !citeKey.get().isEmpty() && name.startsWith(citeKey.get())) {
-                        result.get(entry).add(file);
-                        continue nextFile;
-                    }
-                }
+            // If we get here, we did not find any exact matches. If non-exact matches are allowed, try to find one
+            if (!exactKeyOnly && matches(name, citeKey)) {
+                    result.add(file);
             }
         }
 
-        return result;
+        return result.stream().sorted().collect(Collectors.toList());
+    }
+
+    private boolean matches(String filename, String citeKey) {
+        boolean startsWithKey = filename.startsWith(citeKey);
+        if (startsWithKey) {
+            // The file name starts with the key, that's already a good start
+            // However, we do not want to match "JabRefa" for "JabRef" since this is probably a file belonging to another entry published in the same time / same name
+            char charAfterKey = filename.charAt(citeKey.length());
+            return !BibtexKeyPatternUtil.CHARS.contains(Character.toString(charAfterKey));
+        }
+        return false;
     }
 
     /**
      * Returns a list of all files in the given directories which have one of the given extension.
      */
-    public Set<Path> findFilesByExtension(List<Path> directories, List<String> extensions) {
+    private Set<Path> findFilesByExtension(List<Path> directories, List<String> extensions) {
         Objects.requireNonNull(extensions, "Extensions must not be null!");
 
         BiPredicate<Path, BasicFileAttributes> isFileWithCorrectExtension = (path, attributes) ->
diff --git a/src/main/java/org/jabref/logic/util/io/FileFinder.java b/src/main/java/org/jabref/logic/util/io/FileFinder.java
@@ -1,9 +1,7 @@
 package org.jabref.logic.util.io;
 
 import java.nio.file.Path;
-import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 
 import org.jabref.model.entry.BibEntry;
 
@@ -13,14 +11,9 @@ public interface FileFinder {
      * Finds all files in the given directories that are probably associated with the given entries and have one of the
      * passed extensions.
      *
-     * @param entries     The entries to search for.
+     * @param entry       The entry to search files for.
      * @param directories The root directories to search.
      * @param extensions  The extensions that are acceptable.
      */
-    Map<BibEntry, List<Path>> findAssociatedFiles(List<BibEntry> entries, List<Path> directories, List<String> extensions);
-
-    default List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, List<String> extensions) {
-        Map<BibEntry, List<Path>> associatedFiles = findAssociatedFiles(Collections.singletonList(entry), directories, extensions);
-        return associatedFiles.getOrDefault(entry, Collections.emptyList());
-    }
+    List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, List<String> extensions);
 }
diff --git a/src/main/java/org/jabref/logic/util/io/FileUtil.java b/src/main/java/org/jabref/logic/util/io/FileUtil.java
@@ -27,6 +27,7 @@
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.util.OptionalUtil;
 
+import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
@@ -65,13 +66,8 @@ public static Optional<String> getFileExtension(File file) {
     /**
      * Returns the name part of a file name (i.e., everything in front of last ".").
      */
-    public static String getFileName(String fileNameWithExtension) {
-        int dotPosition = fileNameWithExtension.lastIndexOf('.');
-        if (dotPosition >= 0) {
-            return fileNameWithExtension.substring(0, dotPosition);
-        } else {
-            return fileNameWithExtension;
-        }
+    public static String getBaseName(String fileNameWithExtension) {
+        return FilenameUtils.getBaseName(fileNameWithExtension);
     }
 
     /**
@@ -80,7 +76,7 @@ public static String getFileName(String fileNameWithExtension) {
      * Currently, only the length is restricted to 255 chars, see MAXIMUM_FILE_NAME_LENGTH.
      */
     public static String getValidFileName(String fileName) {
-        String nameWithoutExtension = getFileName(fileName);
+        String nameWithoutExtension = getBaseName(fileName);
 
         if (nameWithoutExtension.length() > MAXIMUM_FILE_NAME_LENGTH) {
             Optional<String> extension = getFileExtension(fileName);
diff --git a/src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java b/src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java
@@ -6,9 +6,7 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
@@ -64,15 +62,6 @@ public static String expandBrackets(String bracketString, BibEntry entry, BibDat
         return expandedStringBuffer.toString();
     }
 
-    @Override
-    public Map<BibEntry, List<Path>> findAssociatedFiles(List<BibEntry> entries, List<Path> directories, List<String> extensions) {
-        Map<BibEntry, List<Path>> res = new HashMap<>();
-        for (BibEntry entry : entries) {
-            res.put(entry, findFiles(entry, extensions, directories));
-        }
-        return res;
-    }
-
     /**
      * Method for searching for files using regexp. A list of extensions and directories can be
      * given.
@@ -81,7 +70,8 @@ public Map<BibEntry, List<Path>> findAssociatedFiles(List<BibEntry> entries, Lis
      * @param directories The root directories to search.
      * @return A list of files paths matching the given criteria.
      */
-    private List<Path> findFiles(BibEntry entry, List<String> extensions, List<Path> directories) {
+    @Override
+    public List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, List<String> extensions) {
         String extensionRegExp = '(' + String.join("|", extensions) + ')';
         return findFile(entry, directories, extensionRegExp);
     }
diff --git a/src/test/java/org/jabref/BibtexTestData.java b/src/test/java/org/jabref/BibtexTestData.java
@@ -16,7 +16,7 @@ public static BibEntry getBibtexEntry(ImportFormatPreferences importFormatPrefer
         return database.getEntryByKey("HipKro03").get();
     }
 
-    public static BibDatabase getBibtexDatabase(ImportFormatPreferences importFormatPreferences) throws IOException {
+    private static BibDatabase getBibtexDatabase(ImportFormatPreferences importFormatPreferences) throws IOException {
         String article = "@ARTICLE{HipKro03,\n" + "  author = {Eric von Hippel and Georg von Krogh},\n"
                         + "  title = {Open Source Software and the \"Private-Collective\" Innovation Model: Issues for Organization Science},\n"
                         + "  journal = {Organization Science},\n" + "  year = {2003},\n" + "  volume = {14},\n"
diff --git a/src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java b/src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java
@@ -35,8 +35,7 @@ public void setUp() throws Exception {
         entry.setCiteKey("CiteKey");
         folder.newFile("CiteKey.pdf");
         when(databaseContext.getFileDirectoriesAsPaths(any())).thenReturn(Collections.singletonList(folder.getRoot().toPath()));
-        when(externalFileTypes.getExternalFileTypeSelection()).thenReturn(new TreeSet<>(externalFileTypes.getDefaultExternalFileTypes()));
-
+        when(externalFileTypes.getExternalFileTypeSelection()).thenReturn(new TreeSet<>(ExternalFileTypes.getDefaultExternalFileTypes()));
     }
 
     @Test
@@ -45,8 +44,8 @@ public void test() {
 
         List<LinkedFile> expected = Collections.singletonList(new LinkedFile("", "CiteKey.pdf", ""));
 
-        AutoSetFileLinksUtil util = new AutoSetFileLinksUtil();
-        List<LinkedFile> actual = util.findassociatedNotLinkedFiles(entry, databaseContext, fileDirPrefs, autoLinkPrefs, externalFileTypes);
+        AutoSetFileLinksUtil util = new AutoSetFileLinksUtil(databaseContext, fileDirPrefs, autoLinkPrefs, externalFileTypes);
+        List<LinkedFile> actual = util.findAssociatedNotLinkedFiles(entry);
         assertEquals(expected, actual);
     }
 
diff --git a/src/test/java/org/jabref/logic/util/io/CiteKeyBasedFileFinderTest.java b/src/test/java/org/jabref/logic/util/io/CiteKeyBasedFileFinderTest.java
diff --git a/src/test/java/org/jabref/logic/util/io/FileUtilTest.java b/src/test/java/org/jabref/logic/util/io/FileUtilTest.java

Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,7 @@`
`2`	`2`
`3`	`3`	`import javafx.scene.control.Tab;`
`4`	`4`
	`5`	`+import org.jabref.gui.util.DefaultTaskExecutor;`
`5`	`6`	`import org.jabref.model.entry.BibEntry;`
`6`	`7`
`7`	`8`	`public abstract class EntryEditorTab extends Tab {`
`@@ -32,7 +33,7 @@ protected void handleFocus() {`
`32`	`33`	`public void notifyAboutFocus(BibEntry entry) {`
`33`	`34`	`if (!entry.equals(currentEntry)) {`
`34`	`35`	`currentEntry = entry;`
`35`		`- bindToEntry(entry);`
	`36`	`+ DefaultTaskExecutor.runInJavaFXThread(() -> bindToEntry(entry));`
`36`	`37`	`}`
`37`	`38`	`handleFocus();`
`38`	`39`	`}`