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

Add feature relinking moved file #10954

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
- When pasting HTML into the abstract or a comment field, the hypertext is automatically converted to Markdown. [#10558](https://github.com/JabRef/jabref/issues/10558)
- We added the possibility to redownload files that had been present but are no longer in the specified location. [#10848](https://github.com/JabRef/jabref/issues/10848)
- We added the citation key pattern `[camelN]`. Equivalent to the first N words of the `[camel]` pattern.
- We added file relinking functionality after a file is moved when clicking the "Automatically set file links" button. Implementation based and improved upon work done in closed issue [#10526]. [#9798]
Copy link
Member

Choose a reason for hiding this comment

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

CHANGELOG is only for user-facing changes. You can just remove the last part.

Please add a full link to the issue as shown in the other lines of the CHANGELOG.md


### Changed

Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/jabref/cli/ArgumentProcessor.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.cli;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
Expand Down Expand Up @@ -188,7 +189,7 @@ private Optional<ParserResult> importFile(Path file, String importFormat) {
}
}

public void processArguments() {
public void processArguments() throws FileNotFoundException {
Copy link
Member

Choose a reason for hiding this comment

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

Please add JavaDoc @throws to state when this exception is thrown.

It is strange that upon startup of JabRef something is happening.

The issue itself has two headings "First implementation" and "Follow-up implementation". I cannot relate something happening on startup at each of the headings. Maybe you can explain?

uiCommands.clear();

if ((startupMode == Mode.INITIAL_START) && cli.isShowVersion()) {
Expand Down Expand Up @@ -711,7 +712,7 @@ private void resetPreferences(String value) {
}
}

private void automaticallySetFileLinks(List<ParserResult> loaded) {
private void automaticallySetFileLinks(List<ParserResult> loaded) throws FileNotFoundException {
for (ParserResult parserResult : loaded) {
BibDatabase database = parserResult.getDatabase();
LOGGER.info(Localization.lang("Automatically setting file links"));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.gui.externalfiles;

import java.io.FileNotFoundException;
import java.util.List;

import javax.swing.undo.UndoManager;
Expand Down Expand Up @@ -55,7 +56,7 @@ public void execute() {

Task<AutoSetFileLinksUtil.LinkFilesResult> linkFilesTask = new Task<>() {
@Override
protected AutoSetFileLinksUtil.LinkFilesResult call() {
protected AutoSetFileLinksUtil.LinkFilesResult call() throws FileNotFoundException {
return util.linkAssociatedFiles(entries, nc);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.jabref.gui.externalfiles;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.FileVisitOption;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
Expand Down Expand Up @@ -30,23 +32,25 @@

public class AutoSetFileLinksUtil {

record RelinkedResults(List<LinkedFile> relinkedFiles, List<String> exceptions) { }

public static class LinkFilesResult {
private final List<BibEntry> changedEntries = new ArrayList<>();
private final List<IOException> fileExceptions = new ArrayList<>();
private final List<String> fileExceptions = new ArrayList<String>();

protected void addBibEntry(BibEntry bibEntry) {
changedEntries.add(bibEntry);
}

protected void addFileException(IOException exception) {
protected void addFileException(String exception) {
fileExceptions.add(exception);
}

public List<BibEntry> getChangedEntries() {
return changedEntries;
}

public List<IOException> getFileExceptions() {
public List<String> getFileExceptions() {
Copy link
Member

Choose a reason for hiding this comment

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

Why reduction to weaker type? String should not be used if more finer-grainded data types are available.

See https://www.pearson.de/effective-java-9780134686073

return fileExceptions;
}
}
Expand All @@ -66,7 +70,7 @@ private AutoSetFileLinksUtil(List<Path> directories, FilePreferences filePrefere
this.filePreferences = filePreferences;
}

public LinkFilesResult linkAssociatedFiles(List<BibEntry> entries, NamedCompound ce) {
public LinkFilesResult linkAssociatedFiles(List<BibEntry> entries, NamedCompound ce) throws FileNotFoundException {
LinkFilesResult result = new LinkFilesResult();

for (BibEntry entry : entries) {
Expand All @@ -75,7 +79,7 @@ public LinkFilesResult linkAssociatedFiles(List<BibEntry> entries, NamedCompound
try {
linkedFiles = findAssociatedNotLinkedFiles(entry);
} catch (IOException e) {
result.addFileException(e);
result.addFileException(String.valueOf(e));
Copy link
Member

Choose a reason for hiding this comment

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

Can you please describe, why this change seems necessary?

LOGGER.error("Problem finding files", e);
}

Expand All @@ -94,10 +98,18 @@ public LinkFilesResult linkAssociatedFiles(List<BibEntry> entries, NamedCompound
entry.addFile(linkedFile);
});
}

if (changed) {
result.addBibEntry(entry);
}
// Run Relinking Process
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not say anything... You can extract a method to group statements together.

The algorithm itself should be explained

RelinkedResults relink = relinkingFiles(entry.getFiles());
entry.setFiles(relink.relinkedFiles);
if (!relink.relinkedFiles().isEmpty()) {
result.addBibEntry(entry);
}
for (String e : (relink.exceptions)) {
result.addFileException(e);
}
}
}
return result;
Expand Down Expand Up @@ -127,16 +139,60 @@ public List<LinkedFile> findAssociatedNotLinkedFiles(BibEntry entry) throws IOEx

if (!fileAlreadyLinked) {
Optional<ExternalFileType> type = FileUtil.getFileExtension(foundFile)
.map(extension -> ExternalFileTypes.getExternalFileTypeByExt(extension, filePreferences))
.orElse(Optional.of(new UnknownExternalFileType("")));
.map(extension -> ExternalFileTypes.getExternalFileTypeByExt(extension, filePreferences))
.orElse(Optional.of(new UnknownExternalFileType("")));

String strType = type.isPresent() ? type.get().getName() : "";
Path relativeFilePath = FileUtil.relativize(foundFile, directories);
LinkedFile linkedFile = new LinkedFile("", relativeFilePath, strType);
linkedFiles.add(linkedFile);
}
}

return linkedFiles;
}

public RelinkedResults relinkingFiles(List<LinkedFile> listlinked) throws FileNotFoundException {
Copy link
Member

Choose a reason for hiding this comment

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

method names are imperative normally. Rename to relinkFiles.

List<LinkedFile> changedFiles = new ArrayList<>();
List<String> exceptions = new ArrayList<>();

for (LinkedFile file : listlinked) {
Path path = Path.of(file.getLink());
if (!Files.exists(path)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is incomplete.

Use org.jabref.logic.util.io.FileUtil#find(org.jabref.model.database.BibDatabaseContext, java.lang.String, org.jabref.preferences.FilePreferences)

Reason: JabRef has multiple directories to store files. See https://docs.jabref.org/finding-sorting-and-cleaning-entries/filelinks#directories-for-files for an explanation.

Path filePath = Path.of(file.getLink());
// May need to put some limit since it can cause considerable performance problems.
String directoryPath = filePath.getParent().getParent().toString();

String fileNameString = filePath.getFileName().toString();

List<Path> fileLocations = searchFileInDirectoryAndSubdirectories(Path.of(directoryPath), fileNameString);
Copy link
Member

Choose a reason for hiding this comment

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

Wrong start. Search in databaseContext.getFileDirectories(filePreferences) only.

if (!fileLocations.isEmpty()) {
// File locations is a list but as stated in the link below, it is a rare case. But can be solved if time allows.
// https://github.com/JabRef/jabref/issues/9798#issuecomment-1524155132
file.setLink(fileLocations.get(0).toString());
changedFiles.add(file);
} else {
exceptions.add(fileNameString);
throw new FileNotFoundException();
Comment on lines +174 to +175
Copy link
Member

Choose a reason for hiding this comment

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

No exception here. Just leave the link as is.

Exceptions are huge derivations from the normal results.

See, if I have a library wtih 10000 PDFs, moved 20 files and 19 can be relinked, the whole processing should not be stopped if one file cannot be found.

It is OK to report a list of files which could not be relinked. If this is hard, then just log the non-found files.

}
}
}
return new RelinkedResults(changedFiles, exceptions);
}

public List<Path> searchFileInDirectoryAndSubdirectories(Path directory, String targetFileName) {
List<Path> paths = new ArrayList<>();
try {
Files.walk(directory, Integer.MAX_VALUE, FileVisitOption.FOLLOW_LINKS)
.filter(path -> Files.isRegularFile(path))
.filter(path -> path.getFileName().toString().equals(targetFileName))
.forEach(paths::add);
} catch (IOException e) {
// Handle any exceptions here
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be ChatGPT generated.

You can simply log the exception.

}
List<Path> output = new ArrayList<>();
for (Path p : paths) {
output.add(p);
}
return output;
Comment on lines +192 to +196
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Why not return paths?

}
}
4 changes: 4 additions & 0 deletions src/main/java/org/jabref/gui/remote/CLIMessageHandler.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.gui.remote;

import java.io.FileNotFoundException;
import java.util.Arrays;

import javafx.application.Platform;
Expand Down Expand Up @@ -42,6 +43,9 @@ public void handleCommandLineArguments(String[] message) {
Platform.runLater(() -> JabRefGUI.getMainFrame().handleUiCommands(argumentProcessor.getUiCommands()));
} catch (ParseException e) {
LOGGER.error("Error when parsing CLI args", e);
} catch (
FileNotFoundException e) {
throw new RuntimeException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.TreeSet;
Expand All @@ -26,7 +28,6 @@
import static org.mockito.Mockito.when;

public class AutoSetFileLinksUtilTest {

private final FilePreferences filePreferences = mock(FilePreferences.class);
private final AutoLinkPreferences autoLinkPrefs = new AutoLinkPreferences(
AutoLinkPreferences.CitationKeyDependency.START,
Expand All @@ -47,7 +48,7 @@ public void setUp(@TempDir Path folder) throws Exception {
}

@Test
public void findAssociatedNotLinkedFilesSuccess() throws Exception {
public void testFindAssociatedNotLinkedFilesSuccess() throws Exception {
when(databaseContext.getFileDirectories(any())).thenReturn(Collections.singletonList(path.getParent()));
List<LinkedFile> expected = Collections.singletonList(new LinkedFile("", Path.of("CiteKey.pdf"), "PDF"));
AutoSetFileLinksUtil util = new AutoSetFileLinksUtil(databaseContext, filePreferences, autoLinkPrefs);
Expand All @@ -56,10 +57,78 @@ public void findAssociatedNotLinkedFilesSuccess() throws Exception {
}

@Test
public void findAssociatedNotLinkedFilesForEmptySearchDir() throws Exception {
public void testFindAssociatedNotLinkedFilesForEmptySearchDir() throws Exception {
when(filePreferences.shouldStoreFilesRelativeToBibFile()).thenReturn(false);
AutoSetFileLinksUtil util = new AutoSetFileLinksUtil(databaseContext, filePreferences, autoLinkPrefs);
List<LinkedFile> actual = util.findAssociatedNotLinkedFiles(entry);
assertEquals(Collections.emptyList(), actual);
}

/* @BeforeEach
public void setUp(@TempDir Path folder) throws Exception {
A = folder.resolve("A");
Files.createDirectory(A);
B = folder.resolve("B");
Files.createDirectory(B);
path = A.resolve("CiteKey.pdf");
Files.createFile(path);
entry.setCitationKey("CiteKey");
when(filePreferences.getExternalFileTypes())
.thenReturn(FXCollections.observableSet(new TreeSet<>(ExternalFileTypes.getDefaultExternalFileTypes())));
} */

@Test
public void testFileLinksAfterMoving() throws Exception {
// Run "Automatically set file links" - check that the bib file was not modified
path = path.getParent();

Path A = path.resolve("A");
Files.createDirectory(A);
Path B = path.resolve("B");
Files.createDirectory(B);
Path filePath = A.resolve("Test.pdf");
Files.createFile(filePath);
entry.setCitationKey("Test");

LinkedFile linkedFile = new LinkedFile("desc", filePath, "PDF");
List<LinkedFile> listLinked = new ArrayList<>();
listLinked.add(linkedFile);
entry.setFiles(listLinked);

Path destination = B.resolve("Test.pdf");
Files.move(A.resolve("Test.pdf"), destination, StandardCopyOption.REPLACE_EXISTING);

AutoSetFileLinksUtil util = new AutoSetFileLinksUtil(databaseContext, filePreferences, autoLinkPrefs);
AutoSetFileLinksUtil.RelinkedResults results = util.relinkingFiles(entry.getFiles());
List<LinkedFile> list = results.relinkedFiles();
List<String> exceptions = results.exceptions();
assertEquals(Collections.emptyList(), exceptions);

// Change Entry to match required result and run method on bib
Copy link
Member

Choose a reason for hiding this comment

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

"required result" is too unspecifiy. Explain, what is the intention.

LinkedFile linkedDestFile = new LinkedFile("desc", B.resolve("Test.pdf"), "PDF");
List<LinkedFile> listLinked2 = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Having numbers as variable name suffix is bad style. Please craft self-descriptive variable names. E.g., "existingFiles" or something.

listLinked2.add(linkedDestFile);
entry.setFiles(listLinked2);
assertEquals(entry.getFiles(), list);

// After this point, tested different extension along with the case where document has multiple file references
Path filePath2 = B.resolve("Test1.txt");
Files.createFile(filePath2);
linkedFile = new LinkedFile("desc", filePath2, "TXT");
listLinked.add(linkedFile);
entry.setFiles(listLinked);

destination = A.resolve("Test1.txt");
Files.move(B.resolve("Test1.txt"), destination, StandardCopyOption.REPLACE_EXISTING);
util = new AutoSetFileLinksUtil(databaseContext, filePreferences, autoLinkPrefs);
results = util.relinkingFiles(entry.getFiles());
list = results.relinkedFiles();
exceptions = results.exceptions();

listLinked2.clear();
listLinked2.add(new LinkedFile("desc", B.resolve("Test.pdf"), "PDF"));
listLinked2.add(new LinkedFile("desc", A.resolve("Test1.txt"), "TXT"));

assertEquals(listLinked2, list);
}
}