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

Properly Copy Associated PDFs Between .bib Files #700

Closed
wants to merge 3 commits into from

Conversation

rioxddd
Copy link

@rioxddd rioxddd commented Oct 25, 2024

Describe the changes you have made here: what, why, ...
This pull request fixes the issue where associated PDF files were not being copied when entries were moved from one .bib file to another. Now, when copying entries, the linked PDFs are also transferred to the correct folder for the target .bib file.

Changes Made:
Updated copyEntry() to store the source .bib file context.
Modified pasteEntry() to copy linked PDFs to the target .bib file's directory.

Link the issue that will be closed: (#520)

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor
Copy link
Owner

koppor commented Oct 25, 2024

@rioxddd Do you need a review by a teammate here in this repository? If not, please submit a pull request to https://github.com/JabRef/jabref/pulls - because only PRs there go into the release.

Copy link
Owner

@koppor koppor left a comment

Choose a reason for hiding this comment

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

"Unfortunately", JabRef also heads methods for copying files. Please use them. They need to be adapted. I put hints in the code comments.

Comment on lines +967 to +979
List<Path> folderList = List.of(sourceBibFolder);
Optional<Path> sourcePath = linkedFile.findIn(folderList);
if(sourcePath.isPresent() && Files.exists(sourcePath.get())){
Path targetFolder = bibDatabaseContext.getMetaData().getDefaultFileDirectory().map(Paths::get).orElseThrow();
Path targetPath = targetFolder.resolve(sourcePath.get().getFileName());
try{
Files.copy(sourcePath.get(), targetPath, StandardCopyOption.REPLACE_EXISTING);
} catch (IOException e) {
LOGGER.error("failed to copy PDF file: {}", sourcePath.get(),e);
}
}else{
LOGGER.warn("did not find PDF: {}", linkedFile.getLink());
}
Copy link
Owner

Choose a reason for hiding this comment

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

No manual moving. Try to adapt public boolean copyOrMoveToDefaultDirectory(boolean shouldMove, boolean shouldRenameToFilenamePattern) throws IOException { from LinkedFileHandler.java.

I think, it can be used if user-specific file directory exists.

Proper code for that is to introduce FileDirectoriesInfo. (see below)

org.jabref.model.database.BibDatabaseContext#getFirstExistingFileDir needs to be made for FileDirectoriesInfo.

Then, the result can be checked if it is NOT mainFileDirectory.

Then, before you even start copy, check if the file is reachable from the NEW library (org.jabref.model.entry.LinkedFile#findIn(java.util.List<java.nio.file.Path>)). If yes, do not copy.

Handle all files, not just PDFs.

Introduced following in BibdatabaseContext:

    public record FileDirectoriesInfo(
            Optional<Path> mainFileDirectory,
            Optional<Path> librarySpecificDirectory,
            Optional<Path> userFileDirectory) {
    }

    public FileDirectoriesInfo getFileDirectoriesInfo(FilePreferences preferences) {
        Optional<Path> mainFileDirectory = metaData.getUserFileDirectory(preferences.getUserAndHost()).map(dir -> getFileDirectoryPath(dir));
       if (mainFileDirectory.isEmpty() && preferences.shouldStoreFilesRelativeToBibFile()) {
             // TODO
       }

        Optional<Path> librarySpecificDirectory = metaData.getLibrarySpecificFileDirectory()
                                                          .map(Path::of);

        Optional<Path> userFileDirectory = metaData.getUserFileDirectory(preferences.getUserAndHost())
                                                   .map(Path::of);

        return new FileDirectoriesInfo(mainFileDirectory, librarySpecificDirectory, userFileDirectory);
    }

Replace the TODO part with something like follows - you need to extract that part from org.jabref.model.database.BibDatabaseContext#getFileDirectories into a new method.

            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.toAbsolutePath());
            });


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants