Allow updating entries from multi merge results#15562
Draft
deepgupta6 wants to merge 4 commits into
Draft
Conversation
Contributor
Review Summary by Qodo(Agentic_describe updated until commit c69403b)Persist merge results and improve entry type handling
WalkthroughsDescription• Refactor merge logic into reusable UpdateOriginalEntry class • Apply merged results to original entry as single undoable action • Preserve specific entry types instead of defaulting to Misc • Add guards against empty merge results to prevent field clearing • Add regression tests for merge initialization and empty result handling Diagramflowchart LR
A["User selects merge result"] --> B["UpdateOriginalEntry processes merge"]
B --> C["Apply field changes"]
B --> D["Update entry type if needed"]
C --> E["Record as single undo action"]
D --> E
E --> F["Notify user of update"]
G["Empty merge result"] --> H["Guard prevents field clearing"]
H --> F
File Changes1. jabgui/src/main/java/org/jabref/gui/frame/MainMenu.java
|
Contributor
Code Review by Qodo
1. Passing null to UndoableFieldChange
|
Comment on lines
+77
to
+119
| Optional<BibEntry> mergedEntry = dialogService.showCustomDialogAndWait(mergedEntriesView); | ||
|
|
||
| if (mergedEntry.isPresent()) { | ||
| NamedCompoundEdit compoundEdit = new NamedCompoundEdit(Localization.lang("Merge entry with information")); | ||
|
|
||
| // Updated the original entry with the new fields | ||
| Set<Field> jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| jointFields.addAll(mergedEntry.get().getFields()); | ||
| Set<Field> originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| originalFields.addAll(originalEntry.getFields()); | ||
| boolean edited = false; | ||
|
|
||
| // entry type | ||
| EntryType oldType = originalEntry.getType(); | ||
| EntryType newType = mergedEntry.get().getType(); | ||
|
|
||
| if (!oldType.equals(newType)) { | ||
| originalEntry.setType(newType); | ||
| compoundEdit.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); | ||
| edited = true; | ||
| } | ||
|
|
||
| // fields | ||
| for (Field field : jointFields) { | ||
| Optional<String> originalString = originalEntry.getField(field); | ||
| Optional<String> mergedString = mergedEntry.get().getField(field); | ||
| if (originalString.isEmpty() || !originalString.equals(mergedString)) { | ||
| originalEntry.setField(field, mergedString.get()); // mergedString always present | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.orElse(null), | ||
| mergedString.get())); | ||
| edited = true; | ||
| } | ||
| } | ||
|
|
||
| // Remove fields which are not in the merged entry, unless they are internal fields | ||
| for (Field field : originalFields) { | ||
| if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { | ||
| Optional<String> originalString = originalEntry.getField(field); | ||
| originalEntry.clearField(field); | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); // originalString always present | ||
| edited = true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
1. Early merge wipes fields 🐞 Bug ≡ Correctness
If the user clicks “Merge entries” before sources finish loading, the dialog can return the still-empty default merged entry, and the new apply logic will then clear nearly all non-internal fields on the original entry. This can cause severe, silent data loss in the selected BibEntry (only recoverable if the user immediately uses Undo).
Agent Prompt
## Issue description
The merge dialog’s result can be the default, still-empty `mergedEntry` if the user confirms before sources finish loading (the original entry is currently loaded via `BackgroundTask` too). The new apply logic then clears all original fields not present in that empty result, causing severe data loss.
## Issue Context
`MultiMergeEntriesViewModel` initializes `mergedEntry` as `new BibEntry()` and only fills it after each `EntrySource` finishes loading. `UpdateWithBibliographicInformationByWebFetchers` now applies the dialog result back to `originalEntry` and clears fields missing from the merged result.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/mergeentries/UpdateWithBibliographicInformationByWebFetchers.java[55-75]
- jabgui/src/main/java/org/jabref/gui/mergeentries/UpdateWithBibliographicInformationByWebFetchers.java[77-130]
- jabgui/src/main/java/org/jabref/gui/mergeentries/multiwaymerge/MultiMergeEntriesView.java[91-141]
## Suggested fix
1. Ensure the original entry is added synchronously so the merged result is at least initialized with the original fields immediately (e.g., use the `addSource(String, BibEntry)` overload for the original entry).
2. Additionally (recommended), disable the OK/"Merge entries" button until at least the original source has loaded and/or until `mergedEntry` has been populated (e.g., bind the OK button’s `disableProperty` to a loading indicator / mergedEntry fields emptiness).
3. As a safety net, before applying changes, guard against an empty merged result (e.g., if `mergedEntry.getFields().isEmpty()` then treat as cancel and do not mutate `originalEntry`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
faneeshh
reviewed
Apr 16, 2026
| if (edited) { | ||
| compoundEdit.end(); | ||
| undoManager.addEdit(compoundEdit); | ||
| dialogService.notify(Localization.lang("Updated entry with merged information from multiple sources")); |
Collaborator
There was a problem hiding this comment.
Does this key exist in the properties?
Comment on lines
+79
to
+109
| if (mergedEntry.isPresent()) { | ||
| NamedCompoundEdit compoundEdit = new NamedCompoundEdit(Localization.lang("Merge entry with information")); | ||
|
|
||
| // Updated the original entry with the new fields | ||
| Set<Field> jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| jointFields.addAll(mergedEntry.get().getFields()); | ||
| Set<Field> originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| originalFields.addAll(originalEntry.getFields()); | ||
| boolean edited = false; | ||
|
|
||
| // entry type | ||
| EntryType oldType = originalEntry.getType(); | ||
| EntryType newType = mergedEntry.get().getType(); | ||
|
|
||
| if (!oldType.equals(newType)) { | ||
| originalEntry.setType(newType); | ||
| compoundEdit.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); | ||
| edited = true; | ||
| } | ||
|
|
||
| // fields | ||
| for (Field field : jointFields) { | ||
| Optional<String> originalString = originalEntry.getField(field); | ||
| Optional<String> mergedString = mergedEntry.get().getField(field); | ||
| if (originalString.isEmpty() || !originalString.equals(mergedString)) { | ||
| originalEntry.setField(field, mergedString.get()); // mergedString always present | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.orElse(null), | ||
| mergedString.get())); | ||
| edited = true; | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
The merge logic is same to what's already in FetchAndMergeEntry.showMergeDialog( ). You can extract that into a shared helper method maybe.
| return; | ||
| } | ||
| if (mergedEntry.get().getType() == StandardEntryType.Misc | ||
| && entry.getType() != null |
Collaborator
There was a problem hiding this comment.
You can simplify this to just the Misc comparison.
Member
|
@deepgupta6 if there are no responses to review comments for the next two days, we will be closing the PR due to inactivity. @faneeshh thank you for reviewing! |
d4e0763 to
0426817
Compare
merged result is initialized immediately. Guard UpdateOriginalEntry against empty merged results to avoid clearing fields when the dialog is confirmed before background sources finish loading. Add regression tests for immediate merged-entry initialization and empty merge application handling.
Contributor
Review Summary by QodoPersist merge results and improve entry type handling in bibliographic updates
WalkthroughsDescription• Refactored merge logic into reusable UpdateOriginalEntry class for consistency • Added undo support to web-based bibliographic update flow • Improved entry type preservation by avoiding fallback to Misc type • Added guards against empty merge results to prevent field clearing • Added regression tests for merge initialization and empty result handling Diagramflowchart LR
A["Web Fetchers Update"] --> B["Multi-Merge Dialog"]
C["Single Fetcher Merge"] --> B
B --> D["UpdateOriginalEntry"]
D --> E["Apply Changes with Undo"]
D --> F["Notify User"]
File Changes1. jabgui/src/main/java/org/jabref/gui/frame/MainMenu.java
|
Contributor
Code Review by Qodo
1. Type-only merge skipped
|
Comment on lines
+36
to
+41
| public void update() { | ||
| if (mergedEntry.isPresent() && !mergedEntry.get().getFields().isEmpty()) { | ||
| String editName = fetcher | ||
| .map(value -> Localization.lang("Merge entry with %0 information", value.getName())) | ||
| .orElseGet(() -> Localization.lang("Merge entry with information")); | ||
| NamedCompoundEdit compoundEdit = new NamedCompoundEdit(editName); |
Contributor
There was a problem hiding this comment.
1. Type-only merge skipped 🐞 Bug ≡ Correctness
UpdateOriginalEntry.update() treats a merge result with no fields as "Canceled" and returns without applying any changes, which drops valid type-only merges. In the 3-way merge dialog, changing only the entry type updates the merged entry via setType (not via setField), so mergedEntry.getFields() can remain empty even though the user confirmed the merge.
Agent Prompt
### Issue description
`UpdateOriginalEntry.update()` currently short-circuits when `mergedEntry.get().getFields().isEmpty()`. This prevents persisting type-only merges coming from the 3-way merge dialog (type changes are applied via `BibEntry.setType`, which does not add anything to `getFields()`). It also misreports a confirmed merge as "Canceled".
### Issue Context
- `BibEntry.getFields()` only returns keys from the internal `fields` map; entry type is stored separately.
- 3-way merge UI sets type through `mergedEntry.setType(...)` when the `TYPE_HEADER` row changes.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/mergeentries/UpdateOriginalEntry.java[36-94]
### What to change
1. Treat only `mergedEntry.isEmpty()` as cancel.
2. Allow proceeding when the merge result contains *either* (a) field changes or (b) a type change.
3. Avoid destructive field clearing when `mergedEntry.get().getFields()` is empty:
- Apply type change if needed.
- Skip field update/removal loops when there are no merged fields.
4. Ensure notifications reflect the action:
- If user confirmed but nothing changed: "No information added".
- If type changed only: record undo + show updated notification.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
|
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
Author
|
@faneesh Sorry for the late response, I was out of town. now I have done the changes you suggested |
Author
|
@koppor I think I’ve completed the requested changes. Could you please let me know why the PR was moved back to draft? If there is something remaining to change you can suggest |
Member
|
This needs to be reviewed with refactoring miner |
Author
|
@koppor I have checked with refactoring miner and found no issue. Adding screenshot for proof |
Author
|
Let me know if anything else needs to be changed, I’ll take care of it. |
Member
You did not fully understand about the UI and intention of RefactoringMiner... |
Author
|
@koppor sorry for taking too much of your time. |
Author
|
@Siedlerchr @calixtus @koppor |
Member
Please be patient. If you are done with your part, mark the PR ready for review (move it out of draft status). We will take a look as we find time. |
Contributor
Code Review by Qodo
1. mergedEntry uses isPresent()+get()
|
Contributor
|
Do not mark a PR as ready-for-review if changes are required. |
Comment on lines
+36
to
+38
| public void update() { | ||
| if (mergedEntry.isPresent() && !mergedEntry.get().getFields().isEmpty()) { | ||
| String editName = fetcher |
Contributor
There was a problem hiding this comment.
1. mergedentry uses ispresent()+get() 📘 Rule violation ⚙ Maintainability
UpdateOriginalEntry.update() uses mergedEntry.isPresent() and then calls mergedEntry.get() multiple times, which is discouraged and less safe/idiomatic than fluent Optional handling. This violates the Optional control-flow compliance requirement and increases the chance of future misuse/maintenance errors.
Agent Prompt
## Issue description
`UpdateOriginalEntry.update()` uses `mergedEntry.isPresent()` and then dereferences with `mergedEntry.get()`. This violates the project’s Optional style rule and is less readable/robust than fluent Optional control flow.
## Issue Context
This is newly introduced code in `UpdateOriginalEntry` and should follow JabRef’s Optional conventions.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/mergeentries/UpdateOriginalEntry.java[36-95]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
The requested changes were not addressed for 10 days. Please follow-up in the next 10 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
Author
@subhramit whenever I mark the PR as “Ready for Review,” it automatically gets converted back to Draft again and i dont know how to move further could you please suggest what might be causing this? |
Contributor
|
Persistent review updated to latest commit c69403b |
Comment on lines
+64
to
+79
| if (originalString.isEmpty() || !originalString.equals(mergedString)) { | ||
| originalEntry.setField(field, mergedString.get()); // mergedString always present | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.orElse(null), | ||
| mergedString.get())); | ||
| edited = true; | ||
| } | ||
| } | ||
|
|
||
| // Remove fields which are not in the merged entry, unless they are internal fields | ||
| for (Field field : originalFields) { | ||
| if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { | ||
| Optional<String> originalString = originalEntry.getField(field); | ||
| originalEntry.clearField(field); | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); // originalString always present | ||
| edited = true; | ||
| } |
Contributor
There was a problem hiding this comment.
1. Passing null to undoablefieldchange 📘 Rule violation ☼ Reliability
New code passes null as an argument to UndoableFieldChange, extending a null-based contract instead of using explicit nullability/safer alternatives. This violates the project rule to avoid passing null and can lead to null-related bugs and unclear contracts.
Agent Prompt
## Issue description
`UpdateOriginalEntry.update()` constructs `UndoableFieldChange` edits passing `null` for old/new values (`originalString.orElse(null)` and `..., null`). This violates the project's null-safety rule.
## Issue Context
Prefer an API that encodes absence explicitly (e.g., Optional) or a dedicated constant/sentinel, or adjust `UndoableFieldChange` usage so that `null` is not passed.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/mergeentries/UpdateOriginalEntry.java[64-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+54
to
+56
| if (mergedEntry.get().getType() == StandardEntryType.Misc | ||
| && entry.getType() != StandardEntryType.Misc) { | ||
| mergedEntry.get().setType(entry.getType()); |
Contributor
There was a problem hiding this comment.
2. Entrytype identity comparison 🐞 Bug ≡ Correctness
MultiMergeEntriesViewModel.updateFields() uses '=='/'!=' to compare EntryType values against StandardEntryType.Misc, which is not reliable because EntryType is an interface and not all implementations are enums. This can cause the merged entry type to remain Misc even when a source entry has a more specific type, reducing merge accuracy.
Agent Prompt
### Issue description
`MultiMergeEntriesViewModel.updateFields` compares `EntryType` values using `==`/`!=` against `StandardEntryType.Misc`. Since `EntryType` is an interface and can be implemented by non-enum types, identity comparison can fail even when types are logically equal, preventing the merged entry type from being upgraded away from `Misc`.
### Issue Context
This logic is part of the PR’s goal to keep a more specific entry type when possible.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/mergeentries/multiwaymerge/MultiMergeEntriesViewModel.java[50-57]
### Suggested change
Replace identity checks with logical equality, e.g.:
```java
if (StandardEntryType.Misc.equals(mergedEntry.get().getType())
&& !StandardEntryType.Misc.equals(entry.getType())) {
mergedEntry.get().setType(entry.getType());
}
```
(Equivalent `.equals(...)` forms are fine; the key is to avoid `==`/`!=` for `EntryType`.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
The requested changes were not addressed for 10 days. Please follow-up in the next 10 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
Author
@koppor could you please suggest the changes? I would be very grateful if you could spend some time reviewing this PR. |
koppor
reviewed
May 18, 2026
Comment on lines
+40
to
58
| @BeforeEach | ||
| void setUp() { | ||
| viewModel = new MultiMergeEntriesViewModel(); | ||
| } | ||
|
|
||
| @Test | ||
| void updateFieldsIgnoresNullEntry() { | ||
| viewModel.updateFields(null); | ||
| assertEquals(new BibEntry(), viewModel.mergedEntryProperty().get()); | ||
| } | ||
|
|
||
| @Test | ||
| void updateFieldsSetsFieldWhenNotYetPresent() { | ||
| BibEntry source = new BibEntry().withField(StandardField.YEAR, "2015"); | ||
| viewModel.updateFields(source); | ||
| assertEquals("2015", viewModel.mergedEntryProperty().get().getField(StandardField.YEAR).orElse("")); | ||
| } | ||
|
|
||
| @ParameterizedTest |
Member
There was a problem hiding this comment.
Source move is wrong - breaks coherence here.
wanling0000
reviewed
May 18, 2026
Comment on lines
+60
to
+79
| // fields | ||
| for (Field field : jointFields) { | ||
| Optional<String> originalString = originalEntry.getField(field); | ||
| Optional<String> mergedString = mergedEntry.get().getField(field); | ||
| if (originalString.isEmpty() || !originalString.equals(mergedString)) { | ||
| originalEntry.setField(field, mergedString.get()); // mergedString always present | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.orElse(null), | ||
| mergedString.get())); | ||
| edited = true; | ||
| } | ||
| } | ||
|
|
||
| // Remove fields which are not in the merged entry, unless they are internal fields | ||
| for (Field field : originalFields) { | ||
| if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { | ||
| Optional<String> originalString = originalEntry.getField(field); | ||
| originalEntry.clearField(field); | ||
| compoundEdit.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); // originalString always present | ||
| edited = true; | ||
| } |
Collaborator
There was a problem hiding this comment.
I want to ask for clarification of the intended behavior here:
Because now, when a user deselects all toggles for a field (neither the old nor the new value was selected),
setField(field, "") was called, which treats "" field as removed from mergedEntry entirely.
Then in line 76 clearField(field) is called, because the field is no longer in jointFields.
Result: the original field is silently deleted from the entry.
I think, "deselect all" could mean either "don't update this field" (preserve original) or "clear this field(current behavior). The distinction isn't visible to the user in the UI
Member
There was a problem hiding this comment.
I think, "deselect all" could mean either "don't update this field" (preserve original)
+1 for this behavior
2173d86 to
6b75833
Compare
Collaborator
|
Hey, we noticed that you force-pushed your changes. Force pushing is a bad practice when working together on a project (mainly because it is not supported well by GitHub itself). Commits are lost and comments on commits lose their context, thus making it harder to review changes. When the pull request is getting integrated into In future, please avoid that. For now, you can continue working. |
Collaborator
|
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
Member
|
What did you do with your force-push? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
jabref.15265.mp4
Related issues and pull requests
Closes #15265
PR Description
This PR makes the web-based bibliographic update flow persist the selected merge result back to the original entry and records the change as a single undoable action, so the feature now matches the expected behavior. It also keeps a more specific entry type when possible instead of falling back to Misc, which improves merge accuracy.
Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)