-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Copy linked files on entry transfer #13535
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?
Changes from 71 commits
7b35786
507c203
327e8b3
80a5dd8
4a94235
39e0484
2a58afb
4024b48
87a064f
f864dc3
638bc91
593d36f
f6cf22f
f53d4cc
d4871af
7385bd0
52fd290
4b3011f
2a9d039
646da11
b06fbbc
3f03974
a85b157
a0dcacd
0de8f52
9cc27f7
16bfa81
a734930
2766ba9
98b0a24
0309d35
ee18968
582c392
031f835
17d97b3
397efaf
f0e59a0
540fb72
1e4e08c
3863c2c
30c6ef5
ac3cdd3
11d7dbe
ae369d5
dd02975
cd2cbf5
6e24f48
d862e20
0e22f72
526a86e
bc21d34
857380d
9fd3881
140dc2d
ab218d8
b171636
08847d2
76a7e16
fe33a01
041c2df
d2de51d
765b7f8
9a11db1
91e67fb
ad9d623
2fa7951
7330a9e
61dfd6c
f6544ea
49ae45c
6a99ffd
59d8bc7
49d9127
3bdd340
e6e8454
88e8506
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# File Transfer Between Bib Entries | ||
|
||
*Note:* | ||
"Reachable" here denotes that the linked file can be accessed via a relative path that does **not** climb up the directory structure (i.e., no "`..`" segments beyond the root directory). | ||
Additionally, this check respects all configured **directories for files** as defined in JabRef's file linking settings (see [directories for files](https://docs.jabref.org/finding-sorting-and-cleaning-entries/filelinks#directories-for-files)). | ||
|
||
## File is reachable and should not be copied | ||
`req~logic.externalfiles.file-transfer.reachable-no-copy~1` | ||
When a linked file is reachable from the target context, the system must adjust the relative path in the target entry but must not copy the file again. | ||
|
||
Needs: impl | ||
|
||
## File is not reachable, but the path is the same | ||
`req~logic.externalfiles.file-transfer.not-reachable-same-path~1` | ||
When a linked file is not reachable from the target context, and the relative path in both source and target entry is the same, the file must be copied to the target context. | ||
|
||
Needs: impl | ||
|
||
## File is not reachable, and a different path is used | ||
`req~logic.externalfiles.file-transfer.not-reachable-different-path~1` | ||
When a linked file is not reachable from the target context, and the relative path differs between source and target entries, the file must be copied and the directory structure must be created to preserve the relative link. | ||
|
||
Needs: impl | ||
|
||
<!-- markdownlint-disable-file MD022 --> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ | |
import org.jabref.gui.dialogs.AutosaveUiManager; | ||
import org.jabref.gui.exporter.SaveDatabaseAction; | ||
import org.jabref.gui.externalfiles.AutoRenameFileOnEntryChange; | ||
import org.jabref.gui.externalfiles.EntryImportHandlerTracker; | ||
import org.jabref.gui.externalfiles.ImportHandler; | ||
import org.jabref.gui.fieldeditors.LinkedFileViewModel; | ||
import org.jabref.gui.importer.actions.OpenDatabaseAction; | ||
|
@@ -61,6 +62,7 @@ | |
import org.jabref.logic.ai.AiService; | ||
import org.jabref.logic.citationstyle.CitationStyleCache; | ||
import org.jabref.logic.command.CommandSelectionTab; | ||
import org.jabref.logic.externalfiles.LinkedFileTransferHelper; | ||
import org.jabref.logic.importer.FetcherClientException; | ||
import org.jabref.logic.importer.FetcherException; | ||
import org.jabref.logic.importer.FetcherServerException; | ||
|
@@ -810,6 +812,7 @@ public void insertEntries(final List<BibEntry> entries) { | |
} | ||
|
||
public void copyEntry() { | ||
clipBoardManager.setSourceBibDatabaseContext(this.getBibDatabaseContext()); | ||
int entriesCopied = doCopyEntry(getSelectedEntries()); | ||
if (entriesCopied >= 0) { | ||
dialogService.notify(Localization.lang("Copied %0 entry(s)", entriesCopied)); | ||
|
@@ -838,17 +841,48 @@ private int doCopyEntry(List<BibEntry> selectedEntries) { | |
} | ||
|
||
public void pasteEntry() { | ||
List<BibEntry> entriesToAdd; | ||
String content = ClipBoardManager.getContents(); | ||
entriesToAdd = importHandler.handleBibTeXData(content); | ||
List<BibEntry> entriesToAdd = importHandler.handleBibTeXData(content); | ||
if (entriesToAdd.isEmpty()) { | ||
entriesToAdd = handleNonBibTeXStringData(content); | ||
} | ||
if (entriesToAdd.isEmpty()) { | ||
return; | ||
} | ||
BibDatabaseContext sourceBibDatabaseContext = clipBoardManager.getSourceBibDatabaseContext().orElse(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment before needs to be added
|
||
copyEntriesWithFeedback(entriesToAdd, sourceBibDatabaseContext); | ||
} | ||
|
||
private void copyEntriesWithFeedback(List<BibEntry> entriesToAdd, BibDatabaseContext sourceBibDatabaseContext) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The method is a nearly 100% code duplication from The version of Move the method to package There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
final List<BibEntry> finalEntriesToAdd = entriesToAdd; | ||
|
||
EntryImportHandlerTracker tracker = new EntryImportHandlerTracker(finalEntriesToAdd.size()); | ||
|
||
tracker.setOnFinish(() -> { | ||
int importedCount = tracker.getImportedCount(); | ||
int skippedCount = tracker.getSkippedCount(); | ||
|
||
String targetName = bibDatabaseContext.getDatabasePath() | ||
.map(path -> path.getFileName().toString()) | ||
.orElse(Localization.lang("target library")); | ||
|
||
if (importedCount == finalEntriesToAdd.size()) { | ||
dialogService.notify(Localization.lang("Pasted %0 entry(s) to %1", | ||
String.valueOf(importedCount), targetName)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indent is off |
||
} else if (importedCount == 0) { | ||
dialogService.notify(Localization.lang("No entry was pasted to %0", targetName)); | ||
} else { | ||
dialogService.notify(Localization.lang("Pasted %0 entry(s) to %1. %2 were skipped", | ||
String.valueOf(importedCount), targetName, String.valueOf(skippedCount))); | ||
} | ||
if (sourceBibDatabaseContext != null) { | ||
LinkedFileTransferHelper | ||
.adjustLinkedFilesForTarget(sourceBibDatabaseContext, | ||
bibDatabaseContext, preferences.getFilePreferences()); | ||
} | ||
}); | ||
|
||
importHandler.importEntriesWithDuplicateCheck(bibDatabaseContext, entriesToAdd); | ||
importHandler.importEntriesWithDuplicateCheck(bibDatabaseContext, finalEntriesToAdd, tracker); | ||
} | ||
|
||
private List<BibEntry> handleNonBibTeXStringData(String data) { | ||
|
@@ -866,11 +900,12 @@ private List<BibEntry> handleNonBibTeXStringData(String data) { | |
} | ||
} | ||
|
||
public void dropEntry(List<BibEntry> entriesToAdd) { | ||
importHandler.importEntriesWithDuplicateCheck(bibDatabaseContext, entriesToAdd); | ||
public void dropEntry(List<BibEntry> entriesToAdd, BibDatabaseContext sourceBibDatabaseContext) { | ||
copyEntriesWithFeedback(entriesToAdd, sourceBibDatabaseContext); | ||
} | ||
|
||
public void cutEntry() { | ||
clipBoardManager.setSourceBibDatabaseContext(this.getBibDatabaseContext()); | ||
int entriesCopied = doCopyEntry(getSelectedEntries()); | ||
int entriesDeleted = doDeleteEntry(StandardActions.CUT, mainTable.getSelectedEntries()); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,6 +251,7 @@ public void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext, | |
} | ||
|
||
private void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext, BibEntry entry, DuplicateResolverDialog.DuplicateResolverResult decision, EntryImportHandlerTracker tracker) { | ||
// The original entry should not be modified | ||
BibEntry entryToInsert = cleanUpEntry(bibDatabaseContext, entry); | ||
|
||
BackgroundTask.wrap(() -> findDuplicate(bibDatabaseContext, entryToInsert)) | ||
|
@@ -279,8 +280,10 @@ private void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext | |
|
||
@VisibleForTesting | ||
BibEntry cleanUpEntry(BibDatabaseContext bibDatabaseContext, BibEntry entry) { | ||
// The original entry should not be modified | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this - the comment is already at line 254. |
||
BibEntry entryCopy = new BibEntry(entry); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this to line 255 (in the calling method) Add JavaDoc that the entry at hand is modified. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean I should add JavaDoc to the cleanUpEntry method? I am not the author of this method, should I still add the JavaDoc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please. Make the software better as a whole. Reasons: |
||
ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); | ||
return cleanup.doPostCleanup(entry); | ||
return cleanup.doPostCleanup(entryCopy); | ||
} | ||
|
||
public Optional<BibEntry> findDuplicate(BibDatabaseContext bibDatabaseContext, BibEntry entryToCheck) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
import org.jabref.gui.importer.actions.OpenDatabaseAction; | ||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.logic.util.io.FileUtil; | ||
import org.jabref.model.database.BibDatabaseContext; | ||
import org.jabref.model.entry.BibEntry; | ||
import org.jabref.model.groups.GroupTreeNode; | ||
|
||
|
@@ -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); | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
destinationLibraryTab.dropEntry(entryCopies, sourceBibDatabaseContext); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swap the parameters to preserve the order source, target (aspects). |
||
} else if (hasGroups(dragboard)) { | ||
dropGroups(dragboard, destinationLibraryTab); | ||
} | ||
|
@@ -213,7 +215,7 @@ private void copyGroupTreeNode(LibraryTab destinationLibraryTab, GroupTreeNode p | |
// add groupTreeNodeToCopy to the parent-- in the first run that will the source/main GroupTreeNode | ||
GroupTreeNode copiedNode = parent.addSubgroup(groupTreeNodeToCopy.copyNode().getGroup()); | ||
// add all entries of a groupTreeNode to the new library. | ||
destinationLibraryTab.dropEntry(groupTreeNodeToCopy.getEntriesInGroup(allEntries)); | ||
destinationLibraryTab.dropEntry(groupTreeNodeToCopy.getEntriesInGroup(allEntries), stateManager.getActiveDatabase().get()); | ||
// List of all children of groupTreeNodeToCopy | ||
List<GroupTreeNode> children = groupTreeNodeToCopy.getChildren(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,8 @@ private static Menu createCopyToMenu(ActionFactory factory, | |
factory.createCustomMenuItem( | ||
StandardActions.COPY_TO, | ||
new CopyTo(dialogService, stateManager, preferences.getCopyToPreferences(), | ||
importHandler, sourceDatabaseContext, targetDatabaseContext), | ||
preferences.getFilePreferences(), importHandler, sourceDatabaseContext, | ||
targetDatabaseContext), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indent seems to be off somehow |
||
targetDatabaseName | ||
) | ||
); | ||
|
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.
Maybe, add that in the following "reachable" denotes: reachable using a relative path not climbing up the directory structure?
Also that it respects all configured directories for files
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.
Here’s my update to the description regarding file reachability:
“Reachable” means the linked file can be accessed via a relative path that does not climb up the directory structure (no .. segments beyond the root).
Also, this respects all configured directories for files as per JabRef’s settings (link).
According to this understanding, the implementation is aligned with these cases.
Does this match your understanding?
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.
You should move the "reachable" paragraph before the list of requirements. Reason: One often jumps directly to a requirement - and does NOT read the other requirements --> but the definition is put in another requiement.
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.
Done.