If entry data is changed, file name should also be changed. #12024

AdiAr11 · 2024-10-19T07:00:43Z

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.

UI change:

…it would be saved for subsequent runs

…ogic

AdiAr11 · 2024-10-22T08:26:14Z

Hello, @HoussemNasri, good evening. I have implemented the changes. Please let me know if any changes are needed.
Thank you!

src/main/java/org/jabref/gui/externalfiles/LinkedFileAutoRenamer.java

…r-issue-11316

HoussemNasri · 2024-10-23T10:30:58Z

src/main/java/org/jabref/logic/util/io/FileUtil.java

@@ -524,4 +524,66 @@ public static boolean detectBadFileName(String fileName) {
    public static boolean isCharLegal(char c) {
        return Arrays.binarySearch(ILLEGAL_CHARS, c) < 0;
    }
+
+    public static boolean renameLinkedFile(LinkedFile linkedFile, BibEntry entry, String fileNamePattern, List<Path> fileDirectories, FilePreferences filePreferences, BibDatabaseContext bibDatabaseContext) {


Is this method duplicated from somewhere else in the codebase? I'm sure we have already implemented this.

We have another implementation at LinkedFileHandler that differs slightly from this one. Can we refactor this to remove the duplication? Perhaps we can combine both implementations and change dependents of LinkedFileHandler and LinkedFileAutoRenamer to depend on the same implementation.

Did this as well!

HoussemNasri · 2024-10-23T10:35:53Z

src/main/java/org/jabref/gui/externalfiles/LinkedFileAutoRenamer.java

+
+            if (renamed) {
+                UiTaskExecutor.runInJavaFXThread(() -> {
+                    entry.setFiles(linkedFiles);


I'm uncertain whether we need to do this for every linked file. The typical user would have a handful or linked files per entry, but you never now, people can get very creative, so if it's possible to minimize calls to setFiles, we should do it.

Hi @HoussemNasri, do you have any suggestion on what should be done for this?

Please checkout the comment below.

HoussemNasri · 2024-10-23T10:49:56Z

src/main/java/org/jabref/gui/externalfiles/FileNameParser.java

+
+public class FileNameParser {
+
+    private static final Pattern FIELD_PATTERN = Pattern.compile("\\[([^\\]]+)\\]");


I'm pretty sure this is already implemented. Just to clarify, it is possible to change the linked file name to follow a pattern by right-clicking on the linked file and choosing 'rename file to a defined pattern'.

Changed the code to use BracketedPattern instead and deleted this class!!

HoussemNasri

The feature works great! Kudos on the fantastic code quality. Just a few notes on code duplication.

AdiAr11 · 2024-10-24T01:06:06Z

@HoussemNasri Thank you so much for the feedback. I will try my best to make these changes as soon as possible!

Merge branch 'fix-for-issue-11316' of https://github.com/AdiAr11/jabref into fix-for-issue-11316

AdiAr11 · 2024-10-26T14:22:06Z

Hi @HoussemNasri, i have made the changes you requested. Hope it is fine now.
Thank you!

Merge branch 'main' into fix-for-issue-11316

HoussemNasri · 2024-10-27T15:15:32Z

src/main/java/org/jabref/gui/externalfiles/LinkedFileAutoRenamer.java

+                boolean renamed = FileUtil.renameLinkedFileToName(linkedFile, entry, newFileName, false, bibDatabaseContext, fileDirectories, preferences.getFilePreferences());
+
+                if (renamed) {
+                    UiTaskExecutor.runInJavaFXThread(() -> {
+                        entry.setFiles(linkedFiles);
+                        LOGGER.debug("Updated entry files after renaming.");
+                    });
+                }
+            } catch (IOException e) {
+                LOGGER.error("Failed to rename file '{}' to '{}'", linkedFile.getLink(), fileNamePattern, e);
+            }
+        }


Suggested change

boolean renamed = FileUtil.renameLinkedFileToName(linkedFile, entry, newFileName, false, bibDatabaseContext, fileDirectories, preferences.getFilePreferences());

if (renamed) {

UiTaskExecutor.runInJavaFXThread(() -> {

entry.setFiles(linkedFiles);

LOGGER.debug("Updated entry files after renaming.");

});

}

} catch (IOException e) {

LOGGER.error("Failed to rename file '{}' to '{}'", linkedFile.getLink(), fileNamePattern, e);

}

}

FileUtil.renameLinkedFileToName(linkedFile, entry, newFileName, false, bibDatabaseContext, fileDirectories, preferences.getFilePreferences());

}

} catch (IOException e) {

LOGGER.error("Failed to rename file '{}' to '{}'", linkedFile.getLink(), fileNamePattern, e);

}

}

entry.setFiles(linkedFiles);

LOGGER.debug("Updated entry files after renaming.");

Scheduling the linked files update on the JavaFX thread will lead to glitches if you change the fields fast enough. Also, we only need to call setFiles once we have renamed all the linked files that needed renaming.

HoussemNasri · 2024-10-27T15:22:36Z

src/main/java/org/jabref/gui/externalfiles/LinkedFileAutoRenamer.java

+                String newFileName = FileUtil.createFileNameFromPattern(bibDatabaseContext.getDatabase(), entry, fileNamePattern);
+                LOGGER.debug("Generated new file name: '{}'", newFileName);
+
+                boolean renamed = FileUtil.renameLinkedFileToName(linkedFile, entry, newFileName, false, bibDatabaseContext, fileDirectories, preferences.getFilePreferences());


