Support modifier keys entry drag drop#16286
Conversation
PR Summary by QodoSupport modifier keys for entry drag-and-drop between libraries
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Duplicate result ignored
|
|
my plan for this feature:
|
| } | ||
| } | ||
|
|
||
| private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) { |
There was a problem hiding this comment.
@koppor
Should I move this conversion method to another class (may be utility)?
| } | ||
| } | ||
|
|
||
| private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) { |
There was a problem hiding this comment.
But what about having both javafx.scene.input.TransferMode and org.jabref.model.TransferMode used in FrameDndHandler?
| GroupTreeNode copiedNode = parent.addSubgroup(groupTreeNodeToCopy.copyNode().getGroup()); | ||
| // add all entries of a groupTreeNode to the new library. | ||
| destinationLibraryTab.dropEntry(stateManager.getActiveDatabase().get(), groupTreeNodeToCopy.getEntriesInGroup(allEntries)); | ||
| destinationLibraryTab.dropEntry(stateManager.getActiveDatabase().get(), groupTreeNodeToCopy.getEntriesInGroup(allEntries), groupTreeNodeToCopy.getEntriesInGroup(allEntries), org.jabref.model.TransferMode.COPY); |
There was a problem hiding this comment.
This is the only existing call that passes the same list for both originalEntries and entriesToAdd, is this approach acceptable ?
|
|
||
| @SuppressWarnings({"FieldCanBeLocal"}) | ||
| private Subscription dividerPositionSubscription; | ||
| @SuppressWarnings({"FieldCanBeLocal"}) private Subscription dividerPositionSubscription; |
There was a problem hiding this comment.
1. Unrelated reformatting in librarytab 📘 Rule violation ⚙ Maintainability
This PR includes multiple whitespace/line-wrapping-only edits (e.g., collapsing multi-line declarations/constructors into single long lines) that add diff noise and reduce readability without being required for the behavior change. This violates the requirement to preserve existing formatting and avoid unrelated reformatting.
Agent Prompt
## Issue description
The PR includes formatting-only changes (line wrapping/whitespace) unrelated to the functional drag-and-drop/move behavior change.
## Issue Context
Compliance requires minimizing diff noise and preserving existing formatting except where needed for the change.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[155-180]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[101-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| AutoDetectionImportOutcome createAutoDetectionImportOutcome(Path file, ImportResult importResult) { | ||
| List<BibEntry> importedEntries = importResult.parserResult().getDatabase().getEntries(); | ||
| if (importResult.parserResult().hasWarnings()) { | ||
| String warningMessage = Localization.lang("File was imported as %0, but warnings were reported: %1", | ||
| importResult.format(), | ||
| importResult.parserResult().getErrorMessage()); | ||
| String warningMessage = Localization.lang("File was imported as %0, but warnings were reported: %1", importResult.format(), importResult.parserResult().getErrorMessage()); | ||
| if (importedEntries.isEmpty()) { | ||
| return new AutoDetectionImportOutcome( | ||
| List.of(createEmptyEntryWithLink(file)), | ||
| false, | ||
| Localization.lang("No importable data was found in %0. An empty entry was created with file link.", importResult.format()) | ||
| + " " | ||
| + importResult.parserResult().getErrorMessage()); | ||
| return new AutoDetectionImportOutcome(List.of(createEmptyEntryWithLink(file)), false, Localization.lang("No importable data was found in %0. An empty entry was created with file link.", importResult.format()) + " " + importResult.parserResult().getErrorMessage()); | ||
| } |
There was a problem hiding this comment.
2. Localized message built by concatenation 📘 Rule violation ⚙ Maintainability
A user-facing message is constructed via string concatenation (`Localization.lang(...) + " " + ...`), which prevents proper localization and translator reordering. This violates the placeholder-based localization requirement.
Agent Prompt
## Issue description
A user-facing localized message is built using string concatenation instead of a single translation string with placeholders.
## Issue Context
Localization rules require the full message format to be in the translation resource, with dynamic parts provided via placeholders.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[252-258]
- jablib/src/main/resources/l10n/JabRef_en.properties[3037-3045]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision).thenAccept(duplicateHandledEntry -> { | ||
| if (duplicateHandledEntry.isEmpty()) { | ||
| tracker.markSkipped(); | ||
| return; | ||
| } | ||
| continueImportAfterDuplicateHandling(transferInformation, entry, tracker); | ||
| }).exceptionally(exception -> { |
There was a problem hiding this comment.
3. Duplicate result ignored 🐞 Bug ≡ Correctness
In ImportHandler.importEntryWithDuplicateCheck, when a duplicate is found and resolved, the code ignores the Optional<BibEntry> returned from handleDuplicates and imports the original entry instead. This bypasses merge/keep-right outcomes and can insert the wrong (uncleaned/unmerged) entry into the target library.
Agent Prompt
## Issue description
When a duplicate is detected and `handleDuplicates(...)` returns a non-empty `Optional<BibEntry>`, the import flow must continue with that returned entry. The current code continues with the original `entry`, discarding duplicate-resolution results (e.g., merged entry) and the cleaned-up entry variant.
## Issue Context
- `handleDuplicates(...)` may return a merged entry or other decision-dependent result.
- The current callback checks `duplicateHandledEntry.isEmpty()` but then imports `entry` anyway.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[303-327]
## Suggested change
Replace:
- `continueImportAfterDuplicateHandling(transferInformation, entry, tracker);`
With:
- `continueImportAfterDuplicateHandling(transferInformation, duplicateHandledEntry.get(), tracker);`
Also ensure the imported entry is the cleaned/decision-result entry (not the original input).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| BackgroundTask.wrap(() -> adjustLinkedFilesForTargetIfRequired(transferInformation, entryForImport)).onFailure(e -> { | ||
| LOGGER.error("Error adjusting linked files for target", e); | ||
| dialogService.notify(Localization.lang("Could not adjust linked files. The entry was imported without linked-file adjustments.")); | ||
| importCleanedEntries(transferInformation, List.of(entryForImport)); | ||
| tracker.markImported(entryForImport); | ||
| }).onSuccess(adjustedEntry -> { | ||
| importCleanedEntries(transferInformation, List.of(adjustedEntry)); | ||
| tracker.markImported(adjustedEntry); | ||
| if (transferInformation != null && transferInformation.transferMode() == org.jabref.model.TransferMode.MOVE && tracker.getImportedCount() == transferInformation.sourceEntries().size()) { | ||
| BibDatabase sourceDatabase = transferInformation.bibDatabaseContext().getDatabase(); | ||
| List<BibEntry> sourceEntries = transferInformation.sourceEntries(); | ||
| sourceDatabase.removeEntries(sourceEntries); | ||
| } |
There was a problem hiding this comment.
4. Move cleanup order-dependent 🐞 Bug ☼ Reliability
Source entry removal for MOVE is executed only inside the linked-file-adjustment onSuccess callback, while the onFailure callback still imports the entry and increments the imported count without ever attempting cleanup. Because per-entry imports run concurrently, MOVE cleanup becomes order-dependent and can be skipped entirely if the last completed entry goes through onFailure.
Agent Prompt
## Issue description
MOVE source-entry cleanup (`sourceDatabase.removeEntries(sourceEntries)`) is performed only in the `onSuccess` branch of linked-file adjustment. If the last processed entry completes via `onFailure`, cleanup never runs even though the failure path still imports the entry and marks it as imported.
## Issue Context
- Imports run concurrently (one task chain per entry).
- `onFailure` calls `importCleanedEntries(...)` and `tracker.markImported(...)` but does not evaluate MOVE cleanup.
- Linked-file adjustment can involve filesystem operations and may fail.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[330-345]
- jabgui/src/main/java/org/jabref/gui/externalfiles/EntryImportHandlerTracker.java[19-75]
## Suggested fix
Implement MOVE cleanup in a completion-safe place that runs exactly once after all entries are processed, regardless of per-entry success/failure ordering. Two viable approaches:
1) Call a shared `maybeFinalizeMove(transferInformation, tracker)` after `tracker.markImported(...)` in BOTH `onSuccess` and `onFailure`, and guard it so it runs once.
2) Move the MOVE finalization to the tracker’s `onFinish` hook (single-shot), checking `transferMode == MOVE` and `importedCount == sourceEntries.size()` before removing source entries.
Ensure the cleanup cannot be skipped due to callback ordering.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit be69001 |
Related issues and pull requests
Closes #16109
PR Description
This PR fixes the move operation between libraries by removing the original entries from the source library after all entries have been successfully imported into the target library.
pr.des.mp4
Steps to test
AI usage
ChatGPT (GPT-5.5)
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)