make file path relative when creatig entry from pdf

[Arshadpd]

Closes #11173

Mandatory checks

• 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.

[Siedlerchr]

You can just push your changes, the PR will be updated automatically. No need to open close

[koppor]

Code changes are strange. Many lines removed. No proper spacing between fields and class.

File was attached before. Why an additional setting of the file field?

[Arshadpd]

Thanks for the review @koppor ,

1. The spacing was actually reduced in efforts to remove the Checkstyle issues. It is rectified now.

2. I am trying to understand the comment on the "File" field, but if the concern is about the object " this.filePreferences = preferences.getFilePreferences(); want", I just wanted to let you know that I got the object name updated in one of the other commits to Jabref main branch, when I was syncing my fork. I also had to change my code to adapt the new object name for "FilePreferences".

[Siedlerchr]

That's stupid. We have a method which takes of all of those things. Try to understand the code first before doing any changes
see addResultToList and it takes a Path as argument.
You basically just have to call FileUtil.relative before adding it tot he list

[Arshadpd]

Hi @Siedlerchr ,

Thanks for your review. Please help me to clear my doubts
as per my understanding,

1. The addResultToList() methiod is just used for logging purpose, as we add the result of the operations and just use it to log at the end of the operation.

2. As per your suggestion, I am already using the FileUtil.relativize() method, this call is in the private relativize() method that has been added to the ImportHandler class. This code was approved by @koppor in the comment section of the issue, and we have created the pull request for the same.

If you suggest, instead of having a different private relativize() method in the class, directly implement it in the following way,

for (BibEntry entry : pdfEntriesInFile) {
entry.setField(StandardField.FILE, FileUtil.relativize(file));
}

I can implement it in this way as well. As of now we went ahead with the approach discussed and approved in the issue comments section. Please let us know your thoughts and feel free to correct me if my understanding is wrong.

Thanks,
Arshad

[koppor]

I tried to understand the code and the position seems to be OK.

The hard part is to deal with non-usual flows. Did you read in org.jabref.logic.importer.fileformat.PdfMergeMetadataImporter#importDatabase(java.nio.file.Path)? There, an exceptional case for mulitple linked files is written down.

Do

1. entry.getFiles()
2. Iterate through the list and make relativize each path
3. Call entry.setFiles(...)

In addition:

1. Move String relPath = relativize(file); down to here. The variable is not used else
2. Inline relativize here. It is a two line method. Thus, no need to create a new method.

---

Please note that at #11173 (comment) I had not the full content of your pull request and no IDE opened at my side. The updated comment results of an half an our investigation of JabRef's code.

[koppor]

Maybe, the more non-hacky fix would be to have create a relative path in org.jabref.logic.importer.fileformat.PdfMergeMetadataImporter#importDatabase(java.nio.file.Path) at the end at entry.addFile(new LinkedFile("", filePath, StandardFileType.PDF.getName()));

Maybe, the call can be like this:

var pdfImporterResult = contentImporter.importPDFContent(file, bibDatabaseContext, preferences.getFilePreferences());

But (much) more code changes will be needed, because importDatabase is @Override. Thus, the context and filePreferences need to be stored elsewhere etc.

[koppor]

@Arshadpd Any feedback on this?

[Siedlerchr]

Form my point of view the current implementaton is fine and we can leave the more complex one as a future task

[koppor]

The current implementation removes all links contained in the BibEntry objects contained. Thus, if more than one file link is contained, it is removed. I need to recheck, if this case really happens. Reading my comment from last week (#11409 (comment)), it might happen.

Moreover, I had code comments even if we agree that (potentially) destroying data is OK. These should be addressed in all cases. -- Maybe, it is OK to create a log output (warn) if file entries are overwritten.

We spend now at least one hour for reviewing and we should make use of the intellecutal propery we crafted there.