Passing the boolean false like this is against best practices, please define a constant OVERWRITE_EXISTING_FILE = true, and pass !OVERWRITE_EXISTING_FILE to the method.

HoussemNasri · 2024-10-27T16:03:23Z

src/main/java/org/jabref/logic/util/io/FileUtil.java

@@ -588,4 +588,73 @@ public static String shortenFileName(String fileName, Integer maxLength) {
    public static boolean isCharLegal(char c) {
        return Arrays.binarySearch(ILLEGAL_CHARS, c) < 0;
    }
+
+    public static boolean renameLinkedFileToName(LinkedFile linkedFile,


This is too much logic to go untested. Please add unit tests for it. You can refer to our testing guide in the devdocs, and reviewing other test classes may also provide useful insights.

You can call the already existing RenamePdfCleanup which does exactly this

AdiAr11 · 2024-10-28T06:44:49Z

Noted! Will make these changes and write the test cases. Thank you!

koppor · 2024-12-09T20:52:09Z

Closing this issue due to inactivity 💤

We discussed in the DevCall. The implementation should make use of the existing renaming cleanup.

implemented the ui for this issue in relevant .fxml file

96c0fff

HoussemNasri changed the title ~~Started working on task- If entry data is changed, file name should also be changed.~~ [WIP] If entry data is changed, file name should also be changed. Oct 19, 2024

AdiAr11 and others added 8 commits October 20, 2024 22:47

persisting or saving the option selected.Like if checkbox is ticked, …

107d416

…it would be saved for subsequent runs

created a new class to handle the event listening and file renaming l…

07bc89c

…ogic

more progress to register when change occurs in required fields

b8a2eb4

automatic renaming of file based on pattern done. But has some bugs

510e5eb

Performing as expected!!!!

5863b83

code refactoring

1bd7381

change added in CHANGELOG.md file

5191607

Merge branch 'main' into fix-for-issue-11316

157e283

AdiAr11 marked this pull request as ready for review October 22, 2024 08:25

HoussemNasri reviewed Oct 22, 2024

View reviewed changes

src/main/java/org/jabref/gui/externalfiles/LinkedFileAutoRenamer.java Outdated Show resolved Hide resolved

HoussemNasri changed the title ~~[WIP] If entry data is changed, file name should also be changed.~~ If entry data is changed, file name should also be changed. Oct 22, 2024

AdiAr11 and others added 6 commits October 22, 2024 19:51

made changes as suggested by reviewer

7e41328

Merge branch 'main' into fix-for-issue-11316

34fa786

removed a comment

bac3554

Merge remote-tracking branch 'origin/fix-for-issue-11316' into fix-fo…

4d63533

…r-issue-11316

code cleanup

059283c

Merge branch 'main' into fix-for-issue-11316

cddf31d

HoussemNasri reviewed Oct 23, 2024

View reviewed changes

HoussemNasri requested changes Oct 23, 2024

View reviewed changes

AdiAr11 and others added 5 commits October 24, 2024 11:56

Merge branch 'main' into fix-for-issue-11316

d6b5cc9

Merge branch 'main' into fix-for-issue-11316

330af7b

refactoring code to remove redundancy as suggested by reviewer

1e5f278

Merge branch 'main' into fix-for-issue-11316

8414f71

merging with main

3fac142

Merge branch 'fix-for-issue-11316' of https://github.com/AdiAr11/jabref into fix-for-issue-11316

refactoring code to remove redundancy as suggested by reviewer

95396d6

merging with main

53804ae

Merge branch 'main' into fix-for-issue-11316

HoussemNasri reviewed Oct 27, 2024

View reviewed changes

koppor added the status: changes required Pull requests that are not yet complete label Oct 31, 2024

koppor closed this Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If entry data is changed, file name should also be changed. #12024

If entry data is changed, file name should also be changed. #12024

AdiAr11 commented Oct 19, 2024 •

edited by koppor

Loading

AdiAr11 commented Oct 22, 2024

HoussemNasri Oct 23, 2024

HoussemNasri Oct 23, 2024

AdiAr11 Oct 26, 2024

HoussemNasri Oct 23, 2024

AdiAr11 Oct 26, 2024

HoussemNasri Oct 27, 2024

HoussemNasri Oct 23, 2024

AdiAr11 Oct 26, 2024

HoussemNasri left a comment

AdiAr11 commented Oct 24, 2024

AdiAr11 commented Oct 26, 2024

HoussemNasri Oct 27, 2024

HoussemNasri Oct 27, 2024 •

edited

Loading

HoussemNasri Oct 27, 2024 •

edited

Loading

Siedlerchr Dec 9, 2024

AdiAr11 commented Oct 28, 2024

koppor commented Dec 9, 2024


		public class FileNameParser {

		private static final Pattern FIELD_PATTERN = Pattern.compile("\\[([^\\]]+)\\]");

If entry data is changed, file name should also be changed. #12024

If entry data is changed, file name should also be changed. #12024

Conversation

AdiAr11 commented Oct 19, 2024 • edited by koppor Loading

Mandatory checks

AdiAr11 commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoussemNasri left a comment

Choose a reason for hiding this comment

AdiAr11 commented Oct 24, 2024

AdiAr11 commented Oct 26, 2024

Choose a reason for hiding this comment

HoussemNasri Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

HoussemNasri Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdiAr11 commented Oct 28, 2024

koppor commented Dec 9, 2024

AdiAr11 commented Oct 19, 2024 •

edited by koppor

Loading

HoussemNasri Oct 27, 2024 •

edited

Loading

HoussemNasri Oct 27, 2024 •

edited

Loading