Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3472: Files starting with BibTeX key of a similar entry are no longer found by mistake #3509

Merged
merged 10 commits into from
Dec 23, 2017
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/jabref/gui/entryeditor/EntryEditorTab.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against creating a second constructor. It was my initial idea in #3368 and guess who supposed the current alternative? 😆
See your comment #3368 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean. The linked comment is about the list of entries and not about how the AutoSetFileLinksUtil gets the things needed (like directories and preferences). I think, the current setup makes sense: on initialization, specify everything the auto set functionality needs to know. Why don't you like these constructors?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I could not see all code in the line on my mobile, so I was a b9it confused. Forget about my remark,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem!

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);
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/logic/net/URLDownload.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
66 changes: 34 additions & 32 deletions src/main/java/org/jabref/logic/util/io/CiteKeyBasedFileFinder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) ->
Expand Down
11 changes: 2 additions & 9 deletions src/main/java/org/jabref/logic/util/io/FileFinder.java
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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);
}
12 changes: 4 additions & 8 deletions src/main/java/org/jabref/logic/util/io/FileUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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);
Expand Down
14 changes: 2 additions & 12 deletions src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/jabref/BibtexTestData.java
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}

Expand Down
Loading