Copy linked files on entry transfer #13535

UmutAkbayin · 2025-07-13T21:31:46Z

Adds a requirements specification file files.md documenting the linked file transfer logic between BibTeX entries in JabRef.
The document captures three key scenarios for when linked files are reachable or not in the target context and the expected behavior regarding path adjustment and file copying.

This improves traceability by formally specifying requirements that are linked to implementation and tests via OpenFastTrace.

Steps to test

Verify that the new requirements specification file docs/requirements/files.md is present, properly formatted, and follows JabRef conventions.
Run all existing and new unit tests in LinkedFileTransferHelperTest to ensure the linked file transfer logic behaves correctly across all covered scenarios.
Manually verify the business logic in JabRef, especially for the scenario where the linked file is not reachable and a nested directory structure must be created. Reviewers should confirm that the path adjustments and file copying behavior conform to the requirements.
Provide feedback if the handling of the last case (unreachable file with differing paths) aligns with project expectations.

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
I manually tested my changes in running JabRef (always required)
I added JUnit tests for changes (if applicable)
I added screenshots in the PR description (if change is visible to the user)
I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
[/] I checked the user 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 updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

jabgui/src/main/java/org/jabref/gui/LibraryTab.java

jablib/src/test/java/org/jabref/logic/externalfiles/LinkedFileTransferHelperTest.java

trag-bot · 2025-07-13T22:34:38Z

jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java

+    FilePreferences filePreferences
+  ) {
+
+    Set<BibEntry> modifiedEntries = new HashSet<>();


Using constructor for HashSet instead of modern Java collection factory method Set.of(). Modern Java practices recommend using factory methods for cleaner and more maintainable code.

could not find a quick solution. would have to refactor the logic then.

trag-bot · 2025-07-13T22:34:42Z

jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java

+
+    for (BibEntry entry : targetContext.getEntries()) {
+      boolean entryChanged = false;
+      List<LinkedFile> linkedFiles = new ArrayList<>();


Using ArrayList constructor instead of modern Java collection factory methods. Should use List.of() for empty list initialization following JabRef's conventions.

could not find a quick solution. would have to refactor the logic then.

TracBot is wrong here 😅

koppor · 2025-07-23T19:18:41Z

@UmutAkbayin Please read the bot comment - one week old

Fix the tests

koppor · 2025-07-23T19:19:05Z

I converted the PR to draft, because a) merge conflicts b) tests failing.

koppor · 2025-07-23T19:19:41Z

"Steps to test" seems to be AI generated. It is not a step-by-step guide. Please refine.

UmutAkbayin · 2025-07-24T06:12:35Z

I noticed that one unit test is affected by the changes in LibraryTab#pasteEntry. I’ll look into it tomorrow or over the weekend. I’ll also review the other things like code style.

Allows tracking the source BibDatabaseContext for later use in pasteEntry()

…er and setter Enables tracking the source database context for use in pasteEntry()

…variable

… CopyTo#copyEntriesWithFeedback and LibraryTab#pasteEntry

Add filePreferences parameter to CopyTo.java to enable LinkedFile#findIn functionality

… transfer

… primary directory

…sferHelperTest#isReachableFromPrimaryDirectory method

Cloning the BibEntry in importEntryWithDuplicateCheck to ensure that any file path adjustments during the copy/paste process do not affect the original entry in the source database. This avoids side effects when imported entries are modified (e.g. file links), especially when the same BibEntry instance is shared between databases.

…ls in LibraryTab and CopyTo to align with new method signature

…pying logic during entry transfer

UmutAkbayin · 2025-08-27T16:25:24Z

It looks like the LibraryTab part stopped working after the merge. Currently only CopyTo works, while copy & paste and drag & drop don’t. I’ll take a closer look at it.

Previously, EditAction created a new ClipBoardManager, which caused copy, cut, and paste to not use the shared manager. Now EditAction uses the shared ClipBoardManager, fixing copy/paste and drag & drop behavior in the right-click menu and elsewhere.

…nd drop

…xt for linked file adjustments

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java

UmutAkbayin · 2025-08-27T19:42:43Z

Originally, EditAction created a new ClipBoardManager, which caused copy, cut, and paste to not use the shared manager. This was my fault and has been fixed by using the existing ClipBoardManager. Copy/paste work correctly in the right-click menu and with keys.

Drag and Drop actually never worked, but I have fixed it now. I have passed the BibDatabaseContext in FrameDndHandler. Seems to work, but I trust the reviewer. Sorry, I had to experiment a little bit.

Note: This issue was not detected by unit tests, because the code never reached the helper class. It could be covered by integration tests in the future.

During testing on my NAS device, I noticed that deleting files via JabRef sometimes fails, and copied PDFs can occasionally become corrupted, while text files work fine. This appears to be related to NAS file permissions or network file system behavior, not the JabRef logic itself.

Situation in setup:

and after the paste:

I thinks this is the desired behavior.

UmutAkbayin · 2025-08-27T19:57:30Z

Only PR Checklist seems to fail. The layout has changed I guess, I tried to adjust it.

koppor

I did some code adapations.

Some comments.

