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

Fix handling of relative-file storage and auto linking #11492

Merged
merged 6 commits into from
Jul 16, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv

### Changed

- JabRef respects the [configuration for storing files relative to the .bib file](https://docs.jabref.org/finding-sorting-and-cleaning-entries/filelinks#directories-for-files) in more cases. [#11492](https://github.com/JabRef/jabref/pull/11492)

### Fixed

### Removed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import org.jabref.gui.externalfiletype.ExternalFileType;
import org.jabref.gui.externalfiletype.ExternalFileTypes;
Expand Down Expand Up @@ -52,6 +51,7 @@ public List<IOException> getFileExceptions() {
}

private static final Logger LOGGER = LoggerFactory.getLogger(AutoSetFileLinksUtil.class);

private final List<Path> directories;
private final AutoLinkPreferences autoLinkPreferences;
private final FilePreferences filePreferences;
Expand Down Expand Up @@ -106,7 +106,9 @@ public LinkFilesResult linkAssociatedFiles(List<BibEntry> entries, NamedCompound
public List<LinkedFile> findAssociatedNotLinkedFiles(BibEntry entry) throws IOException {
List<LinkedFile> linkedFiles = new ArrayList<>();

List<String> extensions = filePreferences.getExternalFileTypes().stream().map(ExternalFileType::getExtension).collect(Collectors.toList());
List<String> extensions = filePreferences.getExternalFileTypes().stream().map(ExternalFileType::getExtension).toList();

LOGGER.debug("Searching for extensions {} in directories {}", extensions, directories);

// Run the search operation
FileFinder fileFinder = FileFinders.constructFromConfiguration(autoLinkPreferences);
Expand Down Expand Up @@ -134,6 +136,7 @@ public List<LinkedFile> findAssociatedNotLinkedFiles(BibEntry entry) throws IOEx
Path relativeFilePath = FileUtil.relativize(foundFile, directories);
LinkedFile linkedFile = new LinkedFile("", relativeFilePath, strType);
linkedFiles.add(linkedFile);
LOGGER.debug("Found file {} for entry {}", linkedFile, entry.getCitationKey());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,15 @@ public void bindToEntry(BibEntry entry) {
super.bindToEntry(entry);

if ((entry != null) && preferences.getEntryEditorPreferences().autoLinkFilesEnabled()) {
LOGGER.debug("Auto-linking files for entry {}", entry);
BackgroundTask<List<LinkedFileViewModel>> findAssociatedNotLinkedFiles = BackgroundTask
.wrap(() -> findAssociatedNotLinkedFiles(entry))
.onSuccess(files::addAll);
.onSuccess(list -> {
if (!list.isEmpty()) {
LOGGER.debug("Found non-associated files:", list);
files.addAll(list);
}
});
taskExecutor.execute(findAssociatedNotLinkedFiles);
}
}
Expand Down Expand Up @@ -224,6 +230,7 @@ private List<LinkedFileViewModel> findAssociatedNotLinkedFiles(BibEntry entry) {
dialogService.showErrorDialogAndWait("Error accessing the file system", e);
}

LOGGER.trace("Found {} associated files for entry {}", result.size(), entry.getCitationKey());
return result;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@
import org.jabref.model.entry.BibEntry;
import org.jabref.model.strings.StringUtil;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

class CitationKeyBasedFileFinder implements FileFinder {

private static final Logger LOGGER = LoggerFactory.getLogger(CitationKeyBasedFileFinder.class);

private final boolean exactKeyOnly;

CitationKeyBasedFileFinder(boolean exactKeyOnly) {
Expand All @@ -36,6 +41,7 @@ public List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, Li

Optional<String> citeKeyOptional = entry.getCitationKey();
if (StringUtil.isBlank(citeKeyOptional)) {
LOGGER.debug("No citation key found in entry {}", entry);
return Collections.emptyList();
}
String citeKey = citeKeyOptional.get();
Expand All @@ -52,16 +58,18 @@ public List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, Li

// First, look for exact matches
if (nameWithoutExtension.equals(citeKey)) {
LOGGER.debug("Found exact match for key {} in file {}", citeKey, file);
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 && matches(name, citeKey)) {
LOGGER.debug("Found non-exact match for key {} in file {}", citeKey, file);
result.add(file);
}
}

return result.stream().sorted().collect(Collectors.toList());
return result.stream().sorted().toList();
}

private boolean matches(String filename, String citeKey) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/logic/util/io/FileFinders.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

public class FileFinders {
/**
* Creates a preconfigurated file finder based on the given AutoLink preferences.
* Creates a preconfigured file finder based on the given AutoLink preferences.
*/
public static FileFinder constructFromConfiguration(AutoLinkPreferences autoLinkPreferences) {
return switch (autoLinkPreferences.getCitationKeyDependency()) {
Expand Down
58 changes: 33 additions & 25 deletions src/main/java/org/jabref/model/database/BibDatabaseContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
import java.util.stream.Collectors;

import org.jabref.architecture.AllowedToUseLogic;
import org.jabref.gui.LibraryTab;
Expand Down Expand Up @@ -154,40 +153,45 @@ public boolean isStudy() {
* <li>user-specific metadata directory</li>
* <li>general metadata 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>
* <li>preferences directory (if .bib file directory should not be used according to the (global) preferences)</li>
* </ol>
*
* @param preferences The fileDirectory preferences
*/
public List<Path> getFileDirectories(FilePreferences preferences) {
List<Path> fileDirs = new ArrayList<>();
List<Path> fileDirs = new ArrayList<>(3);

// 1. Metadata user-specific directory
metaData.getUserFileDirectory(preferences.getUserAndHost())
.ifPresent(userFileDirectory -> fileDirs.add(getFileDirectoryPath(userFileDirectory)));
Optional<Path> userFileDirectory = metaData.getUserFileDirectory(preferences.getUserAndHost()).map(dir -> getFileDirectoryPath(dir));
userFileDirectory.ifPresent(fileDirs::add);

// 2. Metadata general directory
metaData.getDefaultFileDirectory()
.ifPresent(metaDataDirectory -> fileDirs.add(getFileDirectoryPath(metaDataDirectory)));
Optional<Path> generalFileDirectory = metaData.getDefaultFileDirectory().map(dir -> getFileDirectoryPath(dir));
generalFileDirectory.ifPresent(fileDirs::add);

// 3. BIB file directory or main file directory
// fileDirs.isEmpty in the case, 1) no user-specific file directory and 2) no general file directory is set
// (in the metadata of the bib file)
if (fileDirs.isEmpty() && preferences.shouldStoreFilesRelativeToBibFile()) {
// fileDirs.isEmpty() is true after these two if there are no directories set in the BIB file itself:
// 1) no user-specific file directory set (in the metadata of the bib file) and
// 2) no general file directory is set (in the metadata of the bib file)

// BIB file directory or main file directory (according to (global) preferences)
if (preferences.shouldStoreFilesRelativeToBibFile()) {
getDatabasePath().ifPresent(dbPath -> {
Path parentPath = dbPath.getParent();
if (parentPath == null) {
parentPath = Path.of(System.getProperty("user.dir"));
LOGGER.warn("Parent path of database file {} is null. Falling back to {}.", dbPath, parentPath);
}
Objects.requireNonNull(parentPath, "BibTeX database parent path is null");
fileDirs.add(parentPath);
Path absolutePath = parentPath.toAbsolutePath();
if (!fileDirs.contains(absolutePath)) {
fileDirs.add(absolutePath);
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe we can make a set out of this...

Copy link
Member

Choose a reason for hiding this comment

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

Not useful, we need to keep the order and we only have like 3 possible values

Copy link
Member

Choose a reason for hiding this comment

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

sortedset

Copy link
Member Author

@koppor koppor Jul 16, 2024

Choose a reason for hiding this comment

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

sortedset

We need to keep the order.

Sequenced Collection Set is the "right" data type. It checks for containment at each insert. With my approach, only one check needs to be made.

Moreover, we have at most two elements to check. We use an ArrayList where the "forEach" is faster than with a LinkedList.

Changing the return type to a sequencedcollection results in much code changes. Using it internally and then converting to a list results in more CPU cycles than "just" checking two elements.

I can rewrite to return SequencedCollection, write an ADR, add Java comments, link to this text or just leave it.

What should I do?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I just switched to LinkedHashSet. This also ensures that the second path is checked for uniqueness.

Copy link
Member

Choose a reason for hiding this comment

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

Was just an idea, the code just smelled like a set. no strong opinion on this.

});
} else {
// Main file directory
preferences.getMainFileDirectory().ifPresent(fileDirs::add);
preferences.getMainFileDirectory()
.filter(path -> !fileDirs.contains(path))
.ifPresent(fileDirs::add);
}

return fileDirs.stream().map(Path::toAbsolutePath).collect(Collectors.toList());
return fileDirs;
}

/**
Expand All @@ -201,15 +205,19 @@ public Optional<Path> getFirstExistingFileDir(FilePreferences preferences) {
.findFirst();
}

private Path getFileDirectoryPath(String directoryName) {
Path directory = Path.of(directoryName);
// If this directory is relative, we try to interpret it as relative to
// the file path of this BIB file:
Optional<Path> databaseFile = getDatabasePath();
if (!directory.isAbsolute() && databaseFile.isPresent()) {
return databaseFile.get().getParent().resolve(directory).normalize();
/**
* @return The absolute path for the given directory
*/
private Path getFileDirectoryPath(String directory) {
Path path = Path.of(directory);
if (path.isAbsolute()) {
return path;
}
return directory;

// If this path is relative, we try to interpret it as relative to the file path of this BIB file:
return getDatabasePath()
.map(databaseFile -> databaseFile.getParent().resolve(path).normalize().toAbsolutePath())
.orElse(path);
}

public DatabaseSynchronizer getDBMSSynchronizer() {
Expand Down
36 changes: 24 additions & 12 deletions src/test/java/org/jabref/model/database/BibDatabaseContextTest.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.jabref.model.database;

import java.nio.file.Path;
import java.util.Collections;
import java.util.List;
import java.util.Optional;

Expand Down Expand Up @@ -38,7 +37,7 @@ void setUp() {
void getFileDirectoriesWithEmptyDbParent() {
BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(Path.of("biblio.bib"));
assertEquals(Collections.singletonList(currentWorkingDir), database.getFileDirectories(fileDirPrefs));
assertEquals(List.of(currentWorkingDir), database.getFileDirectories(fileDirPrefs));
}

@Test
Expand All @@ -47,7 +46,7 @@ void getFileDirectoriesWithRelativeDbParent() {

BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(file);
assertEquals(Collections.singletonList(currentWorkingDir.resolve(file.getParent())), database.getFileDirectories(fileDirPrefs));
assertEquals(List.of(currentWorkingDir.resolve(file.getParent())), database.getFileDirectories(fileDirPrefs));
}

@Test
Expand All @@ -56,7 +55,16 @@ void getFileDirectoriesWithRelativeDottedDbParent() {

BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(file);
assertEquals(Collections.singletonList(currentWorkingDir.resolve(file.getParent())), database.getFileDirectories(fileDirPrefs));
assertEquals(List.of(currentWorkingDir.resolve(file.getParent())), database.getFileDirectories(fileDirPrefs));
}

@Test
void getFileDirectoriesWithDotAsDirectory() {
Path file = Path.of("biblio.bib");
BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(currentWorkingDir.resolve(file));
database.getMetaData().setDefaultFileDirectory(".");
assertEquals(List.of(currentWorkingDir), database.getFileDirectories(fileDirPrefs));
}

@Test
Expand All @@ -65,7 +73,7 @@ void getFileDirectoriesWithAbsoluteDbParent() {

BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(file);
assertEquals(Collections.singletonList(currentWorkingDir.resolve(file.getParent())), database.getFileDirectories(fileDirPrefs));
assertEquals(List.of(currentWorkingDir.resolve(file.getParent())), database.getFileDirectories(fileDirPrefs));
}

@Test
Expand All @@ -75,9 +83,11 @@ void getFileDirectoriesWithRelativeMetadata() {
BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(file);
database.getMetaData().setDefaultFileDirectory("../Literature");
// first directory is the metadata
// the bib file location is not included, because only the library-configured directories should be searched and the fallback should be the global directory.
assertEquals(List.of(Path.of("/absolute/Literature").toAbsolutePath()),
assertEquals(List.of(
// first directory originates from the metadata
Path.of("/absolute/Literature").toAbsolutePath(),
Path.of("/absolute/subdir").toAbsolutePath()
),
database.getFileDirectories(fileDirPrefs));
}

Expand All @@ -88,9 +98,11 @@ void getFileDirectoriesWithMetadata() {
BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(file);
database.getMetaData().setDefaultFileDirectory("Literature");
// first directory is the metadata
// the bib file location is not included, because only the library-configured directories should be searched and the fallback should be the global directory.
assertEquals(List.of(Path.of("/absolute/subdir/Literature").toAbsolutePath()),
assertEquals(List.of(
// first directory originates from the metadata
Path.of("/absolute/subdir/Literature").toAbsolutePath(),
Path.of("/absolute/subdir").toAbsolutePath()
),
database.getFileDirectories(fileDirPrefs));
}

Expand All @@ -102,7 +114,7 @@ void getUserFileDirectoryIfAllAreEmpty() {
when(fileDirPrefs.getMainFileDirectory()).thenReturn(Optional.of(userDirJabRef));
BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(Path.of("biblio.bib"));
assertEquals(Collections.singletonList(userDirJabRef), database.getFileDirectories(fileDirPrefs));
assertEquals(List.of(userDirJabRef), database.getFileDirectories(fileDirPrefs));
}

@Test
Expand Down