Implement mass addition of bib information (Closes #372) #12025
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -70,38 +70,143 @@ public void fetchAndMerge(BibEntry entry, Field field) { | |||||
| } | ||||||
|
|
||||||
| public void fetchAndMerge(BibEntry entry, List<Field> fields) { | ||||||
| fields.forEach(field -> fetchAndMergeEntryWithDialog(entry, field)); | ||||||
| } | ||||||
|
|
||||||
| private void fetchAndMergeEntryWithDialog(BibEntry entry, Field field) { | ||||||
| entry.getField(field).ifPresent( | ||||||
| fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()).ifPresent( | ||||||
| fetcher -> executeFetchTaskWithDialog(fetcher, field, fieldContent, entry) | ||||||
| ) | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| private void executeFetchTaskWithDialog(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) { | ||||||
| BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) | ||||||
| .onSuccess(fetchedEntry -> processFetchedEntryWithDialog(fetchedEntry, field, fieldContent, entry, fetcher)) | ||||||
| .onFailure(exception -> handleFetchException(exception, fetcher, entry)) | ||||||
| .executeWith(taskExecutor); | ||||||
| } | ||||||
|
|
||||||
| private void processFetchedEntryWithDialog(Optional<BibEntry> fetchedEntry, Field field, String fieldContent, BibEntry originalEntry, IdBasedFetcher fetcher) { | ||||||
| ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); | ||||||
| if (fetchedEntry.isPresent()) { | ||||||
| cleanup.doPostCleanup(fetchedEntry.get()); | ||||||
| showMergeDialog(originalEntry, fetchedEntry.get(), fetcher); | ||||||
| } else { | ||||||
| dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), fieldContent)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| public void fetchAndMergeBatch(List<BibEntry> entries) { | ||||||
| entries.forEach(entry -> SUPPORTED_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); | ||||||
|
There was a problem hiding this comment. Rename variable |
||||||
| } | ||||||
|
|
||||||
| private void fetchAndMergeEntry(BibEntry entry, Field field) { | ||||||
| entry.getField(field).ifPresentOrElse( | ||||||
| fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()).ifPresent( | ||||||
| fetcher -> executeFetchTask(fetcher, field, fieldContent, entry) | ||||||
| ), | ||||||
| () -> { | ||||||
| if (hasAnySupportedField(entry)) { | ||||||
| dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); | ||||||
| } | ||||||
|
There was a problem hiding this comment. It is unclear why this function is there. I think, this can just be removed. No need to inform the user. Moreover, the logic is wrong. -- It is OK if the entry has an identifier field There was a problem hiding this comment. We also can use |
||||||
| } | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| private boolean hasAnySupportedField(BibEntry entry) { | ||||||
| return SUPPORTED_FIELDS.stream().noneMatch(field -> entry.getField(field).isPresent()); | ||||||
| } | ||||||
|
|
||||||
| private void executeFetchTask(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) { | ||||||
| BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) | ||||||
|
There was a problem hiding this comment. We don't need the |
||||||
| .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, field, fieldContent, entry)) | ||||||
| .onFailure(exception -> handleFetchException(exception, fetcher, entry)) | ||||||
| .executeWith(taskExecutor); | ||||||
| } | ||||||
|
|
||||||
| private void processFetchedEntry(Optional<BibEntry> fetchedEntry, Field field, String fieldContent, BibEntry originalEntry) { | ||||||
| if (fetchedEntry.isPresent()) { | ||||||
| ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); | ||||||
| cleanup.doPostCleanup(fetchedEntry.get()); | ||||||
| mergeWithoutDialog(originalEntry, fetchedEntry.get()); | ||||||
| } else { | ||||||
| dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), fieldContent)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private void handleFetchException(Exception exception, IdBasedFetcher fetcher, BibEntry entry) { | ||||||
| if (hasAnySupportedField(entry)) { | ||||||
|
There was a problem hiding this comment. I think, this is not required. The id fetcher should only be called if the id is present. If id present, the exception is shown. --> remove this if (and remove |
||||||
| LOGGER.error("Error while fetching bibliographic information", exception); | ||||||
| String fetcherName = fetcher.getName(); | ||||||
| if (exception instanceof FetcherClientException) { | ||||||
| dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcherName), Localization.lang("No data was found for the identifier")); | ||||||
| } else if (exception instanceof FetcherServerException) { | ||||||
| dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcherName), Localization.lang("Server not available")); | ||||||
| } else { | ||||||
| dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcherName), Localization.lang("Error occurred %0", exception.getMessage())); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { | ||||||
|
There was a problem hiding this comment. This whole method can be go into Annote the test using JUnit There was a problem hiding this comment. I apologize for the oversight; I should have provided my comments after pushing the changes. Over the past few hours, I have been working on optimizing the code further and transitioning the class into the logic directory. Refactor FetchAndMergeEntry: The single-entry fetching and merging logic remains in FetchAndMergeEntry. Extracted logic for batch processing and handling multiple entries from FetchAndMergeEntry. Introduce MergingIdBasedFetcher: Created the MergingIdBasedFetcher class in the org.jabref.logic.importer.fetcher package. MergingIdBasedFetcher handles mass operations such as fetching and merging multiple BibEntry objects. Uses NotificationService to provide error and status notifications, removing reliance on user confirmation dialogs for better automation. Create MergeEntriesHelper: Introduced the MergeEntriesHelper class to centralize and handle the actual merging logic between BibEntry instances. The class includes methods to update entry types, fields, and remove obsolete fields during merging. ''' |
||||||
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); | ||||||
|
There was a problem hiding this comment. Two entries are morged, not a single one.
Suggested change
|
||||||
|
|
||||||
| // Set of fields present in both the original and fetched entries | ||||||
| Set<Field> jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||||||
| jointFields.addAll(fetchedEntry.getFields()); | ||||||
| Set<Field> originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||||||
| originalFields.addAll(originalEntry.getFields()); | ||||||
|
There was a problem hiding this comment. This is duplicated coude. Please refactor to a method. |
||||||
|
|
||||||
| boolean edited = false; | ||||||
|
|
||||||
| // Compare entry type and update if needed | ||||||
| EntryType oldType = originalEntry.getType(); | ||||||
| EntryType newType = fetchedEntry.getType(); | ||||||
|
|
||||||
| if (!oldType.equals(newType)) { | ||||||
| originalEntry.setType(newType); | ||||||
| ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); | ||||||
| edited = true; | ||||||
|
There was a problem hiding this comment. I don't think we need the edited boolean flag anymore, we can use NamedCompound#hasEdits. |
||||||
| } | ||||||
|
|
||||||
| // Compare fields and set the longer value | ||||||
| for (Field field : jointFields) { | ||||||
| Optional<String> originalString = originalEntry.getField(field); | ||||||
| Optional<String> fetchedString = fetchedEntry.getField(field); | ||||||
|
|
||||||
| if (fetchedString.isPresent()) { | ||||||
|
There was a problem hiding this comment. Let's reduce nesting and return early here. |
||||||
| if (originalString.isEmpty() || fetchedString.get().length() > originalString.get().length()) { | ||||||
| originalEntry.setField(field, fetchedString.get()); | ||||||
| ce.addEdit(new UndoableFieldChange(originalEntry, field, originalString.orElse(null), fetchedString.get())); | ||||||
| edited = true; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Remove fields not present in fetched entry, unless they are internal | ||||||
| edited = isEdited(originalEntry, ce, jointFields, originalFields, edited); | ||||||
|
|
||||||
| if (edited) { | ||||||
| ce.end(); | ||||||
| undoManager.addEdit(ce); | ||||||
| dialogService.notify(Localization.lang("Updated entry with fetched information")); | ||||||
|
There was a problem hiding this comment. Add the citation key to this information - otherwise it is unclear which entry is used. Use |
||||||
| } else { | ||||||
| dialogService.notify(Localization.lang("No new information was added")); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private boolean isEdited(BibEntry originalEntry, NamedCompound ce, Set<Field> jointFields, Set<Field> originalFields, boolean edited) { | ||||||
| for (Field field : originalFields) { | ||||||
|
There was a problem hiding this comment. Do we need to pass the edited boolean here? We want to minimize the number of parameters. There was a problem hiding this comment. The method is named |
||||||
| if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { | ||||||
| Optional<String> originalString = originalEntry.getField(field); | ||||||
| originalEntry.clearField(field); | ||||||
| ce.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); | ||||||
| edited = true; | ||||||
| } | ||||||
| } | ||||||
| return edited; | ||||||
| } | ||||||
|
|
||||||
| private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { | ||||||
|
|
@@ -144,14 +249,7 @@ private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebF | |||||
| } | ||||||
|
|
||||||
| // Remove fields which are not in the merged entry, unless they are internal fields | ||||||
| edited = isEdited(originalEntry, ce, jointFields, originalFields, edited); | ||||||
|
|
||||||
| if (edited) { | ||||||
| ce.end(); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package org.jabref.gui.mergeentries; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import javax.swing.undo.UndoManager; | ||
|
|
||
| import org.jabref.gui.DialogService; | ||
| import org.jabref.gui.LibraryTab; | ||
| import org.jabref.gui.actions.SimpleCommand; | ||
| import org.jabref.gui.preferences.GuiPreferences; | ||
| import org.jabref.logic.util.TaskExecutor; | ||
| import org.jabref.model.database.BibDatabaseContext; | ||
| import org.jabref.model.entry.BibEntry; | ||
|
|
||
| /** | ||
| * Action for merging multiple entries with fetched data from identifiers like DOI or arXiv. | ||
| */ | ||
| public class MultiEntryMergeWithFetchedDataAction extends SimpleCommand { | ||
|
|
||
| private final LibraryTab libraryTab; | ||
| private final GuiPreferences preferences; | ||
| private final TaskExecutor taskExecutor; | ||
| private final BibDatabaseContext bibDatabaseContext; | ||
| private final DialogService dialogService; | ||
| private final UndoManager undoManager; | ||
|
|
||
| public MultiEntryMergeWithFetchedDataAction(LibraryTab libraryTab, | ||
| GuiPreferences preferences, | ||
| TaskExecutor taskExecutor, | ||
| BibDatabaseContext bibDatabaseContext, | ||
| DialogService dialogService, | ||
| UndoManager undoManager) { | ||
| this.libraryTab = libraryTab; | ||
| this.preferences = preferences; | ||
| this.taskExecutor = taskExecutor; | ||
| this.bibDatabaseContext = bibDatabaseContext; | ||
| this.dialogService = dialogService; | ||
| this.undoManager = undoManager; | ||
| } | ||
|
|
||
| @Override | ||
| public void execute() { | ||
| List<BibEntry> selectedEntries = libraryTab.getSelectedEntries(); | ||
|
|
||
| // Create an instance of FetchAndMergeEntry | ||
|
There was a problem hiding this comment. Not a very useful comment 😅 |
||
| FetchAndMergeEntry fetchAndMergeEntry = new FetchAndMergeEntry(bibDatabaseContext, taskExecutor, preferences, dialogService, undoManager); | ||
|
|
||
| // Perform the batch fetch and merge operation | ||
| fetchAndMergeEntry.fetchAndMergeBatch(selectedEntries); | ||
| } | ||
| } | ||
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.
Please add where you added it.
It really needs to be in the "Lookup" menu.
We want to keep the right click menu small.
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.