I ttoally forgot in the issue to ask for a preference. Default should be "true". See #13295 how to add a preference. You can also use "Linked files" preferences.

koppor · 2025-08-28T10:17:53Z

jabgui/src/main/java/org/jabref/gui/edit/CopyTo.java

+        tracker.setOnFinish(() -> LinkedFileTransferHelper
+          .adjustLinkedFilesForTarget(sourceDatabaseContext, targetDatabaseContext, filePreferences)
+        );


This is assuming that "importHandler" in the line before (!) executes asynchronously.

It overwrites the handler set in line 96.

Did you play around here?

Should we just remove lines 96 to 111?

I forgot to correct this like in LibraryTab. I think it could work inside the first handler.

koppor · 2025-08-28T10:43:09Z

jabgui/src/main/java/org/jabref/gui/edit/EditAction.java

-                    case COPY -> tabSupplier.get().copyEntry();
-                    case CUT -> tabSupplier.get().cutEntry();
+                    case COPY -> {
+                        clipBoardManager.setSourceBibDatabaseContext(tabSupplier.get().getBibDatabaseContext());


This can be included in copyEntry

good point. done.

koppor · 2025-08-28T10:43:20Z

jabgui/src/main/java/org/jabref/gui/edit/EditAction.java

+                        tabSupplier.get().copyEntry();
+                    }
+                    case CUT -> {
+                        clipBoardManager.setSourceBibDatabaseContext(tabSupplier.get().getBibDatabaseContext());


This can be included in cutEntry

good point. done.

koppor · 2025-08-28T10:44:56Z

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

@@ -251,7 +251,7 @@ public void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext,
    }

    private void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext, BibEntry entry, DuplicateResolverDialog.DuplicateResolverResult decision, EntryImportHandlerTracker tracker) {
-        BibEntry entryToInsert = cleanUpEntry(bibDatabaseContext, entry);
+        BibEntry entryToInsert = new BibEntry(cleanUpEntry(bibDatabaseContext, entry));


Please add a short Java comemnt above. Maybe

// The original entry should not be modified

Please do the copy before cleanUpEntry - otherwise the source entry will be modified, too.

trag-bot · 2025-08-28T12:58:55Z

jabgui/src/main/java/org/jabref/gui/edit/EditAction.java

+                    case COPY -> {
+                        tabSupplier.get().copyEntry();
+                    }


The block arrow syntax with curly braces is unnecessarily verbose for a single statement. This reduces code readability and violates the principle of keeping code concise.

trag-bot · 2025-08-28T12:59:05Z

jabgui/src/main/java/org/jabref/gui/edit/EditAction.java

+                    case CUT -> {
+                        tabSupplier.get().cutEntry();
+                    }


The block arrow syntax with curly braces is unnecessarily verbose for a single statement. This reduces code readability and violates the principle of keeping code concise.

UmutAkbayin · 2025-08-28T12:59:10Z

I have adjusted everything except for the preferences part. I will take another look at that later.

I had to move the entry copy to the cleanUpEntry method, otherwise a non-final variable would be used in the lambda expression.

…es-on-entry-transfer

…le preferences Users should be able to disable the automatic file adjustment/copying during transfers via a preference setting (default: enabled).

UmutAkbayin · 2025-08-30T12:20:04Z

I added the File Preferences feature, as well as another unit test, and tested it manually.

…actices

trag-bot · 2025-08-30T12:47:07Z

jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java

+
+        for (BibEntry entry : targetContext.getEntries()) {
+            boolean entryChanged = false;
+            List<LinkedFile> linkedFiles = new ArrayList<>();


ArrayList constructor usage instead of modern Java collection factory methods makes code less maintainable and readable.

koppor

Some initial comments. I need several hours for revewing this. Hope to find time for it on Wednesday.

koppor · 2025-08-31T16:15:04Z

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

@@ -279,8 +280,10 @@ private void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext

    @VisibleForTesting
    BibEntry cleanUpEntry(BibDatabaseContext bibDatabaseContext, BibEntry entry) {
+        // The original entry should not be modified


Remove this - the comment is already at line 254.

koppor · 2025-08-31T16:15:29Z

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

@@ -279,8 +280,10 @@ private void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext

    @VisibleForTesting
    BibEntry cleanUpEntry(BibDatabaseContext bibDatabaseContext, BibEntry entry) {
+        // The original entry should not be modified
+        BibEntry entryCopy = new BibEntry(entry);


Move this to line 255 (in the calling method)

Add JavaDoc that the entry at hand is modified.

You mean I should add JavaDoc to the cleanUpEntry method? I am not the author of this method, should I still add the JavaDoc?
I will add a JavaDoc to the importEntryWithDuplicateCheck method.

Yes, please. Make the software better as a whole.

Reasons:

https://martinfowler.com/bliki/OpportunisticRefactoring.html

https://blog.cleancoder.com/uncle-bob/2015/11/18/TheProgrammersOath.html

koppor · 2025-08-31T17:01:22Z

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java

@@ -106,7 +107,8 @@ private void onTabDragDropped(Node destinationTabNode, DragEvent tabDragEvent, T
            if (hasEntries(dragboard)) {
                List<BibEntry> entryCopies = stateManager.getLocalDragboard().getBibEntries().stream()
                                                         .map(BibEntry::new).toList();
-                destinationLibraryTab.dropEntry(entryCopies);
+                BibDatabaseContext sourceBibDatabaseContext = stateManager.getActiveDatabase().orElse(null);
+                destinationLibraryTab.dropEntry(entryCopies, sourceBibDatabaseContext);


Swap the parameters to preserve the order source, target (aspects).

koppor · 2025-08-31T17:01:43Z

jabgui/src/main/java/org/jabref/gui/maintable/RightClickMenu.java

-                                    importHandler, sourceDatabaseContext, targetDatabaseContext),
+                                    preferences.getFilePreferences(), importHandler, sourceDatabaseContext,
+                                targetDatabaseContext),


Indent seems to be off somehow

koppor · 2025-08-31T17:11:47Z

jabgui/src/main/java/org/jabref/gui/LibraryTab.java

+        copyEntriesWithFeedback(entriesToAdd, sourceBibDatabaseContext);
+    }
+
+    private void copyEntriesWithFeedback(List<BibEntry> entriesToAdd, BibDatabaseContext sourceBibDatabaseContext) {


@Nullable should be annotated

The method is a nearly 100% code duplication from org.jabref.gui.edit.CopyTo#copyEntriesWithFeedback

The version of org.jabref.gui.edit.CopyTo#copyEntriesWithFeedback is not that good, because the non-standard use of Localization.lang should at least be documented.

Move the method to package org.jabref.gui.util. You can create a class CopyUtil with a static method.

Thanks for the review. Just to clarify - this method is code that I reused from the existing org.jabref.gui.edit.CopyTo#copyEntriesWithFeedback method (originally implemented by another author]). I copied it because I needed the same functionality in my code.
Since this represents code duplication as you noted, should I still go ahead and refactor it into the org.jabref.gui.util.CopyUtil class as suggested? This would benefit both the original location and my usage, and I'm happy to do the refactoring work even though I wasn't the original author. Just asking, because I don't want to 'steal' the code.

There should be not a thing such as authorship for methods... The aim of the community is to have the code in a good state. We are in the open source world; there should be a step back from author ship - its not art what we do here, its engineering :)

Thus, please, go ahead with refactoring.

koppor · 2025-08-31T17:12:34Z

jabgui/src/main/java/org/jabref/gui/LibraryTab.java

+                dialogService.notify(Localization.lang("Pasted %0 entry(s) to %1",
+              String.valueOf(importedCount), targetName));


Indent is off

koppor · 2025-08-31T17:55:19Z

jabgui/src/main/java/org/jabref/gui/LibraryTab.java

        if (entriesToAdd.isEmpty()) {
            entriesToAdd = handleNonBibTeXStringData(content);
        }
        if (entriesToAdd.isEmpty()) {
            return;
        }
+        BibDatabaseContext sourceBibDatabaseContext = clipBoardManager.getSourceBibDatabaseContext().orElse(null);


Comment before needs to be added

// Now, the BibEntries to add are known // The definitive insertion needs to happen now.

koppor · 2025-08-31T18:05:03Z

jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java

+                Optional<Path> sourcePathOpt = linkedFile.findIn(sourceContext, filePreferences);
+                Optional<Path> targetPrimaryOpt = getPrimaryPath(targetContext, filePreferences);
+
+                if (sourcePathOpt.isEmpty() || targetPrimaryOpt.isEmpty()) {


Add comment above

// In case the file wasn't found at the source or the target - just re-use the existing link

I don't understand this if. Why is the thing stopping if the fiel is not found in the targetPrimaryPath? There is no copy made before (is it?)

koppor · 2025-08-31T18:05:30Z

jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java

+                if (sourcePathOpt.get().startsWith(targetPrimaryOpt.get())) {
+                        relative = targetPrimaryOpt.get().relativize(sourcePathOpt.get());
+                } else {
+                        relative = Path.of("..").resolve(sourcePathOpt.get().getFileName());


Is this case ever hit? According to org.jabref.logic.util.io.FileUtil#find(java.lang.String, java.nio.file.Path), findIn always returns an aboslute path.

The logic in this part is from quite a while ago, so I’d like to take some time tomorrow to read back into it before responding in detail. I also noticed an issue in my branch (a RepositoryNotFoundException when switching tabs – only visible in the logs, work can still continue), and I’d like to investigate that in the same context.

Yeah - RepositoryNotFoundException is known and should be fixed soon (wanling0000#11). Background: We merged some updates of the git behavior into the main branch early. -- No need to investigate 😅

One could check with test coverage if this line is hit by test cases.

Sorry for the long delays while reviewing. Code review takes much time and all of this happens in our free time.

koppor · 2025-08-31T18:14:07Z

jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java

+                }
+
+                Optional<Path> sourcePathOpt = linkedFile.findIn(sourceContext, filePreferences);
+                Optional<Path> targetPrimaryOpt = getPrimaryPath(targetContext, filePreferences);


I wonder, why not findIn is used here, too. --> if this returns something, the link does not need to be adjusted at all - because it is still found.