-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add auto-renaming of linked files on entry data change #13295
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
base: main
Are you sure you want to change the base?
Conversation
- Implemented a preference option under Linked files -> Linked file name conventions to enable auto-renaming of files when entry data changes (default: false). - Added functionality to listen for entry change events and rename files if the preference is enabled and the file name matches the defined pattern. - Ensured that no action is taken if the pattern is empty, as the file name would not match. - Considered scenarios where an entry has multiple files attached and handled them appropriately. - Added test cases to verify the new functionality.
jabgui/src/main/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChange.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChange.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTab.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTab.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTabViewModel.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTabViewModel.java
Show resolved
Hide resolved
jabgui/src/test/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChangeTest.java
Show resolved
Hide resolved
@@ -106,6 +109,18 @@ public void setStoreFilesRelativeToBibFile(boolean shouldStoreFilesRelativeToBib | |||
this.storeFilesRelativeToBibFile.set(shouldStoreFilesRelativeToBibFile); | |||
} | |||
|
|||
public boolean shouldAutoRenameFilesOnChange() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method should return an Optional instead of a primitive boolean to avoid returning null and to align with modern Java practices.
} | ||
|
||
private boolean relatesToFilePattern(String fileNamePattern, FieldChangedEvent event) { | ||
Set<String> extractedFields = new HashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'new HashSet<>()' is not optimal. Consider using 'Set.of()' for better readability and performance as per modern Java practices.
Ensure if the target file exists, do not use that name, use (Note to us: File should be kept even there are binary equal files. Otherwise, it gets complicated. E.g., if the same file is attached to another entry). |
I am trying implementing it in " ERROR: Could not read timestamp for /Users//Documents/JabRef/Gen28_ - Hands on Machine Learning with Scikit Learn, Keras, and TensorFlow.pdf: java.nio.file.NoSuchFileException: /Users//Documents/JabRef/Gen28_ - Hands on Machine Learning with Scikit Learn, Keras, and TensorFlow.pdf " |
@LoayGhreeb Do you have some hints maybe? |
I don't think the problem is with the indexing itself. The I guess this loop could be happening if the same file is linked to two different entries. Could that be the case? Please try to investigate further by adding some breakpoints to understand why, and where this loop is happening. Also, if possible, please push the commits that reproduce this issue, either here or in a new branch so we can take a proper look and help without guessing what's going on. |
jabgui/src/test/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChangeTest.java
Outdated
Show resolved
Hide resolved
* of duplicate markers. For example, if "paper (1).pdf" is already linked to the entry and is the correct format, | ||
* the method will return "paper (1).pdf" instead of generating "paper (2).pdf". | ||
*/ | ||
public static String generateUniqueFileName(Path targetDirectory, String proposedName, String fileAlreadyInUse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method accepts null parameter 'fileAlreadyInUse' which violates modern Java practices of avoiding null parameters. Should use Optional instead.
when using
Case 1: File Exists but is Not Linked to the Entry: A new unique file name is suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, the loop comes from org.jabref.logic.cleanup.RenamePdfCleanup#cleanup
you can add some LOGGER.trace
calls and adjust the log level (locally) to trace
as desribed at https://devdocs.jabref.org/code-howtos/logging.html
jabgui/src/test/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChangeTest.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChange.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChange.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/externalfiles/AutoRenameFileOnEntryChange.java
Outdated
Show resolved
Hide resolved
// create multiple entries | ||
List<String> fileNames = List.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is trivial and doesn't add any new information beyond what's clearly visible in the code. The list creation is self-explanatory from the variable name and content.
// Change author only | ||
entry.setField(StandardField.AUTHOR, "newKey"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment merely restates what the code does without providing additional context or reasoning. The code action is self-evident from the method call.
void noFileRenameByDefault() throws IOException { | ||
Files.createFile(tempDir.resolve("oldKey2081.pdf")); | ||
entry.setFiles(List.of(new LinkedFile("", "oldKey2081.pdf", "PDF"))); | ||
entry.setField(StandardField.AUTHOR, "newKey"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using setField instead of withField method for BibEntry modification. The code should use the wither pattern as per JabRef conventions.
@trag-bot didn't find any issues in the code! ✅✨ |
Closes #11316
This PR introduces an option to automatically rename linked files when the associated entry data is modified. The feature is configurable in Preferences under Linked files -> Linked file name conventions, with a default setting of false. It also includes handling for entries with multiple attached files and adds relevant test cases.
Steps to test
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)