From 887dd35c4c8083a56c22db06870fb4f72d43fee1 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 19 Oct 2024 15:00:42 +1100 Subject: [PATCH 01/28] feat: enhance right-click context menu with new actions (extract references, attach file, send entries, special fields) and improved layout --- .../jabref/gui/maintable/RightClickMenu.java | 4 +- .../gui/mergeentries/FetchAndMergeEntry.java | 104 ++++++++++++++++-- .../MultiEntryMergeWithFetchedDataAction.java | 51 +++++++++ 3 files changed, 149 insertions(+), 10 deletions(-) create mode 100644 src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java diff --git a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java index 66430e8770e..dc25aa166b7 100644 --- a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java +++ b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java @@ -23,7 +23,7 @@ import org.jabref.gui.linkedfile.AttachFileFromURLAction; import org.jabref.gui.menus.ChangeEntryTypeMenu; import org.jabref.gui.mergeentries.MergeEntriesAction; -import org.jabref.gui.mergeentries.MergeWithFetchedEntryAction; +import org.jabref.gui.mergeentries.MultiEntryMergeWithFetchedDataAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preview.CopyCitationAction; import org.jabref.gui.preview.PreviewPreferences; @@ -92,7 +92,7 @@ public static ContextMenu create(BibEntryTableViewModel entry, new SeparatorMenuItem(), new ChangeEntryTypeMenu(libraryTab.getSelectedEntries(), libraryTab.getBibDatabaseContext(), undoManager, entryTypesManager).asSubMenu(), - factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)) + factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MultiEntryMergeWithFetchedDataAction(libraryTab, preferences, taskExecutor, libraryTab.getBibDatabaseContext(), dialogService, undoManager)) ); EasyBind.subscribe(preferences.getGrobidPreferences().grobidEnabledProperty(), enabled -> { diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index dbe205a24c4..daeec47bdcf 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -104,6 +104,101 @@ public void fetchAndMerge(BibEntry entry, List fields) { } } + public void fetchAndMergeBatch(List entries) { + for (BibEntry entry : entries) { + for (Field field : SUPPORTED_FIELDS) { + Optional fieldContent = entry.getField(field); + if (fieldContent.isPresent()) { + Optional fetcher = WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()); + if (fetcher.isPresent()) { + BackgroundTask.wrap(() -> fetcher.get().performSearchById(fieldContent.get())) + .onSuccess(fetchedEntry -> { + if (fetchedEntry.isPresent()) { + ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); + cleanup.doPostCleanup(fetchedEntry.get()); + mergeWithoutDialog(entry, fetchedEntry.get()); + } else { + dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), fieldContent.get())); + } + }) + .onFailure(exception -> { + LOGGER.error("Error while fetching bibliographic information", exception); + if (exception instanceof FetcherClientException) { + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("No data was found for the identifier")); + } else if (exception instanceof FetcherServerException) { + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Server not available")); + } else { + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Error occurred %0", exception.getMessage())); + } + }) + .executeWith(taskExecutor); + } + } else { + dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); + } + } + } + } + + private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { + NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); + + // Set of fields present in both the original and fetched entries + Set jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); + jointFields.addAll(fetchedEntry.getFields()); + Set originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); + originalFields.addAll(originalEntry.getFields()); + + 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; + } + + // Compare fields and set the longer value + for (Field field : jointFields) { + Optional originalString = originalEntry.getField(field); + Optional fetchedString = fetchedEntry.getField(field); + + if (fetchedString.isPresent()) { + 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")); + } else { + dialogService.notify(Localization.lang("No new information was added")); + } + } + + private boolean isEdited(BibEntry originalEntry, NamedCompound ce, Set jointFields, Set originalFields, boolean edited) { + for (Field field : originalFields) { + if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { + Optional 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) { MergeEntriesDialog dialog = new MergeEntriesDialog(originalEntry, fetchedEntry, preferences); dialog.setTitle(Localization.lang("Merge entry with %0 information", fetcher.getName())); @@ -144,14 +239,7 @@ private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebF } // 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 originalString = originalEntry.getField(field); - originalEntry.clearField(field); - ce.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); // originalString always present - edited = true; - } - } + edited = isEdited(originalEntry, ce, jointFields, originalFields, edited); if (edited) { ce.end(); diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java new file mode 100644 index 00000000000..679ac5d3da8 --- /dev/null +++ b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java @@ -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 selectedEntries = libraryTab.getSelectedEntries(); + + // Create an instance of FetchAndMergeEntry + FetchAndMergeEntry fetchAndMergeEntry = new FetchAndMergeEntry(bibDatabaseContext, taskExecutor, preferences, dialogService, undoManager); + + // Perform the batch fetch and merge operation + fetchAndMergeEntry.fetchAndMergeBatch(selectedEntries); + } +} From 656facfb54448f1cf984a69f1f04d028b07430c3 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 19 Oct 2024 15:54:53 +1100 Subject: [PATCH 02/28] fix: Restore MERGE_WITH_FETCHED_ENTRY and add BATCH_MERGE_WITH_FETCHED_ENTRIES - Reintroduce previously removed MERGE_WITH_FETCHED_ENTRY action - Add new BATCH_MERGE_WITH_FETCHED_ENTRIES action after it - Fixes unintended removal of single-entry merge functionality --- src/main/java/org/jabref/gui/actions/StandardActions.java | 1 + src/main/java/org/jabref/gui/maintable/RightClickMenu.java | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/actions/StandardActions.java b/src/main/java/org/jabref/gui/actions/StandardActions.java index df789a51733..87e37b5134c 100644 --- a/src/main/java/org/jabref/gui/actions/StandardActions.java +++ b/src/main/java/org/jabref/gui/actions/StandardActions.java @@ -37,6 +37,7 @@ public enum StandardActions implements Action { OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI), SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")), MERGE_WITH_FETCHED_ENTRY(Localization.lang("Get bibliographic data from %0", "DOI/ISBN/...")), + BATCH_MERGE_WITH_FETCHED_ENTRIES(Localization.lang("Mass getting bibliographic data from %0", "DOI/ISBN/...")), ATTACH_FILE(Localization.lang("Attach file"), IconTheme.JabRefIcons.ATTACH_FILE), ATTACH_FILE_FROM_URL(Localization.lang("Attach file from URL"), IconTheme.JabRefIcons.DOWNLOAD_FILE), PRIORITY(Localization.lang("Priority"), IconTheme.JabRefIcons.PRIORITY), diff --git a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java index dc25aa166b7..5a16caa363d 100644 --- a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java +++ b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java @@ -23,6 +23,7 @@ import org.jabref.gui.linkedfile.AttachFileFromURLAction; import org.jabref.gui.menus.ChangeEntryTypeMenu; import org.jabref.gui.mergeentries.MergeEntriesAction; +import org.jabref.gui.mergeentries.MergeWithFetchedEntryAction; import org.jabref.gui.mergeentries.MultiEntryMergeWithFetchedDataAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preview.CopyCitationAction; @@ -92,7 +93,8 @@ public static ContextMenu create(BibEntryTableViewModel entry, new SeparatorMenuItem(), new ChangeEntryTypeMenu(libraryTab.getSelectedEntries(), libraryTab.getBibDatabaseContext(), undoManager, entryTypesManager).asSubMenu(), - factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MultiEntryMergeWithFetchedDataAction(libraryTab, preferences, taskExecutor, libraryTab.getBibDatabaseContext(), dialogService, undoManager)) + factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)), + factory.createMenuItem(StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRIES, new MultiEntryMergeWithFetchedDataAction(libraryTab, preferences, taskExecutor, libraryTab.getBibDatabaseContext(), dialogService, undoManager)) ); EasyBind.subscribe(preferences.getGrobidPreferences().grobidEnabledProperty(), enabled -> { From 0409856094ed03a28cd3f914ef5e801a4c01ab7c Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 19 Oct 2024 17:07:07 +1100 Subject: [PATCH 03/28] Refactor and optimize FetchAndMergeEntry class - Improve code structure and reduce duplication - Enhance error handling and user feedback - Streamline fetch and merge process for better performance - Implement more robust field handling and merging logic - Optimize batch processing capabilities - Refactor to use Java 8+ features for cleaner code - Improve modularity and separation of concerns - Reduce unnecessary notifications --- .../gui/mergeentries/FetchAndMergeEntry.java | 130 ++++++++++-------- 1 file changed, 70 insertions(+), 60 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index daeec47bdcf..4e6e7f3b0ff 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -70,72 +70,82 @@ public void fetchAndMerge(BibEntry entry, Field field) { } public void fetchAndMerge(BibEntry entry, List fields) { - for (Field field : fields) { - Optional fieldContent = entry.getField(field); - if (fieldContent.isPresent()) { - Optional fetcher = WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()); - if (fetcher.isPresent()) { - BackgroundTask.wrap(() -> fetcher.get().performSearchById(fieldContent.get())) - .onSuccess(fetchedEntry -> { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - String type = field.getDisplayName(); - if (fetchedEntry.isPresent()) { - cleanup.doPostCleanup(fetchedEntry.get()); - showMergeDialog(entry, fetchedEntry.get(), fetcher.get()); - } else { - dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", type, fieldContent.get())); - } - }) - .onFailure(exception -> { - LOGGER.error("Error while fetching bibliographic information", exception); - if (exception instanceof FetcherClientException) { - dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("No data was found for the identifier")); - } else if (exception instanceof FetcherServerException) { - dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Server not available")); - } else { - dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Error occurred %0", exception.getMessage())); - } - }) - .executeWith(taskExecutor); - } - } else { - dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); - } + 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 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 entries) { - for (BibEntry entry : entries) { - for (Field field : SUPPORTED_FIELDS) { - Optional fieldContent = entry.getField(field); - if (fieldContent.isPresent()) { - Optional fetcher = WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()); - if (fetcher.isPresent()) { - BackgroundTask.wrap(() -> fetcher.get().performSearchById(fieldContent.get())) - .onSuccess(fetchedEntry -> { - if (fetchedEntry.isPresent()) { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - cleanup.doPostCleanup(fetchedEntry.get()); - mergeWithoutDialog(entry, fetchedEntry.get()); - } else { - dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), fieldContent.get())); - } - }) - .onFailure(exception -> { - LOGGER.error("Error while fetching bibliographic information", exception); - if (exception instanceof FetcherClientException) { - dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("No data was found for the identifier")); - } else if (exception instanceof FetcherServerException) { - dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Server not available")); - } else { - dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Error occurred %0", exception.getMessage())); - } - }) - .executeWith(taskExecutor); + entries.forEach(entry -> SUPPORTED_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); + } + + 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())); } - } else { - dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); } + ); + } + + 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)) + .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, field, fieldContent, entry)) + .onFailure(exception -> handleFetchException(exception, fetcher, entry)) + .executeWith(taskExecutor); + } + + private void processFetchedEntry(Optional 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)) { + 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())); } } } From dc138ce4a8a5b1e93d41bd63ea4a9ab1a0f181df Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 19 Oct 2024 18:17:02 +1100 Subject: [PATCH 04/28] Updated CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 789d8ec6968..5bbdc8aff61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) +- Added functionality for mass addition of bibliographic information for multiple entries [#372](https://github.com/JabRef/jabref/issues/372) ### Changed From 0532df4a64d57a43d454678d943189fb9581a78c Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 19 Oct 2024 18:33:10 +1100 Subject: [PATCH 05/28] Added a period at the end of the description in CHANGELOG.md. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bbdc8aff61..63ababbab73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) -- Added functionality for mass addition of bibliographic information for multiple entries [#372](https://github.com/JabRef/jabref/issues/372) +- Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) ### Changed From 012dd9ab2548262a8fb01b7dd98f6bc939f09af9 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 20 Oct 2024 00:03:16 +1100 Subject: [PATCH 06/28] Updated JabRef_en.properties --- src/main/resources/l10n/JabRef_en.properties | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index edda02d9e94..71679afcd14 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1679,6 +1679,10 @@ No\ %0\ found=No %0 found Entry\ from\ %0=Entry from %0 Merge\ entry\ with\ %0\ information=Merge entry with %0 information Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 +Mass\ getting\ bibliographic\ data\ from\ %0=Mass getting bibliographic data from %0 +Merge\ entry\ without\ user\ interaction=Merge entry without user interaction +No\ new\ information\ was\ added=No new information was added +Updated\ entry\ with\ fetched\ information=Updated entry with fetched information Add\ new\ list=Add new list Open\ existing\ list=Open existing list From f8267acdf3740ec3b6747a38250175090226bece Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 20 Oct 2024 14:58:49 +1100 Subject: [PATCH 07/28] Move and rename "Mass Getting bibliographic data" action - Relocate action from Right-click menu to "Lookup" menu for consistency - Rename action from "Mass Getting" to "Get bibliographic data from DOI/ISBN/... (fully automated)" - Other modifications not finished yet --- .../jabref/gui/actions/StandardActions.java | 2 +- .../java/org/jabref/gui/frame/MainMenu.java | 18 +++++++- .../jabref/gui/maintable/RightClickMenu.java | 4 +- .../MultiEntryMergeWithFetchedDataAction.java | 43 ++++++++++++++----- 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/jabref/gui/actions/StandardActions.java b/src/main/java/org/jabref/gui/actions/StandardActions.java index 87e37b5134c..096406bd75c 100644 --- a/src/main/java/org/jabref/gui/actions/StandardActions.java +++ b/src/main/java/org/jabref/gui/actions/StandardActions.java @@ -37,7 +37,7 @@ public enum StandardActions implements Action { OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI), SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")), MERGE_WITH_FETCHED_ENTRY(Localization.lang("Get bibliographic data from %0", "DOI/ISBN/...")), - BATCH_MERGE_WITH_FETCHED_ENTRIES(Localization.lang("Mass getting bibliographic data from %0", "DOI/ISBN/...")), + MASS_GET_BIBLIOGRAPHIC_DATA(Localization.lang("Get bibliographic data from %0 (fully automated)", "DOI/ISBN/...")), ATTACH_FILE(Localization.lang("Attach file"), IconTheme.JabRefIcons.ATTACH_FILE), ATTACH_FILE_FROM_URL(Localization.lang("Attach file from URL"), IconTheme.JabRefIcons.DOWNLOAD_FILE), PRIORITY(Localization.lang("Priority"), IconTheme.JabRefIcons.PRIORITY), diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java index 4a90a556172..a02c6f91f3f 100644 --- a/src/main/java/org/jabref/gui/frame/MainMenu.java +++ b/src/main/java/org/jabref/gui/frame/MainMenu.java @@ -52,6 +52,7 @@ import org.jabref.gui.maintable.NewLibraryFromPdfActionOffline; import org.jabref.gui.maintable.NewLibraryFromPdfActionOnline; import org.jabref.gui.mergeentries.MergeEntriesAction; +import org.jabref.gui.mergeentries.MultiEntryMergeWithFetchedDataAction; import org.jabref.gui.plaincitationparser.PlainCitationParserAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preferences.ShowPreferencesAction; @@ -273,7 +274,22 @@ private void createMenu() { new SeparatorMenuItem(), - factory.createMenuItem(StandardActions.FIND_UNLINKED_FILES, new FindUnlinkedFilesAction(dialogService, stateManager)) + factory.createMenuItem(StandardActions.FIND_UNLINKED_FILES, new FindUnlinkedFilesAction(dialogService, stateManager)), + + new SeparatorMenuItem(), + + // Adding new menu item for mass bibliographic data fetching + factory.createMenuItem( + StandardActions.MASS_GET_BIBLIOGRAPHIC_DATA, + new MultiEntryMergeWithFetchedDataAction( + frame::getCurrentLibraryTab, + preferences, + taskExecutor, + dialogService, + undoManager, + stateManager + ) + ) ); final MenuItem pushToApplicationMenuItem = factory.createMenuItem(pushToApplicationCommand.getAction(), pushToApplicationCommand); diff --git a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java index 5a16caa363d..66430e8770e 100644 --- a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java +++ b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java @@ -24,7 +24,6 @@ import org.jabref.gui.menus.ChangeEntryTypeMenu; import org.jabref.gui.mergeentries.MergeEntriesAction; import org.jabref.gui.mergeentries.MergeWithFetchedEntryAction; -import org.jabref.gui.mergeentries.MultiEntryMergeWithFetchedDataAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preview.CopyCitationAction; import org.jabref.gui.preview.PreviewPreferences; @@ -93,8 +92,7 @@ public static ContextMenu create(BibEntryTableViewModel entry, new SeparatorMenuItem(), new ChangeEntryTypeMenu(libraryTab.getSelectedEntries(), libraryTab.getBibDatabaseContext(), undoManager, entryTypesManager).asSubMenu(), - factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)), - factory.createMenuItem(StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRIES, new MultiEntryMergeWithFetchedDataAction(libraryTab, preferences, taskExecutor, libraryTab.getBibDatabaseContext(), dialogService, undoManager)) + factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)) ); EasyBind.subscribe(preferences.getGrobidPreferences().grobidEnabledProperty(), enabled -> { diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java index 679ac5d3da8..1d842ba7763 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java @@ -1,51 +1,74 @@ package org.jabref.gui.mergeentries; import java.util.List; +import java.util.function.Supplier; import javax.swing.undo.UndoManager; import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; +import org.jabref.gui.StateManager; +import org.jabref.gui.actions.ActionHelper; 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; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * Action for merging multiple entries with fetched data from identifiers like DOI or arXiv. */ public class MultiEntryMergeWithFetchedDataAction extends SimpleCommand { - private final LibraryTab libraryTab; + private static final Logger LOGGER = LoggerFactory.getLogger(MultiEntryMergeWithFetchedDataAction.class); + + private final Supplier tabSupplier; private final GuiPreferences preferences; private final TaskExecutor taskExecutor; - private final BibDatabaseContext bibDatabaseContext; private final DialogService dialogService; private final UndoManager undoManager; - public MultiEntryMergeWithFetchedDataAction(LibraryTab libraryTab, + public MultiEntryMergeWithFetchedDataAction(Supplier tabSupplier, GuiPreferences preferences, TaskExecutor taskExecutor, - BibDatabaseContext bibDatabaseContext, DialogService dialogService, - UndoManager undoManager) { - this.libraryTab = libraryTab; + UndoManager undoManager, + StateManager stateManager) { + this.tabSupplier = tabSupplier; this.preferences = preferences; this.taskExecutor = taskExecutor; - this.bibDatabaseContext = bibDatabaseContext; this.dialogService = dialogService; this.undoManager = undoManager; + + // Binding to only execute if a database is open + this.executable.bind(ActionHelper.needsDatabase(stateManager)); } @Override public void execute() { - List selectedEntries = libraryTab.getSelectedEntries(); + LibraryTab libraryTab = tabSupplier.get(); + + // Ensure that libraryTab is present + if (libraryTab == null) { + LOGGER.error("Action 'Multi Entry Merge' must be disabled when no database is open."); + return; + } + + List selectedEntries = libraryTab.getDatabase().getEntries(); + + // If no entries are selected, log and do not execute + if (selectedEntries.isEmpty()) { + dialogService.notify("No entries selected for merging."); + return; + } - // Create an instance of FetchAndMergeEntry + // Create an instance of FetchAndMergeEntry to perform the batch fetch and merge operation + BibDatabaseContext bibDatabaseContext = libraryTab.getBibDatabaseContext(); FetchAndMergeEntry fetchAndMergeEntry = new FetchAndMergeEntry(bibDatabaseContext, taskExecutor, preferences, dialogService, undoManager); - // Perform the batch fetch and merge operation fetchAndMergeEntry.fetchAndMergeBatch(selectedEntries); } } From 74f2473cfc388fef47bfb1cda4fe0cd9932d12cf Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 20 Oct 2024 16:19:46 +1100 Subject: [PATCH 08/28] Refactor FetchAndMergeEntry class for improved functionality and readability - Rename SUPPORTED_FIELDS to SUPPORTED_IDENTIFIER_FIELDS for clarity - Remove unnecessary hasAnySupportedField method and associated checks - Refactor duplicate code into utility methods (getFields, getJointFields) - Improve error handling with more specific exception messages - Remove edited boolean flag in favor of NamedCompound#hasEdits() - Enhance use of Optional, Stream, and method references - Reduce method parameters for better maintainability - Improve user notifications with entry citation keys - Restructure methods to separate concerns more effectively Minor modifications to MergeWithFetchedEntryAction.java to align with the new version of FetchAndMergeEntry.java --- .../gui/mergeentries/FetchAndMergeEntry.java | 307 +++++++++--------- .../MergeWithFetchedEntryAction.java | 12 +- 2 files changed, 164 insertions(+), 155 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index 4e6e7f3b0ff..e1fe383b26c 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -1,12 +1,13 @@ + package org.jabref.gui.mergeentries; import java.util.Arrays; -import java.util.Collections; import java.util.Comparator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.TreeSet; +import java.util.stream.Collectors; import javax.swing.undo.UndoManager; @@ -40,7 +41,7 @@ */ public class FetchAndMergeEntry { - public static List SUPPORTED_FIELDS = Arrays.asList(StandardField.DOI, StandardField.EPRINT, StandardField.ISBN); + public static final List SUPPORTED_IDENTIFIER_FIELDS = Arrays.asList(StandardField.DOI, StandardField.EPRINT, StandardField.ISBN); private static final Logger LOGGER = LoggerFactory.getLogger(FetchAndMergeEntry.class); private final DialogService dialogService; @@ -62,11 +63,11 @@ public FetchAndMergeEntry(BibDatabaseContext bibDatabaseContext, } public void fetchAndMerge(BibEntry entry) { - fetchAndMerge(entry, SUPPORTED_FIELDS); + fetchAndMerge(entry, SUPPORTED_IDENTIFIER_FIELDS); } public void fetchAndMerge(BibEntry entry, Field field) { - fetchAndMerge(entry, Collections.singletonList(field)); + fetchAndMerge(entry, List.of(field)); } public void fetchAndMerge(BibEntry entry, List fields) { @@ -74,206 +75,206 @@ public void fetchAndMerge(BibEntry entry, List fields) { } private void fetchAndMergeEntryWithDialog(BibEntry entry, Field field) { - entry.getField(field).ifPresent( - fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()).ifPresent( - fetcher -> executeFetchTaskWithDialog(fetcher, field, fieldContent, entry) - ) - ); + entry.getField(field) + .flatMap(fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences())) + .ifPresent(fetcher -> executeFetchTaskWithDialog(fetcher, field, 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 executeFetchTaskWithDialog(IdBasedFetcher fetcher, Field field, BibEntry entry) { + entry.getField(field) + .ifPresent(fieldContent -> + BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) + .onSuccess(fetchedEntry -> processFetchedEntryWithDialog(fetchedEntry, field, entry, fetcher)) + .onFailure(exception -> handleFetchException(exception, fetcher)) + .executeWith(taskExecutor) + ); } - private void processFetchedEntryWithDialog(Optional fetchedEntry, Field field, String fieldContent, BibEntry originalEntry, IdBasedFetcher fetcher) { + private void processFetchedEntryWithDialog(Optional fetchedEntry, Field field, 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)); - } + fetchedEntry.ifPresentOrElse( + entry -> { + cleanup.doPostCleanup(entry); + showMergeDialog(originalEntry, entry, fetcher); + }, + () -> dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), originalEntry.getField(field).orElse(""))) + ); } public void fetchAndMergeBatch(List entries) { - entries.forEach(entry -> SUPPORTED_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); + entries.forEach(entry -> SUPPORTED_IDENTIFIER_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); } 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())); - } - } - ); + entry.getField(field) + .flatMap(fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences())) + .ifPresent(fetcher -> executeFetchTask(fetcher, field, entry)); } - private boolean hasAnySupportedField(BibEntry entry) { - return SUPPORTED_FIELDS.stream().noneMatch(field -> entry.getField(field).isPresent()); + private void executeFetchTask(IdBasedFetcher fetcher, Field field, BibEntry entry) { + entry.getField(field) + .ifPresent(fieldContent -> BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) + .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, field, entry)) + .onFailure(exception -> handleFetchException(exception, fetcher)) + .executeWith(taskExecutor)); } - private void executeFetchTask(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) { - BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) - .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, field, fieldContent, entry)) - .onFailure(exception -> handleFetchException(exception, fetcher, entry)) - .executeWith(taskExecutor); + private void processFetchedEntry(Optional fetchedEntry, Field field, BibEntry originalEntry) { + ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); + fetchedEntry.ifPresentOrElse( + entry -> { + cleanup.doPostCleanup(entry); + mergeWithoutDialog(originalEntry, entry); + }, + // Notify if no entry was fetched + () -> dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), originalEntry.getField(field).orElse(""))) + ); } - private void processFetchedEntry(Optional 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()); + private void handleFetchException(Exception exception, IdBasedFetcher fetcher) { + LOGGER.error("Error while fetching bibliographic information", exception); + String fetcherName = fetcher.getName(); + // Handle different types of exceptions with specific error messages + 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.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)) { - 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())); - } + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcherName), Localization.lang("Error occurred %0", exception.getMessage())); } } private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); - // Set of fields present in both the original and fetched entries - Set jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); - jointFields.addAll(fetchedEntry.getFields()); - Set originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); - originalFields.addAll(originalEntry.getFields()); + updateEntryTypeIfDifferent(originalEntry, fetchedEntry, ce); + updateFieldsWithNewInfo(originalEntry, fetchedEntry, ce); + removeObsoleteFields(originalEntry, fetchedEntry, ce); - boolean edited = false; - - // Compare entry type and update if needed - EntryType oldType = originalEntry.getType(); - EntryType newType = fetchedEntry.getType(); + finalizeMerge(ce, originalEntry); + } - if (!oldType.equals(newType)) { - originalEntry.setType(newType); - ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); - edited = true; + private void updateFieldsWithNewInfo(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + Set jointFields = getJointFields(originalEntry, fetchedEntry); + for (Field field : jointFields) { + updateFieldIfNecessary(originalEntry, fetchedEntry, field, ce); } + } - // Compare fields and set the longer value - for (Field field : jointFields) { - Optional originalString = originalEntry.getField(field); - Optional fetchedString = fetchedEntry.getField(field); - - if (fetchedString.isPresent()) { - 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; - } + private void updateFieldIfNecessary(BibEntry originalEntry, BibEntry fetchedEntry, Field field, NamedCompound ce) { + fetchedEntry.getField(field).ifPresent(fetchedValue -> { + Optional originalValue = originalEntry.getField(field); + if (originalValue.isEmpty() || fetchedValue.length() > originalValue.get().length()) { + originalEntry.setField(field, fetchedValue); + ce.addEdit(new UndoableFieldChange(originalEntry, field, originalValue.orElse(null), fetchedValue)); + } + }); + } + + private void removeObsoleteFields(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + Set jointFields = getJointFields(originalEntry, fetchedEntry); + Set originalFields = getFields(originalEntry); + + for (Field field : originalFields) { + if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { + removeField(originalEntry, field, ce); } } + } - // Remove fields not present in fetched entry, unless they are internal - edited = isEdited(originalEntry, ce, jointFields, originalFields, edited); + private void removeField(BibEntry entry, Field field, NamedCompound ce) { + Optional originalValue = entry.getField(field); + entry.clearField(field); + ce.addEdit(new UndoableFieldChange(entry, field, originalValue.orElse(null), null)); + } + + private void finalizeMerge(NamedCompound ce, BibEntry entry) { + String citationKey = entry.getCitationKey().orElse(entry.getAuthorTitleYear(40)); + String message = ce.hasEdits() + ? Localization.lang("Updated entry with fetched information [%0]", citationKey) + : Localization.lang("No new information was added [%0]", citationKey); - if (edited) { + if (ce.hasEdits()) { ce.end(); undoManager.addEdit(ce); - dialogService.notify(Localization.lang("Updated entry with fetched information")); - } else { - dialogService.notify(Localization.lang("No new information was added")); } + dialogService.notify(message); } - private boolean isEdited(BibEntry originalEntry, NamedCompound ce, Set jointFields, Set originalFields, boolean edited) { - for (Field field : originalFields) { - if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { - Optional originalString = originalEntry.getField(field); - originalEntry.clearField(field); - ce.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); - edited = true; - } + private Set getFields(BibEntry entry) { + // Get sorted set of fields for consistent ordering + return entry.getFields().stream() + .sorted(Comparator.comparing(Field::getName)) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + + private Set getJointFields(BibEntry entry1, BibEntry entry2) { + Set fields = new LinkedHashSet<>(); + fields.addAll(getFields(entry1)); + fields.addAll(getFields(entry2)); + return fields; + } + + private void updateEntryTypeIfDifferent(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + EntryType oldType = originalEntry.getType(); + EntryType newType = fetchedEntry.getType(); + + if (!oldType.equals(newType)) { + originalEntry.setType(newType); + ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); } - return edited; } private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { + MergeEntriesDialog dialog = createMergeDialog(originalEntry, fetchedEntry, fetcher); + Optional mergedEntry = showDialogAndGetResult(dialog); + + mergedEntry.ifPresentOrElse( + entry -> processMergedEntry(originalEntry, entry, fetcher), + () -> notifyCanceledMerge(originalEntry) + ); + } + + private MergeEntriesDialog createMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { MergeEntriesDialog dialog = new MergeEntriesDialog(originalEntry, fetchedEntry, preferences); dialog.setTitle(Localization.lang("Merge entry with %0 information", fetcher.getName())); dialog.setLeftHeaderText(Localization.lang("Original entry")); dialog.setRightHeaderText(Localization.lang("Entry from %0", fetcher.getName())); - Optional mergedEntry = dialogService.showCustomDialogAndWait(dialog).map(EntriesMergeResult::mergedEntry); - - if (mergedEntry.isPresent()) { - NamedCompound ce = new NamedCompound(Localization.lang("Merge entry with %0 information", fetcher.getName())); - - // Updated the original entry with the new fields - Set jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); - jointFields.addAll(mergedEntry.get().getFields()); - Set 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); - ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); - edited = true; - } + return dialog; + } - // fields - for (Field field : jointFields) { - Optional originalString = originalEntry.getField(field); - Optional mergedString = mergedEntry.get().getField(field); - if (originalString.isEmpty() || !originalString.equals(mergedString)) { - originalEntry.setField(field, mergedString.get()); // mergedString always present - ce.addEdit(new UndoableFieldChange(originalEntry, field, originalString.orElse(null), - mergedString.get())); - edited = true; - } - } + private Optional showDialogAndGetResult(MergeEntriesDialog dialog) { + return dialogService.showCustomDialogAndWait(dialog) + .map(EntriesMergeResult::mergedEntry); + } - // Remove fields which are not in the merged entry, unless they are internal fields - edited = isEdited(originalEntry, ce, jointFields, originalFields, edited); + private void processMergedEntry(BibEntry originalEntry, BibEntry mergedEntry, WebFetcher fetcher) { + NamedCompound ce = new NamedCompound(Localization.lang("Merge entry with %0 information", fetcher.getName())); - if (edited) { - ce.end(); - undoManager.addEdit(ce); - dialogService.notify(Localization.lang("Updated entry with info from %0", fetcher.getName())); - } else { - dialogService.notify(Localization.lang("No information added")); - } - } else { - dialogService.notify(Localization.lang("Canceled merging entries")); - } + updateEntryTypeIfDifferent(originalEntry, mergedEntry, ce); + updateFieldsWithNewInfo(originalEntry, mergedEntry, ce); + removeObsoleteFields(originalEntry, mergedEntry, ce); + + finalizeMerge(ce, originalEntry); + } + + private void notifyCanceledMerge(BibEntry entry) { + String citationKey = entry.getCitationKey().orElse(entry.getAuthorTitleYear(40)); + dialogService.notify(Localization.lang("Canceled merging entries") + " [" + citationKey + "]"); } public void fetchAndMerge(BibEntry entry, EntryBasedFetcher fetcher) { BackgroundTask.wrap(() -> fetcher.performSearch(entry).stream().findFirst()) - .onSuccess(fetchedEntry -> { - if (fetchedEntry.isPresent()) { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - cleanup.doPostCleanup(fetchedEntry.get()); - showMergeDialog(entry, fetchedEntry.get(), fetcher); - } else { - dialogService.notify(Localization.lang("Could not find any bibliographic information.")); - } - }) + .onSuccess(fetchedEntry -> fetchedEntry + .map(fe -> { + ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); + cleanup.doPostCleanup(fe); + return fe; + }) + .ifPresentOrElse( + fe -> showMergeDialog(entry, fe, fetcher), + () -> dialogService.notify(Localization.lang("Could not find any bibliographic information.")) + )) .onFailure(exception -> { LOGGER.error("Error while fetching entry with {} ", fetcher.getName(), exception); dialogService.showErrorDialogAndWait(Localization.lang("Error while fetching from %0", fetcher.getName()), exception); diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java b/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java index 743b0aac900..a6d8d65a406 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java @@ -33,7 +33,7 @@ public MergeWithFetchedEntryAction(DialogService dialogService, this.undoManager = undoManager; this.executable.bind(ActionHelper.needsEntriesSelected(1, stateManager) - .and(ActionHelper.isAnyFieldSetForSelectedEntry(FetchAndMergeEntry.SUPPORTED_FIELDS, stateManager))); + .and(ActionHelper.isAnyFieldSetForSelectedEntry(FetchAndMergeEntry.SUPPORTED_IDENTIFIER_FIELDS, stateManager))); } @Override @@ -49,6 +49,14 @@ public void execute() { } BibEntry originalEntry = stateManager.getSelectedEntries().getFirst(); - new FetchAndMergeEntry(stateManager.getActiveDatabase().get(), taskExecutor, preferences, dialogService, undoManager).fetchAndMerge(originalEntry); + FetchAndMergeEntry fetchAndMergeEntry = new FetchAndMergeEntry( + stateManager.getActiveDatabase().get(), + taskExecutor, + preferences, + dialogService, + undoManager + ); + + fetchAndMergeEntry.fetchAndMerge(originalEntry); } } From 37cfeaed684e080c80f6b8cba1935fef4e9abfba Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 20 Oct 2024 23:08:59 +1100 Subject: [PATCH 09/28] Refactor entry fetching and merging process - Introduce `MergingIdBasedFetcher` for batch operations - Create `MergeEntriesHelper` to encapsulate merge logic - Refactor `FetchAndMergeEntry` for improved separation of concerns - Implement `getFetcherForField` method in `MergingIdBasedFetcher` - Add background processing with progress updates - Enhance user notifications and error handling - Optimize logging --- .../java/org/jabref/gui/frame/MainMenu.java | 6 +- .../gui/mergeentries/FetchAndMergeEntry.java | 227 +++++------------- .../MultiEntryMergeWithFetchedDataAction.java | 48 ++-- .../logic/importer/MergeEntriesHelper.java | 87 +++++++ .../fetcher/MergingIdBasedFetcher.java | 132 ++++++++++ 5 files changed, 301 insertions(+), 199 deletions(-) create mode 100644 src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java create mode 100644 src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java index a02c6f91f3f..617e63b448b 100644 --- a/src/main/java/org/jabref/gui/frame/MainMenu.java +++ b/src/main/java/org/jabref/gui/frame/MainMenu.java @@ -278,16 +278,14 @@ private void createMenu() { new SeparatorMenuItem(), - // Adding new menu item for mass bibliographic data fetching factory.createMenuItem( StandardActions.MASS_GET_BIBLIOGRAPHIC_DATA, new MultiEntryMergeWithFetchedDataAction( frame::getCurrentLibraryTab, preferences, - taskExecutor, dialogService, - undoManager, - stateManager + stateManager, + taskExecutor ) ) ); diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index cba845442a8..f6119f81ed5 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -2,25 +2,18 @@ package org.jabref.gui.mergeentries; import java.util.Arrays; -import java.util.Comparator; -import java.util.LinkedHashSet; import java.util.List; import java.util.Optional; -import java.util.Set; -import java.util.stream.Collectors; import javax.swing.undo.UndoManager; import org.jabref.gui.DialogService; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.undo.NamedCompound; -import org.jabref.gui.undo.UndoableChangeType; -import org.jabref.gui.undo.UndoableFieldChange; import org.jabref.logic.importer.EntryBasedFetcher; -import org.jabref.logic.importer.FetcherClientException; -import org.jabref.logic.importer.FetcherServerException; import org.jabref.logic.importer.IdBasedFetcher; import org.jabref.logic.importer.ImportCleanup; +import org.jabref.logic.importer.MergeEntriesHelper; import org.jabref.logic.importer.WebFetcher; import org.jabref.logic.importer.WebFetchers; import org.jabref.logic.l10n.Localization; @@ -29,22 +22,22 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.Field; -import org.jabref.model.entry.field.FieldFactory; import org.jabref.model.entry.field.StandardField; -import org.jabref.model.entry.types.EntryType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** - * Class for fetching and merging bibliographic information - */ public class FetchAndMergeEntry { - // All identifiers listed here should also appear at {@link org.jabref.logic.importer.CompositeIdFetcher#performSearchById} + public static final List SUPPORTED_IDENTIFIER_FIELDS = Arrays.asList(StandardField.DOI, StandardField.EPRINT, StandardField.ISBN); private static final Logger LOGGER = LoggerFactory.getLogger(FetchAndMergeEntry.class); + private static final String ERROR_FETCHING = "Error while fetching from %0"; + private static final String MERGE_ENTRY_WITH = "Merge entry with %0 information"; + private static final String UPDATED_ENTRY = "Updated entry with info from %0"; + private static final String CANCELED_MERGING = "Canceled merging entries"; + private final DialogService dialogService; private final UndoManager undoManager; private final BibDatabaseContext bibDatabaseContext; @@ -72,38 +65,7 @@ public void fetchAndMerge(BibEntry entry, Field field) { } public void fetchAndMerge(BibEntry entry, List fields) { - fields.forEach(field -> fetchAndMergeEntryWithDialog(entry, field)); - } - - private void fetchAndMergeEntryWithDialog(BibEntry entry, Field field) { - entry.getField(field) - .flatMap(fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences())) - .ifPresent(fetcher -> executeFetchTaskWithDialog(fetcher, field, entry)); - } - - private void executeFetchTaskWithDialog(IdBasedFetcher fetcher, Field field, BibEntry entry) { - entry.getField(field) - .ifPresent(fieldContent -> - BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) - .onSuccess(fetchedEntry -> processFetchedEntryWithDialog(fetchedEntry, field, entry, fetcher)) - .onFailure(exception -> handleFetchException(exception, fetcher)) - .executeWith(taskExecutor) - ); - } - - private void processFetchedEntryWithDialog(Optional fetchedEntry, Field field, BibEntry originalEntry, IdBasedFetcher fetcher) { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - fetchedEntry.ifPresentOrElse( - entry -> { - cleanup.doPostCleanup(entry); - showMergeDialog(originalEntry, entry, fetcher); - }, - () -> dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), originalEntry.getField(field).orElse(""))) - ); - } - - public void fetchAndMergeBatch(List entries) { - entries.forEach(entry -> SUPPORTED_IDENTIFIER_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); + fields.forEach(field -> fetchAndMergeEntry(entry, field)); } private void fetchAndMergeEntry(BibEntry entry, Field field) { @@ -113,122 +75,43 @@ private void fetchAndMergeEntry(BibEntry entry, Field field) { } private void executeFetchTask(IdBasedFetcher fetcher, Field field, BibEntry entry) { - entry.getField(field) - .ifPresent(fieldContent -> BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) - .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, field, entry)) - .onFailure(exception -> handleFetchException(exception, fetcher)) - .executeWith(taskExecutor)); + entry.getField(field).ifPresent(fieldContent -> + BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) + .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, entry, fetcher)) + .onFailure(exception -> handleFetchException(exception, fetcher)) + .executeWith(taskExecutor) + ); } - private void processFetchedEntry(Optional fetchedEntry, Field field, BibEntry originalEntry) { + private void processFetchedEntry(Optional fetchedEntry, BibEntry originalEntry, IdBasedFetcher fetcher) { ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); fetchedEntry.ifPresentOrElse( entry -> { cleanup.doPostCleanup(entry); - mergeWithoutDialog(originalEntry, entry); + showMergeDialog(originalEntry, entry, fetcher); }, - // Notify if no entry was fetched - () -> dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", field.getDisplayName(), originalEntry.getField(field).orElse(""))) + () -> notifyNoInfo(originalEntry) ); } - private void handleFetchException(Exception exception, IdBasedFetcher fetcher) { - LOGGER.error("Error while fetching bibliographic information", exception); - String fetcherName = fetcher.getName(); - // Handle different types of exceptions with specific error messages - 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) { - NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); - - updateEntryTypeIfDifferent(originalEntry, fetchedEntry, ce); - updateFieldsWithNewInfo(originalEntry, fetchedEntry, ce); - removeObsoleteFields(originalEntry, fetchedEntry, ce); - - finalizeMerge(ce, originalEntry); - } - - private void updateFieldsWithNewInfo(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - Set jointFields = getJointFields(originalEntry, fetchedEntry); - for (Field field : jointFields) { - updateFieldIfNecessary(originalEntry, fetchedEntry, field, ce); - } - } - - private void updateFieldIfNecessary(BibEntry originalEntry, BibEntry fetchedEntry, Field field, NamedCompound ce) { - fetchedEntry.getField(field).ifPresent(fetchedValue -> { - Optional originalValue = originalEntry.getField(field); - if (originalValue.isEmpty() || fetchedValue.length() > originalValue.get().length()) { - originalEntry.setField(field, fetchedValue); - ce.addEdit(new UndoableFieldChange(originalEntry, field, originalValue.orElse(null), fetchedValue)); - } - }); + private void notifyNoInfo(BibEntry entry) { + dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", + entry.getType().getDisplayName(), + entry.getCitationKey().orElse(""))); } - private void removeObsoleteFields(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - Set jointFields = getJointFields(originalEntry, fetchedEntry); - Set originalFields = getFields(originalEntry); - - for (Field field : originalFields) { - if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { - removeField(originalEntry, field, ce); - } - } - } - - private void removeField(BibEntry entry, Field field, NamedCompound ce) { - Optional originalValue = entry.getField(field); - entry.clearField(field); - ce.addEdit(new UndoableFieldChange(entry, field, originalValue.orElse(null), null)); - } - - private void finalizeMerge(NamedCompound ce, BibEntry entry) { - String citationKey = entry.getCitationKey().orElse(entry.getAuthorTitleYear(40)); - String message = ce.hasEdits() - ? Localization.lang("Updated entry with fetched information [%0]", citationKey) - : Localization.lang("No new information was added [%0]", citationKey); - - if (ce.hasEdits()) { - ce.end(); - undoManager.addEdit(ce); - } - dialogService.notify(message); - } - - private Set getFields(BibEntry entry) { - // Get sorted set of fields for consistent ordering - return entry.getFields().stream() - .sorted(Comparator.comparing(Field::getName)) - .collect(Collectors.toCollection(LinkedHashSet::new)); - } - - private Set getJointFields(BibEntry entry1, BibEntry entry2) { - Set fields = new LinkedHashSet<>(); - fields.addAll(getFields(entry1)); - fields.addAll(getFields(entry2)); - return fields; - } - - private void updateEntryTypeIfDifferent(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - EntryType oldType = originalEntry.getType(); - EntryType newType = fetchedEntry.getType(); - - if (!oldType.equals(newType)) { - originalEntry.setType(newType); - ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); - } + private void handleFetchException(Exception exception, WebFetcher fetcher) { + LOGGER.error("Error while fetching bibliographic information", exception); + dialogService.showErrorDialogAndWait( + Localization.lang(ERROR_FETCHING, fetcher.getName()), + exception + ); } private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { MergeEntriesDialog dialog = createMergeDialog(originalEntry, fetchedEntry, fetcher); - Optional mergedEntry = showDialogAndGetResult(dialog); + Optional mergedEntry = dialogService.showCustomDialogAndWait(dialog) + .map(EntriesMergeResult::mergedEntry); mergedEntry.ifPresentOrElse( entry -> processMergedEntry(originalEntry, entry, fetcher), @@ -238,48 +121,48 @@ private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebF private MergeEntriesDialog createMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { MergeEntriesDialog dialog = new MergeEntriesDialog(originalEntry, fetchedEntry, preferences); - dialog.setTitle(Localization.lang("Merge entry with %0 information", fetcher.getName())); + dialog.setTitle(Localization.lang(MERGE_ENTRY_WITH, fetcher.getName())); dialog.setLeftHeaderText(Localization.lang("Original entry")); dialog.setRightHeaderText(Localization.lang("Entry from %0", fetcher.getName())); return dialog; } - private Optional showDialogAndGetResult(MergeEntriesDialog dialog) { - return dialogService.showCustomDialogAndWait(dialog) - .map(EntriesMergeResult::mergedEntry); - } - private void processMergedEntry(BibEntry originalEntry, BibEntry mergedEntry, WebFetcher fetcher) { - NamedCompound ce = new NamedCompound(Localization.lang("Merge entry with %0 information", fetcher.getName())); + NamedCompound ce = new NamedCompound(Localization.lang(MERGE_ENTRY_WITH, fetcher.getName())); + MergeEntriesHelper.mergeEntries(originalEntry, mergedEntry, ce); - updateEntryTypeIfDifferent(originalEntry, mergedEntry, ce); - updateFieldsWithNewInfo(originalEntry, mergedEntry, ce); - removeObsoleteFields(originalEntry, mergedEntry, ce); + if (ce.hasEdits()) { + ce.end(); + undoManager.addEdit(ce); + } - finalizeMerge(ce, originalEntry); + dialogService.notify(Localization.lang(UPDATED_ENTRY, fetcher.getName())); } private void notifyCanceledMerge(BibEntry entry) { String citationKey = entry.getCitationKey().orElse(entry.getAuthorTitleYear(40)); - dialogService.notify(Localization.lang("Canceled merging entries") + " [" + citationKey + "]"); + dialogService.notify(Localization.lang(CANCELED_MERGING) + " [" + citationKey + "]"); } public void fetchAndMerge(BibEntry entry, EntryBasedFetcher fetcher) { BackgroundTask.wrap(() -> fetcher.performSearch(entry).stream().findFirst()) - .onSuccess(fetchedEntry -> fetchedEntry - .map(fe -> { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - cleanup.doPostCleanup(fe); - return fe; - }) - .ifPresentOrElse( - fe -> showMergeDialog(entry, fe, fetcher), - () -> dialogService.notify(Localization.lang("Could not find any bibliographic information.")) - )) - .onFailure(exception -> { - LOGGER.error("Error while fetching entry with {} ", fetcher.getName(), exception); - dialogService.showErrorDialogAndWait(Localization.lang("Error while fetching from %0", fetcher.getName()), exception); - }) + .onSuccess(fetchedEntry -> processFetchedEntryForEntryBasedFetcher(fetchedEntry, entry, fetcher)) + .onFailure(exception -> handleFetchException(exception, fetcher)) .executeWith(taskExecutor); } + + private void processFetchedEntryForEntryBasedFetcher(Optional fetchedEntry, BibEntry originalEntry, EntryBasedFetcher fetcher) { + fetchedEntry + .map(this::cleanupFetchedEntry) + .ifPresentOrElse( + fe -> showMergeDialog(originalEntry, fe, fetcher), + () -> dialogService.notify(Localization.lang("Could not find any bibliographic information.")) + ); + } + + private BibEntry cleanupFetchedEntry(BibEntry fetchedEntry) { + ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); + cleanup.doPostCleanup(fetchedEntry); + return fetchedEntry; + } } diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java index 1d842ba7763..cc7b6eef886 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java @@ -1,49 +1,45 @@ + package org.jabref.gui.mergeentries; import java.util.List; import java.util.function.Supplier; -import javax.swing.undo.UndoManager; - import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; import org.jabref.gui.StateManager; import org.jabref.gui.actions.ActionHelper; import org.jabref.gui.actions.SimpleCommand; import org.jabref.gui.preferences.GuiPreferences; +import org.jabref.logic.importer.fetcher.MergingIdBasedFetcher; +import org.jabref.logic.importer.fetcher.isbntobibtex.IsbnFetcher; +import org.jabref.logic.l10n.Localization; +import org.jabref.logic.util.BackgroundTask; import org.jabref.logic.util.TaskExecutor; -import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** - * Action for merging multiple entries with fetched data from identifiers like DOI or arXiv. - */ public class MultiEntryMergeWithFetchedDataAction extends SimpleCommand { private static final Logger LOGGER = LoggerFactory.getLogger(MultiEntryMergeWithFetchedDataAction.class); private final Supplier tabSupplier; private final GuiPreferences preferences; - private final TaskExecutor taskExecutor; private final DialogService dialogService; - private final UndoManager undoManager; + private final TaskExecutor taskExecutor; public MultiEntryMergeWithFetchedDataAction(Supplier tabSupplier, GuiPreferences preferences, - TaskExecutor taskExecutor, DialogService dialogService, - UndoManager undoManager, - StateManager stateManager) { + StateManager stateManager, + TaskExecutor taskExecutor + ) { this.tabSupplier = tabSupplier; this.preferences = preferences; - this.taskExecutor = taskExecutor; this.dialogService = dialogService; - this.undoManager = undoManager; + this.taskExecutor = taskExecutor; - // Binding to only execute if a database is open this.executable.bind(ActionHelper.needsDatabase(stateManager)); } @@ -51,24 +47,30 @@ public MultiEntryMergeWithFetchedDataAction(Supplier tabSupplier, public void execute() { LibraryTab libraryTab = tabSupplier.get(); - // Ensure that libraryTab is present if (libraryTab == null) { LOGGER.error("Action 'Multi Entry Merge' must be disabled when no database is open."); return; } - List selectedEntries = libraryTab.getDatabase().getEntries(); + List allEntries = libraryTab.getDatabase().getEntries(); - // If no entries are selected, log and do not execute - if (selectedEntries.isEmpty()) { - dialogService.notify("No entries selected for merging."); + if (allEntries.isEmpty()) { + dialogService.notify(Localization.lang("No entries exist.")); return; } - // Create an instance of FetchAndMergeEntry to perform the batch fetch and merge operation - BibDatabaseContext bibDatabaseContext = libraryTab.getBibDatabaseContext(); - FetchAndMergeEntry fetchAndMergeEntry = new FetchAndMergeEntry(bibDatabaseContext, taskExecutor, preferences, dialogService, undoManager); + preferences.getImportFormatPreferences(); + new IsbnFetcher(preferences.getImportFormatPreferences()); + preferences.getImportFormatPreferences(); + + MergingIdBasedFetcher mergingIdBasedFetcher = new MergingIdBasedFetcher( + preferences, + libraryTab.getUndoManager(), + libraryTab.getBibDatabaseContext(), + dialogService + ); - fetchAndMergeEntry.fetchAndMergeBatch(selectedEntries); + BackgroundTask> task = mergingIdBasedFetcher.fetchAndMergeBatch(allEntries); + taskExecutor.execute(task); } } diff --git a/src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java b/src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java new file mode 100644 index 00000000000..8f0a21d6524 --- /dev/null +++ b/src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java @@ -0,0 +1,87 @@ + +package org.jabref.logic.importer; + +import java.util.Comparator; +import java.util.LinkedHashSet; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +import org.jabref.gui.undo.NamedCompound; +import org.jabref.gui.undo.UndoableChangeType; +import org.jabref.gui.undo.UndoableFieldChange; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.FieldFactory; +import org.jabref.model.entry.types.EntryType; + +public class MergeEntriesHelper { + + private MergeEntriesHelper() { + // Private constructor to prevent instantiation + } + + public static void mergeEntries(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + updateEntryTypeIfDifferent(originalEntry, fetchedEntry, ce); + updateFieldsWithNewInfo(originalEntry, fetchedEntry, ce); + removeObsoleteFields(originalEntry, fetchedEntry, ce); + } + + private static void updateEntryTypeIfDifferent(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + EntryType oldType = originalEntry.getType(); + EntryType newType = fetchedEntry.getType(); + + if (!oldType.equals(newType)) { + originalEntry.setType(newType); + ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); + } + } + + private static void updateFieldsWithNewInfo(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + Set jointFields = getJointFields(originalEntry, fetchedEntry); + for (Field field : jointFields) { + updateFieldIfNecessary(originalEntry, fetchedEntry, field, ce); + } + } + + private static void updateFieldIfNecessary(BibEntry originalEntry, BibEntry fetchedEntry, Field field, NamedCompound ce) { + fetchedEntry.getField(field).ifPresent(fetchedValue -> { + Optional originalValue = originalEntry.getField(field); + if (originalValue.isEmpty() || fetchedValue.length() > originalValue.get().length()) { + originalEntry.setField(field, fetchedValue); + ce.addEdit(new UndoableFieldChange(originalEntry, field, originalValue.orElse(null), fetchedValue)); + } + }); + } + + private static void removeObsoleteFields(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { + Set jointFields = getJointFields(originalEntry, fetchedEntry); + Set originalFields = getFields(originalEntry); + + for (Field field : originalFields) { + if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { + removeField(originalEntry, field, ce); + } + } + } + + private static void removeField(BibEntry entry, Field field, NamedCompound ce) { + Optional originalValue = entry.getField(field); + entry.clearField(field); + ce.addEdit(new UndoableFieldChange(entry, field, originalValue.orElse(null), null)); + } + + private static Set getFields(BibEntry entry) { + // Get sorted set of fields for consistent ordering + return entry.getFields().stream() + .sorted(Comparator.comparing(Field::getName)) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + + private static Set getJointFields(BibEntry entry1, BibEntry entry2) { + Set fields = new LinkedHashSet<>(); + fields.addAll(getFields(entry1)); + fields.addAll(getFields(entry2)); + return fields; + } +} diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java new file mode 100644 index 00000000000..45186b0addb --- /dev/null +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -0,0 +1,132 @@ + +package org.jabref.logic.importer.fetcher; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; + +import javax.swing.undo.UndoManager; + +import org.jabref.gui.DialogService; +import org.jabref.gui.preferences.GuiPreferences; +import org.jabref.gui.undo.NamedCompound; +import org.jabref.logic.importer.IdBasedFetcher; +import org.jabref.logic.importer.ImportCleanup; +import org.jabref.logic.importer.MergeEntriesHelper; +import org.jabref.logic.importer.fetcher.isbntobibtex.IsbnFetcher; +import org.jabref.logic.l10n.Localization; +import org.jabref.logic.util.BackgroundTask; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.StandardField; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MergingIdBasedFetcher { + + private static final Logger LOGGER = LoggerFactory.getLogger(MergingIdBasedFetcher.class); + private final GuiPreferences preferences; + private final UndoManager undoManager; + private final BibDatabaseContext bibDatabaseContext; + private final DialogService dialogService; + + public MergingIdBasedFetcher(GuiPreferences preferences, UndoManager undoManager, BibDatabaseContext bibDatabaseContext, DialogService dialogService) { + this.preferences = preferences; + this.undoManager = undoManager; + this.bibDatabaseContext = bibDatabaseContext; + this.dialogService = dialogService; + } + + public BackgroundTask> fetchAndMergeBatch(List entries) { + if (entries.isEmpty()) { + return BackgroundTask.wrap(() -> List.of()); + } + + BackgroundTask> backgroundTask = new BackgroundTask<>() { + @Override + public List call() { + List updatedEntries = new ArrayList<>(); + int totalEntries = entries.size(); + for (int i = 0; i < totalEntries; i++) { + if (isCancelled()) { + break; + } + BibEntry entry = entries.get(i); + updateMessage(Localization.lang("Fetching entry %d of %d", i + 1, totalEntries)); + updateProgress(i, totalEntries); + if (fetchAndMergeEntry(entry)) { + entry.getCitationKey().ifPresent(citationKey -> { + updatedEntries.add(citationKey); + LOGGER.info("Updated entry: {}", citationKey); + }); + } else { + LOGGER.info("Entry not updated: {}", entry.getCitationKey().orElse("No citation key")); + } + } + return updatedEntries; + } + }; + + backgroundTask.setTitle(Localization.lang("Fetching and merging entries")); + backgroundTask.showToUser(true); + + backgroundTask.onSuccess(updatedEntries -> { + LOGGER.info("Batch update completed. {} entries updated: {}", updatedEntries.size(), String.join(", ", updatedEntries)); + // Show notification without the citation keys + String message = Localization.lang("%0 entries updated", updatedEntries.size()); + dialogService.notify(message); + }); + + backgroundTask.onFailure(exception -> { + LOGGER.error("Error fetching and merging entries", exception); + dialogService.notify(Localization.lang("Error fetching and merging entries")); + }); + + return backgroundTask; + } + + private boolean fetchAndMergeEntry(BibEntry entry) { + for (Field field : List.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT)) { + Optional identifier = entry.getField(field); + if (identifier.isPresent()) { + Optional fetcherOpt = getFetcherForField(field); + if (fetcherOpt.isPresent()) { + try { + Optional fetchedEntry = fetcherOpt.get().performSearchById(identifier.get()); + if (fetchedEntry.isPresent()) { + return processFetchedEntry(fetchedEntry.get(), entry); + } + } catch (Exception exception) { + LOGGER.error("Error fetching entry with {} {}", field, identifier.get(), exception); + } + } + } + } + return false; + } + + private Optional getFetcherForField(Field field) { + return switch (field) { + case StandardField.DOI -> Optional.of(new DoiFetcher(preferences.getImportFormatPreferences())); + case StandardField.ISBN -> Optional.of(new IsbnFetcher(preferences.getImportFormatPreferences())); + case StandardField.EPRINT -> Optional.of(new IacrEprintFetcher(preferences.getImportFormatPreferences())); + default -> Optional.empty(); + }; + } + + private boolean processFetchedEntry(BibEntry fetchedEntry, BibEntry originalEntry) { + ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()).doPostCleanup(fetchedEntry); + + NamedCompound ce = new NamedCompound(Localization.lang("Merge entry")); + MergeEntriesHelper.mergeEntries(originalEntry, fetchedEntry, ce); + + if (ce.hasEdits()) { + ce.end(); + undoManager.addEdit(ce); + return true; + } + return false; + } +} From 77e0d5163e9e35bdffb3e99b3400d0c501bbc1b0 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 20 Oct 2024 23:11:08 +1100 Subject: [PATCH 10/28] Updated CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63ababbab73..a9147c8d4ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) -- Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) +- Added functionality for mass addition of bibliographic information for multiple entries to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) ### Changed From c5b7b4ccc12c996a8acdbd14da2cfe2de3b32c12 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Fri, 25 Oct 2024 18:17:37 +1100 Subject: [PATCH 11/28] refactor: Separate UI and core logic in entry merging system - Move UI-related merge handling to MultiEntryMergeWithFetchedDataAction - Relocate MergeEntriesHelper to GUI layer - Make MergingIdBasedFetcher pure business logic without UI dependencies - Add FetcherResult record to track merge changes - Enhance error handling and logging Updated JabRef_en.properties. Already Checked LocalizationConsistencyTest and MainArchitectureTest. --- .../gui/mergeentries/FetchAndMergeEntry.java | 15 +- .../gui/mergeentries/MergeEntriesHelper.java | 128 +++++++++++++ .../MultiEntryMergeWithFetchedDataAction.java | 165 ++++++++++++++--- .../logic/importer/MergeEntriesHelper.java | 87 --------- .../fetcher/MergingIdBasedFetcher.java | 171 ++++++++---------- src/main/resources/l10n/JabRef_en.properties | 14 +- 6 files changed, 360 insertions(+), 220 deletions(-) create mode 100644 src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java delete mode 100644 src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index f6119f81ed5..c18177604fa 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -13,7 +13,6 @@ import org.jabref.logic.importer.EntryBasedFetcher; import org.jabref.logic.importer.IdBasedFetcher; import org.jabref.logic.importer.ImportCleanup; -import org.jabref.logic.importer.MergeEntriesHelper; import org.jabref.logic.importer.WebFetcher; import org.jabref.logic.importer.WebFetchers; import org.jabref.logic.l10n.Localization; @@ -33,10 +32,6 @@ public class FetchAndMergeEntry { public static final List SUPPORTED_IDENTIFIER_FIELDS = Arrays.asList(StandardField.DOI, StandardField.EPRINT, StandardField.ISBN); private static final Logger LOGGER = LoggerFactory.getLogger(FetchAndMergeEntry.class); - private static final String ERROR_FETCHING = "Error while fetching from %0"; - private static final String MERGE_ENTRY_WITH = "Merge entry with %0 information"; - private static final String UPDATED_ENTRY = "Updated entry with info from %0"; - private static final String CANCELED_MERGING = "Canceled merging entries"; private final DialogService dialogService; private final UndoManager undoManager; @@ -103,7 +98,7 @@ private void notifyNoInfo(BibEntry entry) { private void handleFetchException(Exception exception, WebFetcher fetcher) { LOGGER.error("Error while fetching bibliographic information", exception); dialogService.showErrorDialogAndWait( - Localization.lang(ERROR_FETCHING, fetcher.getName()), + Localization.lang("Error while fetching from %0", fetcher.getName()), exception ); } @@ -121,14 +116,14 @@ private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebF private MergeEntriesDialog createMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { MergeEntriesDialog dialog = new MergeEntriesDialog(originalEntry, fetchedEntry, preferences); - dialog.setTitle(Localization.lang(MERGE_ENTRY_WITH, fetcher.getName())); + dialog.setTitle(Localization.lang("Merge entry with %0 information", fetcher.getName())); dialog.setLeftHeaderText(Localization.lang("Original entry")); dialog.setRightHeaderText(Localization.lang("Entry from %0", fetcher.getName())); return dialog; } private void processMergedEntry(BibEntry originalEntry, BibEntry mergedEntry, WebFetcher fetcher) { - NamedCompound ce = new NamedCompound(Localization.lang(MERGE_ENTRY_WITH, fetcher.getName())); + NamedCompound ce = new NamedCompound(Localization.lang("Merge entry with %0 information", fetcher.getName())); MergeEntriesHelper.mergeEntries(originalEntry, mergedEntry, ce); if (ce.hasEdits()) { @@ -136,12 +131,12 @@ private void processMergedEntry(BibEntry originalEntry, BibEntry mergedEntry, We undoManager.addEdit(ce); } - dialogService.notify(Localization.lang(UPDATED_ENTRY, fetcher.getName())); + dialogService.notify(Localization.lang("Updated entry with info from %0", fetcher.getName())); } private void notifyCanceledMerge(BibEntry entry) { String citationKey = entry.getCitationKey().orElse(entry.getAuthorTitleYear(40)); - dialogService.notify(Localization.lang(CANCELED_MERGING) + " [" + citationKey + "]"); + dialogService.notify(Localization.lang("Canceled merging entries") + " [" + citationKey + "]"); } public void fetchAndMerge(BibEntry entry, EntryBasedFetcher fetcher) { diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java new file mode 100644 index 00000000000..471e4acee51 --- /dev/null +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -0,0 +1,128 @@ + +package org.jabref.gui.mergeentries; + +import java.util.LinkedHashSet; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +import org.jabref.gui.undo.NamedCompound; +import org.jabref.gui.undo.UndoableChangeType; +import org.jabref.gui.undo.UndoableFieldChange; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.FieldFactory; +import org.jabref.model.entry.types.EntryType; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper class for merging Entries. + */ +public final class MergeEntriesHelper { + + private static final Logger LOGGER = LoggerFactory.getLogger(MergeEntriesHelper.class); + + private MergeEntriesHelper() { + // Private constructor to prevent instantiation of utility class + } + + /** + * Merges two BibEntry objects with undo support. + * Use this method when modifying entries directly in the UI. + * The original entry will be updated with information from the fetched entry. + */ + public static void mergeEntries(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound undoManager) { + Objects.requireNonNull(originalEntry, "Original entry cannot be null"); + Objects.requireNonNull(fetchedEntry, "Fetched entry cannot be null"); + Objects.requireNonNull(undoManager, "Undo manager cannot be null"); + + LOGGER.debug("Starting merge of entries. Original type: {}, Fetched type: {}", + originalEntry.getType(), fetchedEntry.getType()); + + updateEntryTypeIfDifferent(originalEntry, fetchedEntry, undoManager); + updateFieldsWithNewInfo(originalEntry, fetchedEntry, undoManager); + removeObsoleteFields(originalEntry, fetchedEntry, undoManager); + + LOGGER.debug("Finished merging entries"); + } + + private static void updateEntryTypeIfDifferent(final BibEntry originalEntry, + final BibEntry fetchedEntry, final NamedCompound undoManager) { + EntryType oldType = originalEntry.getType(); + EntryType newType = fetchedEntry.getType(); + + if (!oldType.equals(newType)) { + LOGGER.debug("Updating entry type from {} to {}", oldType, newType); + originalEntry.setType(newType); + undoManager.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); + } + } + + private static void updateFieldsWithNewInfo(final BibEntry originalEntry, + final BibEntry fetchedEntry, final NamedCompound undoManager) { + Set jointFields = getJointFields(originalEntry, fetchedEntry); + LOGGER.debug("Processing {} joint fields for updates", jointFields.size()); + + for (Field field : jointFields) { + updateFieldIfNecessaryWithUndo(originalEntry, fetchedEntry, field, undoManager); + } + } + + private static void updateFieldIfNecessaryWithUndo(final BibEntry originalEntry, + final BibEntry fetchedEntry, + final Field field, final NamedCompound undoManager) { + fetchedEntry.getField(field).ifPresent(fetchedValue -> originalEntry.getField(field).ifPresentOrElse( + originalValue -> { + if (fetchedValue.length() > originalValue.length()) { + updateField(originalEntry, field, originalValue, fetchedValue, undoManager); + } + }, + () -> updateField(originalEntry, field, null, fetchedValue, undoManager) + )); + } + + private static void updateField(final BibEntry entry, final Field field, + final String originalValue, final String newValue, final NamedCompound undoManager) { + LOGGER.debug("Updating field '{}' from '{}' to '{}'", field, originalValue, newValue); + entry.setField(field, newValue); + undoManager.addEdit(new UndoableFieldChange(entry, field, originalValue, newValue)); + } + + private static void removeObsoleteFields(final BibEntry originalEntry, + final BibEntry fetchedEntry, + final NamedCompound undoManager) { + + Set jointFields = getJointFields(originalEntry, fetchedEntry); + Set originalFields = getFields(originalEntry); + + LOGGER.debug("Checking {} original fields for obsolete entries", originalFields.size()); + + for (Field field : originalFields) { + if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { + removeFieldWithUndo(originalEntry, field, undoManager); + } + } + } + + private static void removeFieldWithUndo(final BibEntry entry, final Field field, + final NamedCompound undoManager) { + Optional originalValue = entry.getField(field); + LOGGER.debug("Removing obsolete field '{}' with value '{}'", field, originalValue.orElse(null)); + + entry.clearField(field); + undoManager.addEdit(new UndoableFieldChange(entry, field, originalValue.orElse(null), null)); + } + + private static Set getFields(final BibEntry entry) { + return new LinkedHashSet<>(entry.getFields()); + } + + private static Set getJointFields(final BibEntry entry1, final BibEntry entry2) { + Set fields = new LinkedHashSet<>(); + fields.addAll(getFields(entry1)); + fields.addAll(getFields(entry2)); + return fields; + } +} diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java index cc7b6eef886..0a8927b8488 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java @@ -1,76 +1,193 @@ package org.jabref.gui.mergeentries; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.function.Supplier; -import org.jabref.gui.DialogService; +import javafx.application.Platform; + import org.jabref.gui.LibraryTab; import org.jabref.gui.StateManager; import org.jabref.gui.actions.ActionHelper; import org.jabref.gui.actions.SimpleCommand; import org.jabref.gui.preferences.GuiPreferences; +import org.jabref.gui.undo.NamedCompound; import org.jabref.logic.importer.fetcher.MergingIdBasedFetcher; -import org.jabref.logic.importer.fetcher.isbntobibtex.IsbnFetcher; import org.jabref.logic.l10n.Localization; import org.jabref.logic.util.BackgroundTask; +import org.jabref.logic.util.NotificationService; import org.jabref.logic.util.TaskExecutor; import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.StandardField; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * Handles the merging of multiple bibliography entries with fetched data. + * This action performs background fetching and merging of entries while + * providing progress updates to the user. + */ public class MultiEntryMergeWithFetchedDataAction extends SimpleCommand { private static final Logger LOGGER = LoggerFactory.getLogger(MultiEntryMergeWithFetchedDataAction.class); private final Supplier tabSupplier; private final GuiPreferences preferences; - private final DialogService dialogService; + private final NotificationService notificationService; private final TaskExecutor taskExecutor; public MultiEntryMergeWithFetchedDataAction(Supplier tabSupplier, GuiPreferences preferences, - DialogService dialogService, + NotificationService notificationService, StateManager stateManager, - TaskExecutor taskExecutor - ) { + TaskExecutor taskExecutor) { this.tabSupplier = tabSupplier; this.preferences = preferences; - this.dialogService = dialogService; + this.notificationService = notificationService; this.taskExecutor = taskExecutor; - this.executable.bind(ActionHelper.needsDatabase(stateManager)); } @Override public void execute() { LibraryTab libraryTab = tabSupplier.get(); - if (libraryTab == null) { LOGGER.error("Action 'Multi Entry Merge' must be disabled when no database is open."); return; } - List allEntries = libraryTab.getDatabase().getEntries(); - - if (allEntries.isEmpty()) { - dialogService.notify(Localization.lang("No entries exist.")); + List entries = libraryTab.getDatabase().getEntries(); + if (entries.isEmpty()) { + notificationService.notify(Localization.lang("No entries exist.")); return; } - preferences.getImportFormatPreferences(); - new IsbnFetcher(preferences.getImportFormatPreferences()); - preferences.getImportFormatPreferences(); + MergeContext context = new MergeContext(libraryTab, entries, createMergingFetcher()); + BackgroundTask> backgroundTask = createBackgroundTask(context); + taskExecutor.execute(backgroundTask); + } + + private MergingIdBasedFetcher createMergingFetcher() { + return new MergingIdBasedFetcher(preferences.getImportFormatPreferences()); + } + + private BackgroundTask> createBackgroundTask(MergeContext context) { + BackgroundTask> task = new BackgroundTask<>() { + @Override + public List call() { + return processMergeEntries(context, this); + } + }; + configureBackgroundTask(task); + return task; + } + + private List processMergeEntries(MergeContext context, BackgroundTask task) { + if (task.isCancelled()) { + return List.of(); + } + + int totalEntries = context.entries.size(); + for (int i = 0; i < totalEntries && !task.isCancelled(); i++) { + processEntry(context, context.entries.get(i), i, totalEntries, task); + } + + finalizeCompoundEdit(context); + return context.updatedEntries; + } - MergingIdBasedFetcher mergingIdBasedFetcher = new MergingIdBasedFetcher( - preferences, - libraryTab.getUndoManager(), - libraryTab.getBibDatabaseContext(), - dialogService - ); + private void processEntry(MergeContext context, BibEntry entry, int currentIndex, + int totalEntries, BackgroundTask task) { + updateProgress(currentIndex, totalEntries, task); + logEntryDetails(entry); + + Optional fetchResult = context.fetcher.fetchEntry(entry); + fetchResult.ifPresent(result -> { + if (result.hasChanges()) { + Platform.runLater(() -> { + // UI operations must be done on the FX thread + MergeEntriesHelper.mergeEntries(entry, result.mergedEntry(), context.compoundEdit); + entry.getCitationKey().ifPresent(context.updatedEntries::add); + }); + } + }); + } - BackgroundTask> task = mergingIdBasedFetcher.fetchAndMergeBatch(allEntries); - taskExecutor.execute(task); + private void finalizeCompoundEdit(MergeContext context) { + if (!context.updatedEntries.isEmpty()) { + Platform.runLater(() -> { + context.compoundEdit.end(); + context.libraryTab.getUndoManager().addEdit(context.compoundEdit); + }); + } + } + + private void logEntryDetails(BibEntry entry) { + Map details = new HashMap<>(); + entry.getCitationKey().ifPresent(key -> details.put("key", key)); + entry.getField(StandardField.ISBN).ifPresent(isbn -> details.put("isbn", isbn)); + entry.getField(StandardField.DOI).ifPresent(doi -> details.put("doi", doi)); + details.put("type", entry.getType().getName()); + + LOGGER.info("Processing BibEntry: {}", details); + } + + private void updateProgress(int currentIndex, int totalEntries, BackgroundTask task) { + Platform.runLater(() -> { + task.updateMessage(Localization.lang("Fetching entry %d of %d", currentIndex + 1, totalEntries)); + task.updateProgress(currentIndex, totalEntries); + }); + } + + private void configureBackgroundTask(BackgroundTask> task) { + task.setTitle(Localization.lang("Fetching and merging entries")); + task.showToUser(true); + configureSuccessHandler(task); + configureFailureHandler(task); + } + + private void configureSuccessHandler(BackgroundTask> task) { + task.onSuccess(updatedEntries -> Platform.runLater(() -> { + if (updatedEntries.isEmpty()) { + LOGGER.info("Batch update completed. No entries were updated."); + notificationService.notify(Localization.lang("No updates found.")); + } else { + String message = Localization.lang("Batch update successful. %0 entries updated: %1.", + updatedEntries.size(), String.join(", ", updatedEntries)); + notificationService.notify(message); + } + })); + } + + private void configureFailureHandler(BackgroundTask> task) { + task.onFailure(ex -> { + String errorType = ex.getClass().getSimpleName(); + LOGGER.error("{}: {}", errorType, ex.getMessage(), ex); + Platform.runLater(() -> + notificationService.notify(Localization.lang("Error while fetching and merging entries: %0", ex.getMessage())) + ); + }); + } + + private static class MergeContext { + final LibraryTab libraryTab; + final List entries; + final MergingIdBasedFetcher fetcher; + final NamedCompound compoundEdit; + final List updatedEntries; + + MergeContext(LibraryTab libraryTab, List entries, MergingIdBasedFetcher fetcher) { + this.libraryTab = libraryTab; + this.entries = entries; + this.fetcher = fetcher; + this.compoundEdit = new NamedCompound(Localization.lang("Merge entries")); + this.updatedEntries = Collections.synchronizedList(new ArrayList<>()); + } } } diff --git a/src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java b/src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java deleted file mode 100644 index 8f0a21d6524..00000000000 --- a/src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java +++ /dev/null @@ -1,87 +0,0 @@ - -package org.jabref.logic.importer; - -import java.util.Comparator; -import java.util.LinkedHashSet; -import java.util.Optional; -import java.util.Set; -import java.util.stream.Collectors; - -import org.jabref.gui.undo.NamedCompound; -import org.jabref.gui.undo.UndoableChangeType; -import org.jabref.gui.undo.UndoableFieldChange; -import org.jabref.model.entry.BibEntry; -import org.jabref.model.entry.field.Field; -import org.jabref.model.entry.field.FieldFactory; -import org.jabref.model.entry.types.EntryType; - -public class MergeEntriesHelper { - - private MergeEntriesHelper() { - // Private constructor to prevent instantiation - } - - public static void mergeEntries(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - updateEntryTypeIfDifferent(originalEntry, fetchedEntry, ce); - updateFieldsWithNewInfo(originalEntry, fetchedEntry, ce); - removeObsoleteFields(originalEntry, fetchedEntry, ce); - } - - private static void updateEntryTypeIfDifferent(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - EntryType oldType = originalEntry.getType(); - EntryType newType = fetchedEntry.getType(); - - if (!oldType.equals(newType)) { - originalEntry.setType(newType); - ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); - } - } - - private static void updateFieldsWithNewInfo(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - Set jointFields = getJointFields(originalEntry, fetchedEntry); - for (Field field : jointFields) { - updateFieldIfNecessary(originalEntry, fetchedEntry, field, ce); - } - } - - private static void updateFieldIfNecessary(BibEntry originalEntry, BibEntry fetchedEntry, Field field, NamedCompound ce) { - fetchedEntry.getField(field).ifPresent(fetchedValue -> { - Optional originalValue = originalEntry.getField(field); - if (originalValue.isEmpty() || fetchedValue.length() > originalValue.get().length()) { - originalEntry.setField(field, fetchedValue); - ce.addEdit(new UndoableFieldChange(originalEntry, field, originalValue.orElse(null), fetchedValue)); - } - }); - } - - private static void removeObsoleteFields(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound ce) { - Set jointFields = getJointFields(originalEntry, fetchedEntry); - Set originalFields = getFields(originalEntry); - - for (Field field : originalFields) { - if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { - removeField(originalEntry, field, ce); - } - } - } - - private static void removeField(BibEntry entry, Field field, NamedCompound ce) { - Optional originalValue = entry.getField(field); - entry.clearField(field); - ce.addEdit(new UndoableFieldChange(entry, field, originalValue.orElse(null), null)); - } - - private static Set getFields(BibEntry entry) { - // Get sorted set of fields for consistent ordering - return entry.getFields().stream() - .sorted(Comparator.comparing(Field::getName)) - .collect(Collectors.toCollection(LinkedHashSet::new)); - } - - private static Set getJointFields(BibEntry entry1, BibEntry entry2) { - Set fields = new LinkedHashSet<>(); - fields.addAll(getFields(entry1)); - fields.addAll(getFields(entry2)); - return fields; - } -} diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index 45186b0addb..94cda2378a9 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -1,22 +1,14 @@ package org.jabref.logic.importer.fetcher; -import java.util.ArrayList; import java.util.List; import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; -import javax.swing.undo.UndoManager; - -import org.jabref.gui.DialogService; -import org.jabref.gui.preferences.GuiPreferences; -import org.jabref.gui.undo.NamedCompound; import org.jabref.logic.importer.IdBasedFetcher; -import org.jabref.logic.importer.ImportCleanup; -import org.jabref.logic.importer.MergeEntriesHelper; +import org.jabref.logic.importer.ImportFormatPreferences; import org.jabref.logic.importer.fetcher.isbntobibtex.IsbnFetcher; -import org.jabref.logic.l10n.Localization; -import org.jabref.logic.util.BackgroundTask; -import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.Field; import org.jabref.model.entry.field.StandardField; @@ -25,108 +17,103 @@ import org.slf4j.LoggerFactory; public class MergingIdBasedFetcher { + private static final List SUPPORTED_FIELDS = + List.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT); private static final Logger LOGGER = LoggerFactory.getLogger(MergingIdBasedFetcher.class); - private final GuiPreferences preferences; - private final UndoManager undoManager; - private final BibDatabaseContext bibDatabaseContext; - private final DialogService dialogService; - - public MergingIdBasedFetcher(GuiPreferences preferences, UndoManager undoManager, BibDatabaseContext bibDatabaseContext, DialogService dialogService) { - this.preferences = preferences; - this.undoManager = undoManager; - this.bibDatabaseContext = bibDatabaseContext; - this.dialogService = dialogService; - } - - public BackgroundTask> fetchAndMergeBatch(List entries) { - if (entries.isEmpty()) { - return BackgroundTask.wrap(() -> List.of()); - } + private final ImportFormatPreferences importFormatPreferences; - BackgroundTask> backgroundTask = new BackgroundTask<>() { - @Override - public List call() { - List updatedEntries = new ArrayList<>(); - int totalEntries = entries.size(); - for (int i = 0; i < totalEntries; i++) { - if (isCancelled()) { - break; - } - BibEntry entry = entries.get(i); - updateMessage(Localization.lang("Fetching entry %d of %d", i + 1, totalEntries)); - updateProgress(i, totalEntries); - if (fetchAndMergeEntry(entry)) { - entry.getCitationKey().ifPresent(citationKey -> { - updatedEntries.add(citationKey); - LOGGER.info("Updated entry: {}", citationKey); - }); - } else { - LOGGER.info("Entry not updated: {}", entry.getCitationKey().orElse("No citation key")); - } - } - return updatedEntries; - } - }; + public MergingIdBasedFetcher(ImportFormatPreferences importFormatPreferences) { + this.importFormatPreferences = importFormatPreferences; + } - backgroundTask.setTitle(Localization.lang("Fetching and merging entries")); - backgroundTask.showToUser(true); + public Optional fetchEntry(BibEntry entry) { + logEntryIdentifiers(entry); - backgroundTask.onSuccess(updatedEntries -> { - LOGGER.info("Batch update completed. {} entries updated: {}", updatedEntries.size(), String.join(", ", updatedEntries)); - // Show notification without the citation keys - String message = Localization.lang("%0 entries updated", updatedEntries.size()); - dialogService.notify(message); - }); + return SUPPORTED_FIELDS.stream() + .map(field -> tryFetch(entry, field)) + .filter(Optional::isPresent) + .map(Optional::get) + .findFirst(); + } - backgroundTask.onFailure(exception -> { - LOGGER.error("Error fetching and merging entries", exception); - dialogService.notify(Localization.lang("Error fetching and merging entries")); - }); + private Optional tryFetch(BibEntry entry, Field field) { + return entry.getField(field) + .flatMap(identifier -> getFetcherForField(field) + .flatMap(fetcher -> fetchEntry(fetcher, field, identifier, entry))); + } - return backgroundTask; + private Optional fetchEntry(IdBasedFetcher fetcher, Field field, + String identifier, BibEntry originalEntry) { + try { + LOGGER.debug("Attempting to fetch entry using {} fetcher", field); + logEntryDetails(originalEntry, field, identifier); + + return fetcher.performSearchById(identifier) + .map(fetchedEntry -> { + BibEntry mergedEntry = (BibEntry) originalEntry.clone(); + boolean hasChanges = mergeBibEntry(mergedEntry, fetchedEntry); + return new FetcherResult(originalEntry, mergedEntry, hasChanges); + }); + } catch (Exception exception) { + LOGGER.error("Error fetching entry with {} identifier: {}", + field, identifier, exception); + return Optional.empty(); + } } - private boolean fetchAndMergeEntry(BibEntry entry) { - for (Field field : List.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT)) { - Optional identifier = entry.getField(field); - if (identifier.isPresent()) { - Optional fetcherOpt = getFetcherForField(field); - if (fetcherOpt.isPresent()) { - try { - Optional fetchedEntry = fetcherOpt.get().performSearchById(identifier.get()); - if (fetchedEntry.isPresent()) { - return processFetchedEntry(fetchedEntry.get(), entry); - } - } catch (Exception exception) { - LOGGER.error("Error fetching entry with {} {}", field, identifier.get(), exception); - } - } + private boolean mergeBibEntry(BibEntry target, BibEntry source) { + boolean hasChanges = false; + for (Field field : source.getFields()) { + Optional sourceValue = source.getField(field); + Optional targetValue = target.getField(field); + if (sourceValue.isPresent() && !sourceValue.equals(targetValue)) { + target.setField(field, sourceValue.get()); + hasChanges = true; } } - return false; + return hasChanges; } private Optional getFetcherForField(Field field) { return switch (field) { - case StandardField.DOI -> Optional.of(new DoiFetcher(preferences.getImportFormatPreferences())); - case StandardField.ISBN -> Optional.of(new IsbnFetcher(preferences.getImportFormatPreferences())); - case StandardField.EPRINT -> Optional.of(new IacrEprintFetcher(preferences.getImportFormatPreferences())); + case StandardField.DOI -> Optional.of(new DoiFetcher(importFormatPreferences)); + case StandardField.ISBN -> Optional.of(new IsbnFetcher(importFormatPreferences)); + case StandardField.EPRINT -> Optional.of(new IacrEprintFetcher(importFormatPreferences)); default -> Optional.empty(); }; } - private boolean processFetchedEntry(BibEntry fetchedEntry, BibEntry originalEntry) { - ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()).doPostCleanup(fetchedEntry); + private void logEntryIdentifiers(BibEntry entry) { + List availableIds = Stream.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT) + .flatMap(field -> entry.getField(field) + .map(value -> String.format("%s: %s", field, value)) + .stream()) + .collect(Collectors.toList()); + + String citationKey = entry.getCitationKey() + .map(key -> "citation key: " + key) + .orElse("no citation key"); + + LOGGER.debug("Processing entry with {} and identifiers: {}", + citationKey, + availableIds.isEmpty() ? "none" : String.join(", ", availableIds)); + } + + private void logEntryDetails(BibEntry entry, Field field, String identifier) { + StringBuilder details = new StringBuilder(); + details.append(field).append(" identifier: ").append(identifier); - NamedCompound ce = new NamedCompound(Localization.lang("Merge entry")); - MergeEntriesHelper.mergeEntries(originalEntry, fetchedEntry, ce); + entry.getCitationKey().ifPresent(key -> details.append(", citation key: ").append(key)); + entry.getField(StandardField.TITLE).ifPresent(title -> details.append(", title: ").append(title)); + entry.getField(StandardField.AUTHOR).ifPresent(author -> details.append(", author: ").append(author)); + + LOGGER.debug("Entry details - {}", details); + } - if (ce.hasEdits()) { - ce.end(); - undoManager.addEdit(ce); - return true; + public record FetcherResult(BibEntry originalEntry, BibEntry mergedEntry, boolean hasChanges) { + public BibEntry getMergedEntry() { + return mergedEntry; } - return false; } } diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 71679afcd14..ce8b0a90547 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1330,7 +1330,6 @@ Help\ on\ Name\ Formatting=Help on Name Formatting Add\ new\ file\ type=Add new file type Original\ entry=Original entry -No\ information\ added=No information added Select\ at\ least\ one\ entry\ to\ manage\ keywords.=Select at least one entry to manage keywords. OpenDocument\ text=OpenDocument text OpenDocument\ spreadsheet=OpenDocument spreadsheet @@ -1678,12 +1677,14 @@ Get\ bibliographic\ data\ from\ %0=Get bibliographic data from %0 No\ %0\ found=No %0 found Entry\ from\ %0=Entry from %0 Merge\ entry\ with\ %0\ information=Merge entry with %0 information +Batch\ update\ successful.\ %0\ entries\ updated\:\ %1.=Batch update successful. %0 entries updated: %1. +Get\ bibliographic\ data\ from\ %0\ (fully\ automated)=Get bibliographic data from %0 (fully automated) +Error\ while\ fetching\ and\ merging\ entries\:\ %0=Error while fetching and merging entries: %0 +Fetching\ and\ merging\ entries=Fetching and merging entries +Fetching\ entry\ %d\ of\ %d=Fetching entry %d of %d +No\ entries\ exist.=No entries exist. +No\ updates\ found.=No updates found. Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 -Mass\ getting\ bibliographic\ data\ from\ %0=Mass getting bibliographic data from %0 -Merge\ entry\ without\ user\ interaction=Merge entry without user interaction -No\ new\ information\ was\ added=No new information was added -Updated\ entry\ with\ fetched\ information=Updated entry with fetched information - Add\ new\ list=Add new list Open\ existing\ list=Open existing list Remove\ list=Remove list @@ -2496,7 +2497,6 @@ Error\ downloading=Error downloading No\ data\ was\ found\ for\ the\ identifier=No data was found for the identifier Server\ not\ available=Server not available -Fetching\ information\ using\ %0=Fetching information using %0 Look\ up\ identifier=Look up identifier Bibliographic\ data\ not\ found.\ Cause\ is\ likely\ the\ client\ side.\ Please\ check\ connection\ and\ identifier\ for\ correctness.=Bibliographic data not found. Cause is likely the client side. Please check connection and identifier for correctness. Bibliographic\ data\ not\ found.\ Cause\ is\ likely\ the\ server\ side.\ Please\ try\ again\ later.=Bibliographic data not found. Cause is likely the server side. Please try again later. From fe46c0ccff5a023c203c53184046f5f7b734d058 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Fri, 25 Oct 2024 18:43:49 +1100 Subject: [PATCH 12/28] Refactored updateFieldIfNecessaryWithUndo method in MergeEntriesHelper class --- .../gui/mergeentries/MergeEntriesHelper.java | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java index 471e4acee51..0a0bb9d7ccc 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -72,15 +72,20 @@ private static void updateFieldsWithNewInfo(final BibEntry originalEntry, private static void updateFieldIfNecessaryWithUndo(final BibEntry originalEntry, final BibEntry fetchedEntry, - final Field field, final NamedCompound undoManager) { - fetchedEntry.getField(field).ifPresent(fetchedValue -> originalEntry.getField(field).ifPresentOrElse( - originalValue -> { - if (fetchedValue.length() > originalValue.length()) { - updateField(originalEntry, field, originalValue, fetchedValue, undoManager); - } - }, - () -> updateField(originalEntry, field, null, fetchedValue, undoManager) - )); + final Field field, + final NamedCompound undoManager) { + fetchedEntry.getField(field) + .ifPresent(fetchedValue -> + originalEntry.getField(field) + .map(originalValue -> fetchedValue.length() > originalValue.length()) + .filter(shouldUpdate -> shouldUpdate) + .or(() -> { + updateField(originalEntry, field, + originalEntry.getField(field).orElse(null), + fetchedValue, + undoManager); + return Optional.empty(); + })); } private static void updateField(final BibEntry entry, final Field field, From 5d5408321dc17bb5ea314c745de41241748dc673 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Fri, 25 Oct 2024 19:07:51 +1100 Subject: [PATCH 13/28] refactor: Replace String.format with string concatenation in MergingIdBasedFetcher Switch from String.format to direct concatenation in logEntryIdentifiers method to address OpenRewrite StringFormatted warning. --- .../jabref/logic/importer/fetcher/MergingIdBasedFetcher.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index 94cda2378a9..45ad08f7329 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -87,7 +87,9 @@ private Optional getFetcherForField(Field field) { private void logEntryIdentifiers(BibEntry entry) { List availableIds = Stream.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT) .flatMap(field -> entry.getField(field) - .map(value -> String.format("%s: %s", field, value)) + .map(value -> field + + ": " + + value) .stream()) .collect(Collectors.toList()); From 93072cf5814c551a350d56a0bd1e19dbaba93d11 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Fri, 25 Oct 2024 19:46:23 +1100 Subject: [PATCH 14/28] refactor: Fixed incorrect placeholder in updateProgress Method --- .../gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java index 0a8927b8488..4cad7fe167c 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java @@ -140,7 +140,7 @@ private void logEntryDetails(BibEntry entry) { private void updateProgress(int currentIndex, int totalEntries, BackgroundTask task) { Platform.runLater(() -> { - task.updateMessage(Localization.lang("Fetching entry %d of %d", currentIndex + 1, totalEntries)); + task.updateMessage(Localization.lang("Fetching entry %0 of %1", currentIndex + 1, totalEntries)); task.updateProgress(currentIndex, totalEntries); }); } From 262d25d87a1afd9b171e78a0812961a42554ff7f Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 26 Oct 2024 01:36:34 +1100 Subject: [PATCH 15/28] Updated JabRef_en.properties to fix the failing LocalizationConsistencyTest --- src/main/resources/l10n/JabRef_en.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index ee306be5850..0a9d934037a 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1682,9 +1682,9 @@ Batch\ update\ successful.\ %0\ entries\ updated\:\ %1.=Batch update successful. Get\ bibliographic\ data\ from\ %0\ (fully\ automated)=Get bibliographic data from %0 (fully automated) Error\ while\ fetching\ and\ merging\ entries\:\ %0=Error while fetching and merging entries: %0 Fetching\ and\ merging\ entries=Fetching and merging entries -Fetching\ entry\ %d\ of\ %d=Fetching entry %d of %d No\ entries\ exist.=No entries exist. No\ updates\ found.=No updates found. +Fetching\ entry\ %0\ of\ %1=Fetching entry %0 of %1 Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 Add\ new\ list=Add new list Open\ existing\ list=Open existing list From ae4df1491b1f1414fbf4a7de862dd53082fb661b Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 26 Oct 2024 22:47:13 +1100 Subject: [PATCH 16/28] refactor: optimize BibEntry merging and fetching logic: MergingIdBasedFetcher: - Replace custom logging methods with BibEntry's toString() for simpler debugging - Reorder mergeBibEntry parameters to follow source->target convention - Implement safer merge strategy that preserves existing library entry data - Remove unnecessary getMergedEntry() getter from FetcherResult record - Rename originalEntry to entryFromLibrary to align with library terminology MultiEntryMergeWithFetchedDataAction: - Convert MergeContext to a record for better immutability - Make helper methods static where possible - Simplify background task configuration using builder pattern - Consolidate Platform.runLater() calls for better UI thread management - Improve progress messaging and error handling - Move NotificationService to MergeContext for better state management MergeEntriesHelper: - Rename parameters for clarity of data flow - Simplify nested Optional chains and field update logic - Consolidate related operations into clearer methods - Improve field comparison efficiency - Optimize set operations for field management Updated Name of the Action, using Batch instead of Mass Updated JabRef_en.properties Updated CHANGELOG.md --- CHANGELOG.md | 2 +- .../jabref/gui/actions/StandardActions.java | 2 +- .../java/org/jabref/gui/frame/MainMenu.java | 2 +- .../gui/mergeentries/MergeEntriesHelper.java | 140 +++++++---------- .../MultiEntryMergeWithFetchedDataAction.java | 148 +++++++++--------- .../fetcher/MergingIdBasedFetcher.java | 59 ++----- src/main/resources/l10n/JabRef_en.properties | 4 +- 7 files changed, 144 insertions(+), 213 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b8e72b2417..347be76338a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) -- Added functionality for mass addition of bibliographic information for multiple entries to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) +- Added batch fetching of bibliographic data for multiple entries in the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) ### Changed diff --git a/src/main/java/org/jabref/gui/actions/StandardActions.java b/src/main/java/org/jabref/gui/actions/StandardActions.java index 096406bd75c..2e98d027bcd 100644 --- a/src/main/java/org/jabref/gui/actions/StandardActions.java +++ b/src/main/java/org/jabref/gui/actions/StandardActions.java @@ -37,7 +37,7 @@ public enum StandardActions implements Action { OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI), SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")), MERGE_WITH_FETCHED_ENTRY(Localization.lang("Get bibliographic data from %0", "DOI/ISBN/...")), - MASS_GET_BIBLIOGRAPHIC_DATA(Localization.lang("Get bibliographic data from %0 (fully automated)", "DOI/ISBN/...")), + BATCH_MERGE_WITH_FETCHED_ENTRY(Localization.lang("Get bibliographic data from %0 (fully automated)", "DOI/ISBN/...")), ATTACH_FILE(Localization.lang("Attach file"), IconTheme.JabRefIcons.ATTACH_FILE), ATTACH_FILE_FROM_URL(Localization.lang("Attach file from URL"), IconTheme.JabRefIcons.DOWNLOAD_FILE), PRIORITY(Localization.lang("Priority"), IconTheme.JabRefIcons.PRIORITY), diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java index 617e63b448b..ad4b6a2b553 100644 --- a/src/main/java/org/jabref/gui/frame/MainMenu.java +++ b/src/main/java/org/jabref/gui/frame/MainMenu.java @@ -279,7 +279,7 @@ private void createMenu() { new SeparatorMenuItem(), factory.createMenuItem( - StandardActions.MASS_GET_BIBLIOGRAPHIC_DATA, + StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRY, new MultiEntryMergeWithFetchedDataAction( frame::getCurrentLibraryTab, preferences, diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java index 0a0bb9d7ccc..50d33109fc6 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -2,8 +2,6 @@ package org.jabref.gui.mergeentries; import java.util.LinkedHashSet; -import java.util.Objects; -import java.util.Optional; import java.util.Set; import org.jabref.gui.undo.NamedCompound; @@ -18,116 +16,86 @@ import org.slf4j.LoggerFactory; /** - * Helper class for merging Entries. + * Helper class for merging bibliography entries with undo support. + * Source entry data is merged into the library entry, with longer field values preferred + * and obsolete fields removed. */ public final class MergeEntriesHelper { private static final Logger LOGGER = LoggerFactory.getLogger(MergeEntriesHelper.class); private MergeEntriesHelper() { - // Private constructor to prevent instantiation of utility class + // Private constructor to prevent instantiation } /** * Merges two BibEntry objects with undo support. - * Use this method when modifying entries directly in the UI. - * The original entry will be updated with information from the fetched entry. + * Following the typical source -> target pattern, but with domain-specific naming. + * + * @param entryFromLibrary The entry to be updated (target, from the library) + * @param source The entry containing new information (source, from the fetcher) + * @param undoManager Compound edit to collect undo information */ - public static void mergeEntries(BibEntry originalEntry, BibEntry fetchedEntry, NamedCompound undoManager) { - Objects.requireNonNull(originalEntry, "Original entry cannot be null"); - Objects.requireNonNull(fetchedEntry, "Fetched entry cannot be null"); - Objects.requireNonNull(undoManager, "Undo manager cannot be null"); + public static void mergeEntries(BibEntry entryFromLibrary, BibEntry source, NamedCompound undoManager) { + LOGGER.debug("Entry from library: {}", entryFromLibrary); + LOGGER.debug("Source entry: {}", source); - LOGGER.debug("Starting merge of entries. Original type: {}, Fetched type: {}", - originalEntry.getType(), fetchedEntry.getType()); - - updateEntryTypeIfDifferent(originalEntry, fetchedEntry, undoManager); - updateFieldsWithNewInfo(originalEntry, fetchedEntry, undoManager); - removeObsoleteFields(originalEntry, fetchedEntry, undoManager); - - LOGGER.debug("Finished merging entries"); + mergeEntryType(source, entryFromLibrary, undoManager); + mergeFields(source, entryFromLibrary, undoManager); + removeObsoleteFields(source, entryFromLibrary, undoManager); } - private static void updateEntryTypeIfDifferent(final BibEntry originalEntry, - final BibEntry fetchedEntry, final NamedCompound undoManager) { - EntryType oldType = originalEntry.getType(); - EntryType newType = fetchedEntry.getType(); + private static void mergeEntryType(BibEntry source, BibEntry entryFromLibrary, NamedCompound undoManager) { + EntryType libraryType = entryFromLibrary.getType(); + EntryType sourceType = source.getType(); - if (!oldType.equals(newType)) { - LOGGER.debug("Updating entry type from {} to {}", oldType, newType); - originalEntry.setType(newType); - undoManager.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); + if (!libraryType.equals(sourceType)) { + LOGGER.debug("Updating type {} -> {}", libraryType, sourceType); + entryFromLibrary.setType(sourceType); + undoManager.addEdit(new UndoableChangeType(entryFromLibrary, libraryType, sourceType)); } } - private static void updateFieldsWithNewInfo(final BibEntry originalEntry, - final BibEntry fetchedEntry, final NamedCompound undoManager) { - Set jointFields = getJointFields(originalEntry, fetchedEntry); - LOGGER.debug("Processing {} joint fields for updates", jointFields.size()); - - for (Field field : jointFields) { - updateFieldIfNecessaryWithUndo(originalEntry, fetchedEntry, field, undoManager); - } - } - - private static void updateFieldIfNecessaryWithUndo(final BibEntry originalEntry, - final BibEntry fetchedEntry, - final Field field, - final NamedCompound undoManager) { - fetchedEntry.getField(field) - .ifPresent(fetchedValue -> - originalEntry.getField(field) - .map(originalValue -> fetchedValue.length() > originalValue.length()) - .filter(shouldUpdate -> shouldUpdate) - .or(() -> { - updateField(originalEntry, field, - originalEntry.getField(field).orElse(null), - fetchedValue, - undoManager); - return Optional.empty(); - })); - } - - private static void updateField(final BibEntry entry, final Field field, - final String originalValue, final String newValue, final NamedCompound undoManager) { - LOGGER.debug("Updating field '{}' from '{}' to '{}'", field, originalValue, newValue); - entry.setField(field, newValue); - undoManager.addEdit(new UndoableFieldChange(entry, field, originalValue, newValue)); - } - - private static void removeObsoleteFields(final BibEntry originalEntry, - final BibEntry fetchedEntry, - final NamedCompound undoManager) { - - Set jointFields = getJointFields(originalEntry, fetchedEntry); - Set originalFields = getFields(originalEntry); + private static void mergeFields(BibEntry source, BibEntry entryFromLibrary, NamedCompound undoManager) { + Set allFields = new LinkedHashSet<>(); + allFields.addAll(source.getFields()); + allFields.addAll(entryFromLibrary.getFields()); - LOGGER.debug("Checking {} original fields for obsolete entries", originalFields.size()); + for (Field field : allFields) { + String sourceValue = source.getField(field).orElse(null); + if (sourceValue == null) { + continue; + } - for (Field field : originalFields) { - if (!jointFields.contains(field) && !FieldFactory.isInternalField(field)) { - removeFieldWithUndo(originalEntry, field, undoManager); + String libraryValue = entryFromLibrary.getField(field).orElse(null); + if (shouldUpdateField(libraryValue, sourceValue)) { + LOGGER.debug("Updating field {}: {} -> {}", field, libraryValue, sourceValue); + entryFromLibrary.setField(field, sourceValue); + undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue, sourceValue)); } } } - private static void removeFieldWithUndo(final BibEntry entry, final Field field, - final NamedCompound undoManager) { - Optional originalValue = entry.getField(field); - LOGGER.debug("Removing obsolete field '{}' with value '{}'", field, originalValue.orElse(null)); - - entry.clearField(field); - undoManager.addEdit(new UndoableFieldChange(entry, field, originalValue.orElse(null), null)); + private static boolean shouldUpdateField(String libraryValue, String sourceValue) { + if (libraryValue == null) { + return true; + } + return sourceValue.length() > libraryValue.length(); } - private static Set getFields(final BibEntry entry) { - return new LinkedHashSet<>(entry.getFields()); - } + private static void removeObsoleteFields(BibEntry source, BibEntry entryFromLibrary, NamedCompound undoManager) { + Set obsoleteFields = new LinkedHashSet<>(entryFromLibrary.getFields()); + obsoleteFields.removeAll(source.getFields()); + + for (Field field : obsoleteFields) { + if (FieldFactory.isInternalField(field)) { + continue; + } - private static Set getJointFields(final BibEntry entry1, final BibEntry entry2) { - Set fields = new LinkedHashSet<>(); - fields.addAll(getFields(entry1)); - fields.addAll(getFields(entry2)); - return fields; + String value = entryFromLibrary.getField(field).orElse(null); + LOGGER.debug("Removing obsolete field {} with value {}", field, value); + entryFromLibrary.clearField(field); + undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, value, null)); + } } } diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java index 4cad7fe167c..adfbf0ddd78 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java @@ -3,9 +3,7 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.function.Supplier; @@ -23,7 +21,6 @@ import org.jabref.logic.util.NotificationService; import org.jabref.logic.util.TaskExecutor; import org.jabref.model.entry.BibEntry; -import org.jabref.model.entry.field.StandardField; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -68,126 +65,121 @@ public void execute() { return; } - MergeContext context = new MergeContext(libraryTab, entries, createMergingFetcher()); - BackgroundTask> backgroundTask = createBackgroundTask(context); - taskExecutor.execute(backgroundTask); - } + MergeContext context = new MergeContext( + libraryTab, + entries, + new MergingIdBasedFetcher(preferences.getImportFormatPreferences()), + notificationService + ); - private MergingIdBasedFetcher createMergingFetcher() { - return new MergingIdBasedFetcher(preferences.getImportFormatPreferences()); + taskExecutor.execute(createBackgroundTask(context)); } - private BackgroundTask> createBackgroundTask(MergeContext context) { + private static BackgroundTask> createBackgroundTask(MergeContext context) { BackgroundTask> task = new BackgroundTask<>() { @Override - public List call() { + public List call() { return processMergeEntries(context, this); } }; - configureBackgroundTask(task); + + task.setTitle(Localization.lang("Fetching and merging entries")); + task.showToUser(true); + + task.onSuccess(updatedEntries -> + handleSuccess(updatedEntries, context)); + + task.onFailure(ex -> + handleFailure(ex, context.notificationService)); + return task; } - private List processMergeEntries(MergeContext context, BackgroundTask task) { - if (task.isCancelled()) { - return List.of(); - } + private static List processMergeEntries(MergeContext context, BackgroundTask task) { + int totalEntries = context.entries().size(); - int totalEntries = context.entries.size(); for (int i = 0; i < totalEntries && !task.isCancelled(); i++) { - processEntry(context, context.entries.get(i), i, totalEntries, task); + BibEntry entry = context.entries().get(i); + updateProgress(i, totalEntries, entry, task); + processEntry(context, entry); } finalizeCompoundEdit(context); - return context.updatedEntries; + return context.updatedEntries(); } - private void processEntry(MergeContext context, BibEntry entry, int currentIndex, - int totalEntries, BackgroundTask task) { - updateProgress(currentIndex, totalEntries, task); - logEntryDetails(entry); + private static void processEntry(MergeContext context, BibEntry entry) { + LOGGER.debug("Processing entry: {}", entry); - Optional fetchResult = context.fetcher.fetchEntry(entry); + Optional fetchResult = context.fetcher().fetchEntry(entry); fetchResult.ifPresent(result -> { if (result.hasChanges()) { Platform.runLater(() -> { - // UI operations must be done on the FX thread - MergeEntriesHelper.mergeEntries(entry, result.mergedEntry(), context.compoundEdit); - entry.getCitationKey().ifPresent(context.updatedEntries::add); + MergeEntriesHelper.mergeEntries(entry, result.mergedEntry(), context.compoundEdit()); + entry.getCitationKey().ifPresent(context.updatedEntries()::add); }); } }); } - private void finalizeCompoundEdit(MergeContext context) { - if (!context.updatedEntries.isEmpty()) { + private static void finalizeCompoundEdit(MergeContext context) { + if (!context.updatedEntries().isEmpty()) { Platform.runLater(() -> { - context.compoundEdit.end(); - context.libraryTab.getUndoManager().addEdit(context.compoundEdit); + context.compoundEdit().end(); + context.libraryTab().getUndoManager().addEdit(context.compoundEdit()); }); } } - private void logEntryDetails(BibEntry entry) { - Map details = new HashMap<>(); - entry.getCitationKey().ifPresent(key -> details.put("key", key)); - entry.getField(StandardField.ISBN).ifPresent(isbn -> details.put("isbn", isbn)); - entry.getField(StandardField.DOI).ifPresent(doi -> details.put("doi", doi)); - details.put("type", entry.getType().getName()); - - LOGGER.info("Processing BibEntry: {}", details); - } - - private void updateProgress(int currentIndex, int totalEntries, BackgroundTask task) { + private static void updateProgress(int currentIndex, int totalEntries, BibEntry entry, BackgroundTask task) { + LOGGER.debug("Processing entry {}", entry); Platform.runLater(() -> { task.updateMessage(Localization.lang("Fetching entry %0 of %1", currentIndex + 1, totalEntries)); task.updateProgress(currentIndex, totalEntries); }); } - private void configureBackgroundTask(BackgroundTask> task) { - task.setTitle(Localization.lang("Fetching and merging entries")); - task.showToUser(true); - configureSuccessHandler(task); - configureFailureHandler(task); - } - - private void configureSuccessHandler(BackgroundTask> task) { - task.onSuccess(updatedEntries -> Platform.runLater(() -> { + private static void handleSuccess(List updatedEntries, MergeContext context) { + Platform.runLater(() -> { if (updatedEntries.isEmpty()) { - LOGGER.info("Batch update completed. No entries were updated."); - notificationService.notify(Localization.lang("No updates found.")); + LOGGER.debug("Batch update completed. No entries were updated."); + context.notificationService().notify(Localization.lang("No updates found.")); } else { - String message = Localization.lang("Batch update successful. %0 entries updated: %1.", - updatedEntries.size(), String.join(", ", updatedEntries)); - notificationService.notify(message); + LOGGER.debug("Updated entries: {}", String.join(", ", updatedEntries)); + + String message = Localization.lang("Batch update successful. %0 entries updated.", + String.valueOf(updatedEntries.size())); + context.notificationService().notify(message); } - })); + }); } - private void configureFailureHandler(BackgroundTask> task) { - task.onFailure(ex -> { - String errorType = ex.getClass().getSimpleName(); - LOGGER.error("{}: {}", errorType, ex.getMessage(), ex); - Platform.runLater(() -> - notificationService.notify(Localization.lang("Error while fetching and merging entries: %0", ex.getMessage())) - ); - }); + private static void handleFailure(Exception ex, NotificationService notificationService) { + LOGGER.error("Error during fetch and merge", ex); + Platform.runLater(() -> + notificationService.notify( + Localization.lang("Error while fetching and merging entries: %0", ex.getMessage()) + ) + ); } - private static class MergeContext { - final LibraryTab libraryTab; - final List entries; - final MergingIdBasedFetcher fetcher; - final NamedCompound compoundEdit; - final List updatedEntries; - - MergeContext(LibraryTab libraryTab, List entries, MergingIdBasedFetcher fetcher) { - this.libraryTab = libraryTab; - this.entries = entries; - this.fetcher = fetcher; - this.compoundEdit = new NamedCompound(Localization.lang("Merge entries")); - this.updatedEntries = Collections.synchronizedList(new ArrayList<>()); + private record MergeContext( + LibraryTab libraryTab, + List entries, + MergingIdBasedFetcher fetcher, + NamedCompound compoundEdit, + List updatedEntries, + NotificationService notificationService + ) { + MergeContext(LibraryTab libraryTab, List entries, MergingIdBasedFetcher fetcher, NotificationService notificationService) { + this( + libraryTab, + entries, + fetcher, + new NamedCompound(Localization.lang("Merge entries")), + Collections.synchronizedList(new ArrayList<>()), + notificationService + ); } } } diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index 45ad08f7329..f9d5dd15b02 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -3,8 +3,6 @@ import java.util.List; import java.util.Optional; -import java.util.stream.Collectors; -import java.util.stream.Stream; import org.jabref.logic.importer.IdBasedFetcher; import org.jabref.logic.importer.ImportFormatPreferences; @@ -16,6 +14,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * This class fetches bibliographic information from external sources based on the identifiers + * (e.g. DOI, ISBN, arXiv ID) of a {@link BibEntry} and merges the fetched information into the entry. + */ public class MergingIdBasedFetcher { private static final List SUPPORTED_FIELDS = List.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT); @@ -28,7 +30,7 @@ public MergingIdBasedFetcher(ImportFormatPreferences importFormatPreferences) { } public Optional fetchEntry(BibEntry entry) { - logEntryIdentifiers(entry); + LOGGER.debug("Entry {}", entry); return SUPPORTED_FIELDS.stream() .map(field -> tryFetch(entry, field)) @@ -44,16 +46,16 @@ private Optional tryFetch(BibEntry entry, Field field) { } private Optional fetchEntry(IdBasedFetcher fetcher, Field field, - String identifier, BibEntry originalEntry) { + String identifier, BibEntry entryFromLibrary) { try { - LOGGER.debug("Attempting to fetch entry using {} fetcher", field); - logEntryDetails(originalEntry, field, identifier); + LOGGER.debug("Entry {}", + entryFromLibrary); return fetcher.performSearchById(identifier) .map(fetchedEntry -> { - BibEntry mergedEntry = (BibEntry) originalEntry.clone(); - boolean hasChanges = mergeBibEntry(mergedEntry, fetchedEntry); - return new FetcherResult(originalEntry, mergedEntry, hasChanges); + BibEntry mergedEntry = (BibEntry) entryFromLibrary.clone(); + boolean hasChanges = mergeBibEntry(fetchedEntry, mergedEntry); + return new FetcherResult(entryFromLibrary, mergedEntry, hasChanges); }); } catch (Exception exception) { LOGGER.error("Error fetching entry with {} identifier: {}", @@ -62,12 +64,13 @@ private Optional fetchEntry(IdBasedFetcher fetcher, Field field, } } - private boolean mergeBibEntry(BibEntry target, BibEntry source) { + private boolean mergeBibEntry(BibEntry source, BibEntry target) { boolean hasChanges = false; for (Field field : source.getFields()) { Optional sourceValue = source.getField(field); Optional targetValue = target.getField(field); - if (sourceValue.isPresent() && !sourceValue.equals(targetValue)) { + + if (sourceValue.isPresent() && targetValue.isEmpty()) { target.setField(field, sourceValue.get()); hasChanges = true; } @@ -84,38 +87,6 @@ private Optional getFetcherForField(Field field) { }; } - private void logEntryIdentifiers(BibEntry entry) { - List availableIds = Stream.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT) - .flatMap(field -> entry.getField(field) - .map(value -> field + - ": " + - value) - .stream()) - .collect(Collectors.toList()); - - String citationKey = entry.getCitationKey() - .map(key -> "citation key: " + key) - .orElse("no citation key"); - - LOGGER.debug("Processing entry with {} and identifiers: {}", - citationKey, - availableIds.isEmpty() ? "none" : String.join(", ", availableIds)); - } - - private void logEntryDetails(BibEntry entry, Field field, String identifier) { - StringBuilder details = new StringBuilder(); - details.append(field).append(" identifier: ").append(identifier); - - entry.getCitationKey().ifPresent(key -> details.append(", citation key: ").append(key)); - entry.getField(StandardField.TITLE).ifPresent(title -> details.append(", title: ").append(title)); - entry.getField(StandardField.AUTHOR).ifPresent(author -> details.append(", author: ").append(author)); - - LOGGER.debug("Entry details - {}", details); - } - - public record FetcherResult(BibEntry originalEntry, BibEntry mergedEntry, boolean hasChanges) { - public BibEntry getMergedEntry() { - return mergedEntry; - } + public record FetcherResult(BibEntry entryFromLibrary, BibEntry mergedEntry, boolean hasChanges) { } } diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 13b7fbdf0db..6d0dbfcf546 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1678,13 +1678,13 @@ Get\ bibliographic\ data\ from\ %0=Get bibliographic data from %0 No\ %0\ found=No %0 found Entry\ from\ %0=Entry from %0 Merge\ entry\ with\ %0\ information=Merge entry with %0 information -Batch\ update\ successful.\ %0\ entries\ updated\:\ %1.=Batch update successful. %0 entries updated: %1. Get\ bibliographic\ data\ from\ %0\ (fully\ automated)=Get bibliographic data from %0 (fully automated) +Batch\ update\ successful.\ %0\ entries\ updated.=Batch update successful. %0 entries updated. +Fetching\ entry\ %0\ of\ %1=Fetching entry %0 of %1 Error\ while\ fetching\ and\ merging\ entries\:\ %0=Error while fetching and merging entries: %0 Fetching\ and\ merging\ entries=Fetching and merging entries No\ entries\ exist.=No entries exist. No\ updates\ found.=No updates found. -Fetching\ entry\ %0\ of\ %1=Fetching entry %0 of %1 Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 Add\ new\ list=Add new list Open\ existing\ list=Open existing list From 5ed3d7024bc5d6ec97cb0e623dd8a3dd89e79eb4 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 27 Oct 2024 12:27:43 +1100 Subject: [PATCH 17/28] Roll Back: FetchAndMergeEntry.java and MergeWithFetchedEntryAction.java. --- .../gui/mergeentries/FetchAndMergeEntry.java | 206 ++++++++++-------- .../MergeWithFetchedEntryAction.java | 13 +- 2 files changed, 118 insertions(+), 101 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index c18177604fa..869a3a7e83b 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -2,15 +2,23 @@ package org.jabref.gui.mergeentries; import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; import java.util.List; import java.util.Optional; +import java.util.Set; +import java.util.TreeSet; import javax.swing.undo.UndoManager; import org.jabref.gui.DialogService; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.undo.NamedCompound; +import org.jabref.gui.undo.UndoableChangeType; +import org.jabref.gui.undo.UndoableFieldChange; import org.jabref.logic.importer.EntryBasedFetcher; +import org.jabref.logic.importer.FetcherClientException; +import org.jabref.logic.importer.FetcherServerException; import org.jabref.logic.importer.IdBasedFetcher; import org.jabref.logic.importer.ImportCleanup; import org.jabref.logic.importer.WebFetcher; @@ -21,18 +29,22 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.FieldFactory; import org.jabref.model.entry.field.StandardField; +import org.jabref.model.entry.types.EntryType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * Class for fetching and merging bibliographic information + */ public class FetchAndMergeEntry { - - public static final List SUPPORTED_IDENTIFIER_FIELDS = Arrays.asList(StandardField.DOI, StandardField.EPRINT, StandardField.ISBN); + // All identifiers listed here should also appear at {@link org.jabref.logic.importer.CompositeIdFetcher#performSearchById} + public static List SUPPORTED_FIELDS = Arrays.asList(StandardField.DOI, StandardField.EPRINT, StandardField.ISBN); private static final Logger LOGGER = LoggerFactory.getLogger(FetchAndMergeEntry.class); - private final DialogService dialogService; private final UndoManager undoManager; private final BibDatabaseContext bibDatabaseContext; @@ -52,112 +64,124 @@ public FetchAndMergeEntry(BibDatabaseContext bibDatabaseContext, } public void fetchAndMerge(BibEntry entry) { - fetchAndMerge(entry, SUPPORTED_IDENTIFIER_FIELDS); + fetchAndMerge(entry, SUPPORTED_FIELDS); } public void fetchAndMerge(BibEntry entry, Field field) { - fetchAndMerge(entry, List.of(field)); + fetchAndMerge(entry, Collections.singletonList(field)); } public void fetchAndMerge(BibEntry entry, List fields) { - fields.forEach(field -> fetchAndMergeEntry(entry, field)); - } - - private void fetchAndMergeEntry(BibEntry entry, Field field) { - entry.getField(field) - .flatMap(fieldContent -> WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences())) - .ifPresent(fetcher -> executeFetchTask(fetcher, field, entry)); - } - - private void executeFetchTask(IdBasedFetcher fetcher, Field field, BibEntry entry) { - entry.getField(field).ifPresent(fieldContent -> - BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) - .onSuccess(fetchedEntry -> processFetchedEntry(fetchedEntry, entry, fetcher)) - .onFailure(exception -> handleFetchException(exception, fetcher)) - .executeWith(taskExecutor) - ); - } - - private void processFetchedEntry(Optional fetchedEntry, BibEntry originalEntry, IdBasedFetcher fetcher) { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - fetchedEntry.ifPresentOrElse( - entry -> { - cleanup.doPostCleanup(entry); - showMergeDialog(originalEntry, entry, fetcher); - }, - () -> notifyNoInfo(originalEntry) - ); - } - - private void notifyNoInfo(BibEntry entry) { - dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", - entry.getType().getDisplayName(), - entry.getCitationKey().orElse(""))); - } - - private void handleFetchException(Exception exception, WebFetcher fetcher) { - LOGGER.error("Error while fetching bibliographic information", exception); - dialogService.showErrorDialogAndWait( - Localization.lang("Error while fetching from %0", fetcher.getName()), - exception - ); + for (Field field : fields) { + Optional fieldContent = entry.getField(field); + if (fieldContent.isPresent()) { + Optional fetcher = WebFetchers.getIdBasedFetcherForField(field, preferences.getImportFormatPreferences()); + if (fetcher.isPresent()) { + BackgroundTask.wrap(() -> fetcher.get().performSearchById(fieldContent.get())) + .onSuccess(fetchedEntry -> { + ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); + String type = field.getDisplayName(); + if (fetchedEntry.isPresent()) { + cleanup.doPostCleanup(fetchedEntry.get()); + showMergeDialog(entry, fetchedEntry.get(), fetcher.get()); + } else { + dialogService.notify(Localization.lang("Cannot get info based on given %0: %1", type, fieldContent.get())); + } + }) + .onFailure(exception -> { + LOGGER.error("Error while fetching bibliographic information", exception); + if (exception instanceof FetcherClientException) { + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("No data was found for the identifier")); + } else if (exception instanceof FetcherServerException) { + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Server not available")); + } else { + dialogService.showInformationDialogAndWait(Localization.lang("Fetching information using %0", fetcher.get().getName()), Localization.lang("Error occurred %0", exception.getMessage())); + } + }) + .executeWith(taskExecutor); + } + } else { + dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); + } + } } private void showMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { - MergeEntriesDialog dialog = createMergeDialog(originalEntry, fetchedEntry, fetcher); - Optional mergedEntry = dialogService.showCustomDialogAndWait(dialog) - .map(EntriesMergeResult::mergedEntry); - - mergedEntry.ifPresentOrElse( - entry -> processMergedEntry(originalEntry, entry, fetcher), - () -> notifyCanceledMerge(originalEntry) - ); - } - - private MergeEntriesDialog createMergeDialog(BibEntry originalEntry, BibEntry fetchedEntry, WebFetcher fetcher) { MergeEntriesDialog dialog = new MergeEntriesDialog(originalEntry, fetchedEntry, preferences); dialog.setTitle(Localization.lang("Merge entry with %0 information", fetcher.getName())); dialog.setLeftHeaderText(Localization.lang("Original entry")); dialog.setRightHeaderText(Localization.lang("Entry from %0", fetcher.getName())); - return dialog; - } - - private void processMergedEntry(BibEntry originalEntry, BibEntry mergedEntry, WebFetcher fetcher) { - NamedCompound ce = new NamedCompound(Localization.lang("Merge entry with %0 information", fetcher.getName())); - MergeEntriesHelper.mergeEntries(originalEntry, mergedEntry, ce); - - if (ce.hasEdits()) { - ce.end(); - undoManager.addEdit(ce); + Optional mergedEntry = dialogService.showCustomDialogAndWait(dialog).map(EntriesMergeResult::mergedEntry); + + if (mergedEntry.isPresent()) { + NamedCompound ce = new NamedCompound(Localization.lang("Merge entry with %0 information", fetcher.getName())); + + // Updated the original entry with the new fields + Set jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); + jointFields.addAll(mergedEntry.get().getFields()); + Set 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); + ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); + edited = true; + } + + // fields + for (Field field : jointFields) { + Optional originalString = originalEntry.getField(field); + Optional mergedString = mergedEntry.get().getField(field); + if (originalString.isEmpty() || !originalString.equals(mergedString)) { + originalEntry.setField(field, mergedString.get()); // mergedString always present + ce.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 originalString = originalEntry.getField(field); + originalEntry.clearField(field); + ce.addEdit(new UndoableFieldChange(originalEntry, field, originalString.get(), null)); // originalString always present + edited = true; + } + } + + if (edited) { + ce.end(); + undoManager.addEdit(ce); + dialogService.notify(Localization.lang("Updated entry with info from %0", fetcher.getName())); + } else { + dialogService.notify(Localization.lang("No information added")); + } + } else { + dialogService.notify(Localization.lang("Canceled merging entries")); } - - dialogService.notify(Localization.lang("Updated entry with info from %0", fetcher.getName())); - } - - private void notifyCanceledMerge(BibEntry entry) { - String citationKey = entry.getCitationKey().orElse(entry.getAuthorTitleYear(40)); - dialogService.notify(Localization.lang("Canceled merging entries") + " [" + citationKey + "]"); } public void fetchAndMerge(BibEntry entry, EntryBasedFetcher fetcher) { BackgroundTask.wrap(() -> fetcher.performSearch(entry).stream().findFirst()) - .onSuccess(fetchedEntry -> processFetchedEntryForEntryBasedFetcher(fetchedEntry, entry, fetcher)) - .onFailure(exception -> handleFetchException(exception, fetcher)) + .onSuccess(fetchedEntry -> { + if (fetchedEntry.isPresent()) { + ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); + cleanup.doPostCleanup(fetchedEntry.get()); + showMergeDialog(entry, fetchedEntry.get(), fetcher); + } else { + dialogService.notify(Localization.lang("Could not find any bibliographic information.")); + } + }) + .onFailure(exception -> { + LOGGER.error("Error while fetching entry with {} ", fetcher.getName(), exception); + dialogService.showErrorDialogAndWait(Localization.lang("Error while fetching from %0", fetcher.getName()), exception); + }) .executeWith(taskExecutor); } - - private void processFetchedEntryForEntryBasedFetcher(Optional fetchedEntry, BibEntry originalEntry, EntryBasedFetcher fetcher) { - fetchedEntry - .map(this::cleanupFetchedEntry) - .ifPresentOrElse( - fe -> showMergeDialog(originalEntry, fe, fetcher), - () -> dialogService.notify(Localization.lang("Could not find any bibliographic information.")) - ); - } - - private BibEntry cleanupFetchedEntry(BibEntry fetchedEntry) { - ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode(), preferences.getFieldPreferences()); - cleanup.doPostCleanup(fetchedEntry); - return fetchedEntry; - } } diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java b/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java index a6d8d65a406..3e9a4d35e90 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java @@ -1,3 +1,4 @@ + package org.jabref.gui.mergeentries; import javax.swing.undo.UndoManager; @@ -33,7 +34,7 @@ public MergeWithFetchedEntryAction(DialogService dialogService, this.undoManager = undoManager; this.executable.bind(ActionHelper.needsEntriesSelected(1, stateManager) - .and(ActionHelper.isAnyFieldSetForSelectedEntry(FetchAndMergeEntry.SUPPORTED_IDENTIFIER_FIELDS, stateManager))); + .and(ActionHelper.isAnyFieldSetForSelectedEntry(FetchAndMergeEntry.SUPPORTED_FIELDS, stateManager))); } @Override @@ -49,14 +50,6 @@ public void execute() { } BibEntry originalEntry = stateManager.getSelectedEntries().getFirst(); - FetchAndMergeEntry fetchAndMergeEntry = new FetchAndMergeEntry( - stateManager.getActiveDatabase().get(), - taskExecutor, - preferences, - dialogService, - undoManager - ); - - fetchAndMergeEntry.fetchAndMerge(originalEntry); + new FetchAndMergeEntry(stateManager.getActiveDatabase().get(), taskExecutor, preferences, dialogService, undoManager).fetchAndMerge(originalEntry); } } From 8d9bdce4d997b2be3cd330d94606118b2e54de4c Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 27 Oct 2024 13:41:56 +1100 Subject: [PATCH 18/28] Refactor: improve MergingIdBasedFetcher, BatchEntryMergeWithFetchedDataAction and MergeEntriesHelper clarity - Renamed MultiEntryMergeWithFetchedDataAction.java to BatchEntryMergeWithFetchedDataAction.java - Optimized Logger messages for clarity - Refactored the names of parameters for clarity - Refactored code by using optionals - Optimized method name for clarity Updated JabRef_en.properties --- .../java/org/jabref/gui/frame/MainMenu.java | 4 +- ...BatchEntryMergeWithFetchedDataAction.java} | 54 +++++------- .../gui/mergeentries/MergeEntriesHelper.java | 78 ++++++++--------- .../fetcher/MergingIdBasedFetcher.java | 83 +++++++++++-------- src/main/resources/l10n/JabRef_en.properties | 5 +- 5 files changed, 115 insertions(+), 109 deletions(-) rename src/main/java/org/jabref/gui/mergeentries/{MultiEntryMergeWithFetchedDataAction.java => BatchEntryMergeWithFetchedDataAction.java} (74%) diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java index ad4b6a2b553..cb31d98b863 100644 --- a/src/main/java/org/jabref/gui/frame/MainMenu.java +++ b/src/main/java/org/jabref/gui/frame/MainMenu.java @@ -52,7 +52,7 @@ import org.jabref.gui.maintable.NewLibraryFromPdfActionOffline; import org.jabref.gui.maintable.NewLibraryFromPdfActionOnline; import org.jabref.gui.mergeentries.MergeEntriesAction; -import org.jabref.gui.mergeentries.MultiEntryMergeWithFetchedDataAction; +import org.jabref.gui.mergeentries.BatchEntryMergeWithFetchedDataAction; import org.jabref.gui.plaincitationparser.PlainCitationParserAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preferences.ShowPreferencesAction; @@ -280,7 +280,7 @@ private void createMenu() { factory.createMenuItem( StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRY, - new MultiEntryMergeWithFetchedDataAction( + new BatchEntryMergeWithFetchedDataAction( frame::getCurrentLibraryTab, preferences, dialogService, diff --git a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java similarity index 74% rename from src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java rename to src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java index adfbf0ddd78..b02b74fbec9 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java @@ -4,7 +4,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Optional; import java.util.function.Supplier; import javafx.application.Platform; @@ -30,16 +29,16 @@ * This action performs background fetching and merging of entries while * providing progress updates to the user. */ -public class MultiEntryMergeWithFetchedDataAction extends SimpleCommand { +public class BatchEntryMergeWithFetchedDataAction extends SimpleCommand { - private static final Logger LOGGER = LoggerFactory.getLogger(MultiEntryMergeWithFetchedDataAction.class); + private static final Logger LOGGER = LoggerFactory.getLogger(BatchEntryMergeWithFetchedDataAction.class); private final Supplier tabSupplier; private final GuiPreferences preferences; private final NotificationService notificationService; private final TaskExecutor taskExecutor; - public MultiEntryMergeWithFetchedDataAction(Supplier tabSupplier, + public BatchEntryMergeWithFetchedDataAction(Supplier tabSupplier, GuiPreferences preferences, NotificationService notificationService, StateManager stateManager, @@ -55,13 +54,13 @@ public MultiEntryMergeWithFetchedDataAction(Supplier tabSupplier, public void execute() { LibraryTab libraryTab = tabSupplier.get(); if (libraryTab == null) { - LOGGER.error("Action 'Multi Entry Merge' must be disabled when no database is open."); + LOGGER.error("Cannot perform batch merge: no library is open."); return; } List entries = libraryTab.getDatabase().getEntries(); if (entries.isEmpty()) { - notificationService.notify(Localization.lang("No entries exist.")); + notificationService.notify(Localization.lang("empty library")); return; } @@ -100,26 +99,23 @@ private static List processMergeEntries(MergeContext context, Background for (int i = 0; i < totalEntries && !task.isCancelled(); i++) { BibEntry entry = context.entries().get(i); - updateProgress(i, totalEntries, entry, task); - processEntry(context, entry); + updateProgress(i, totalEntries, task); + fetchAndMergeEntry(context, entry); } finalizeCompoundEdit(context); return context.updatedEntries(); } - private static void processEntry(MergeContext context, BibEntry entry) { + private static void fetchAndMergeEntry(MergeContext context, BibEntry entry) { LOGGER.debug("Processing entry: {}", entry); - Optional fetchResult = context.fetcher().fetchEntry(entry); - fetchResult.ifPresent(result -> { - if (result.hasChanges()) { - Platform.runLater(() -> { - MergeEntriesHelper.mergeEntries(entry, result.mergedEntry(), context.compoundEdit()); - entry.getCitationKey().ifPresent(context.updatedEntries()::add); - }); - } - }); + context.fetcher().fetchEntry(entry) + .filter(MergingIdBasedFetcher.FetcherResult::hasChanges) + .ifPresent(result -> Platform.runLater(() -> { + MergeEntriesHelper.mergeEntries(entry, result.mergedEntry(), context.compoundEdit()); + entry.getCitationKey().ifPresent(context.updatedEntries()::add); + })); } private static void finalizeCompoundEdit(MergeContext context) { @@ -131,26 +127,22 @@ private static void finalizeCompoundEdit(MergeContext context) { } } - private static void updateProgress(int currentIndex, int totalEntries, BibEntry entry, BackgroundTask task) { - LOGGER.debug("Processing entry {}", entry); + private static void updateProgress(int currentIndex, int totalEntries, BackgroundTask task) { Platform.runLater(() -> { - task.updateMessage(Localization.lang("Fetching entry %0 of %1", currentIndex + 1, totalEntries)); + task.updateMessage(Localization.lang("Processing entry %0 of %1", currentIndex + 1, totalEntries)); task.updateProgress(currentIndex, totalEntries); }); } private static void handleSuccess(List updatedEntries, MergeContext context) { Platform.runLater(() -> { - if (updatedEntries.isEmpty()) { - LOGGER.debug("Batch update completed. No entries were updated."); - context.notificationService().notify(Localization.lang("No updates found.")); - } else { - LOGGER.debug("Updated entries: {}", String.join(", ", updatedEntries)); - - String message = Localization.lang("Batch update successful. %0 entries updated.", - String.valueOf(updatedEntries.size())); - context.notificationService().notify(message); - } + String message = updatedEntries.isEmpty() + ? Localization.lang("No updates found.") + : Localization.lang("Batch update successful. %0 entries updated.", + updatedEntries.size()); + + LOGGER.debug("Batch update completed. {}", message); + context.notificationService().notify(message); }); } diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java index 50d33109fc6..8b565d4c8a0 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -2,6 +2,7 @@ package org.jabref.gui.mergeentries; import java.util.LinkedHashSet; +import java.util.Optional; import java.util.Set; import org.jabref.gui.undo.NamedCompound; @@ -25,77 +26,78 @@ public final class MergeEntriesHelper { private static final Logger LOGGER = LoggerFactory.getLogger(MergeEntriesHelper.class); private MergeEntriesHelper() { - // Private constructor to prevent instantiation } /** * Merges two BibEntry objects with undo support. - * Following the typical source -> target pattern, but with domain-specific naming. * * @param entryFromLibrary The entry to be updated (target, from the library) - * @param source The entry containing new information (source, from the fetcher) + * @param entryFromFetcher The entry containing new information (source, from the fetcher) * @param undoManager Compound edit to collect undo information */ - public static void mergeEntries(BibEntry entryFromLibrary, BibEntry source, NamedCompound undoManager) { + public static void mergeEntries(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { LOGGER.debug("Entry from library: {}", entryFromLibrary); - LOGGER.debug("Source entry: {}", source); + LOGGER.debug("Entry from fetcher: {}", entryFromFetcher); - mergeEntryType(source, entryFromLibrary, undoManager); - mergeFields(source, entryFromLibrary, undoManager); - removeObsoleteFields(source, entryFromLibrary, undoManager); + mergeEntryType(entryFromLibrary, entryFromFetcher, undoManager); + mergeFields(entryFromLibrary, entryFromFetcher, undoManager); + removeFieldsNotPresentInFetcher(entryFromLibrary, entryFromFetcher, undoManager); } - private static void mergeEntryType(BibEntry source, BibEntry entryFromLibrary, NamedCompound undoManager) { + private static void mergeEntryType(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { EntryType libraryType = entryFromLibrary.getType(); - EntryType sourceType = source.getType(); + EntryType fetcherType = entryFromFetcher.getType(); - if (!libraryType.equals(sourceType)) { - LOGGER.debug("Updating type {} -> {}", libraryType, sourceType); - entryFromLibrary.setType(sourceType); - undoManager.addEdit(new UndoableChangeType(entryFromLibrary, libraryType, sourceType)); + if (!libraryType.equals(fetcherType)) { + LOGGER.debug("Updating type {} -> {}", libraryType, fetcherType); + entryFromLibrary.setType(fetcherType); + undoManager.addEdit(new UndoableChangeType(entryFromLibrary, libraryType, fetcherType)); } } - private static void mergeFields(BibEntry source, BibEntry entryFromLibrary, NamedCompound undoManager) { + private static void mergeFields(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { Set allFields = new LinkedHashSet<>(); - allFields.addAll(source.getFields()); + allFields.addAll(entryFromFetcher.getFields()); allFields.addAll(entryFromLibrary.getFields()); for (Field field : allFields) { - String sourceValue = source.getField(field).orElse(null); - if (sourceValue == null) { - continue; - } - - String libraryValue = entryFromLibrary.getField(field).orElse(null); - if (shouldUpdateField(libraryValue, sourceValue)) { - LOGGER.debug("Updating field {}: {} -> {}", field, libraryValue, sourceValue); - entryFromLibrary.setField(field, sourceValue); - undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue, sourceValue)); - } + Optional fetcherValue = entryFromFetcher.getField(field); + Optional libraryValue = entryFromLibrary.getField(field); + + fetcherValue.ifPresent(newValue -> { + if (shouldUpdateField(libraryValue, newValue)) { + LOGGER.debug("Updating field {}: {} -> {}", field, libraryValue.orElse(null), newValue); + entryFromLibrary.setField(field, newValue); + undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue.orElse(null), newValue)); + } + }); } } - private static boolean shouldUpdateField(String libraryValue, String sourceValue) { - if (libraryValue == null) { - return true; - } - return sourceValue.length() > libraryValue.length(); + private static boolean shouldUpdateField(Optional libraryValue, String fetcherValue) { + return libraryValue.map(value -> fetcherValue.length() > value.length()) + .orElse(true); } - private static void removeObsoleteFields(BibEntry source, BibEntry entryFromLibrary, NamedCompound undoManager) { + /** + * Removes fields from the library entry that are not present in the fetcher entry. + * This ensures the merged entry only contains fields that are considered current + * according to the fetched data. + */ + private static void removeFieldsNotPresentInFetcher(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { Set obsoleteFields = new LinkedHashSet<>(entryFromLibrary.getFields()); - obsoleteFields.removeAll(source.getFields()); + obsoleteFields.removeAll(entryFromFetcher.getFields()); for (Field field : obsoleteFields) { if (FieldFactory.isInternalField(field)) { continue; } - String value = entryFromLibrary.getField(field).orElse(null); - LOGGER.debug("Removing obsolete field {} with value {}", field, value); - entryFromLibrary.clearField(field); - undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, value, null)); + entryFromLibrary.getField(field).ifPresent(value -> { + LOGGER.debug("Removing obsolete field {} with value {}", field, value); + entryFromLibrary.clearField(field); + undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, value, null)); + }); } } } diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index f9d5dd15b02..1d700c10ba3 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -4,6 +4,7 @@ import java.util.List; import java.util.Optional; +import org.jabref.logic.importer.FetcherException; import org.jabref.logic.importer.IdBasedFetcher; import org.jabref.logic.importer.ImportFormatPreferences; import org.jabref.logic.importer.fetcher.isbntobibtex.IsbnFetcher; @@ -15,10 +16,17 @@ import org.slf4j.LoggerFactory; /** - * This class fetches bibliographic information from external sources based on the identifiers - * (e.g. DOI, ISBN, arXiv ID) of a {@link BibEntry} and merges the fetched information into the entry. + * Fetches and merges bibliographic information from external sources into existing BibEntry objects. + * Supports multiple identifier types (DOI, ISBN, Eprint) and attempts fetching in a defined order + * until successful. + * The merging only adds new fields from the fetched entry and does not modify existing fields + * in the library entry. */ public class MergingIdBasedFetcher { + + public record FetcherResult(BibEntry entryFromLibrary, BibEntry mergedEntry, boolean hasChanges) { + } + private static final List SUPPORTED_FIELDS = List.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT); @@ -29,53 +37,59 @@ public MergingIdBasedFetcher(ImportFormatPreferences importFormatPreferences) { this.importFormatPreferences = importFormatPreferences; } - public Optional fetchEntry(BibEntry entry) { - LOGGER.debug("Entry {}", entry); + public Optional fetchEntry(BibEntry entryFromLibrary) { + LOGGER.debug("Processing library entry {}", entryFromLibrary); return SUPPORTED_FIELDS.stream() - .map(field -> tryFetch(entry, field)) + .map(field -> tryFetch(entryFromLibrary, field)) .filter(Optional::isPresent) .map(Optional::get) .findFirst(); } - private Optional tryFetch(BibEntry entry, Field field) { - return entry.getField(field) - .flatMap(identifier -> getFetcherForField(field) - .flatMap(fetcher -> fetchEntry(fetcher, field, identifier, entry))); + private Optional tryFetch(BibEntry entryFromLibrary, Field field) { + return entryFromLibrary.getField(field) + .flatMap(identifier -> getFetcherForField(field) + .flatMap(fetcher -> performFetch(fetcher, field, identifier, entryFromLibrary))); } - private Optional fetchEntry(IdBasedFetcher fetcher, Field field, - String identifier, BibEntry entryFromLibrary) { + private Optional performFetch(IdBasedFetcher fetcher, Field field, + String identifier, BibEntry entryFromLibrary) { try { - LOGGER.debug("Entry {}", - entryFromLibrary); - - return fetcher.performSearchById(identifier) - .map(fetchedEntry -> { - BibEntry mergedEntry = (BibEntry) entryFromLibrary.clone(); - boolean hasChanges = mergeBibEntry(fetchedEntry, mergedEntry); - return new FetcherResult(entryFromLibrary, mergedEntry, hasChanges); - }); + return attemptFetch(fetcher, identifier, entryFromLibrary); } catch (Exception exception) { - LOGGER.error("Error fetching entry with {} identifier: {}", - field, identifier, exception); + LOGGER.error("Error fetching entry with {} identifier: {}", field, identifier, exception); return Optional.empty(); } } - private boolean mergeBibEntry(BibEntry source, BibEntry target) { - boolean hasChanges = false; - for (Field field : source.getFields()) { - Optional sourceValue = source.getField(field); - Optional targetValue = target.getField(field); + private Optional attemptFetch(IdBasedFetcher fetcher, String identifier, + BibEntry entryFromLibrary) throws FetcherException { + return fetcher.performSearchById(identifier) + .map(entryFromFetcher -> { + BibEntry mergedEntry = (BibEntry) entryFromLibrary.clone(); + boolean hasChanges = mergeBibEntry(entryFromFetcher, mergedEntry); + return new FetcherResult(entryFromLibrary, mergedEntry, hasChanges); + }); + } - if (sourceValue.isPresent() && targetValue.isEmpty()) { - target.setField(field, sourceValue.get()); - hasChanges = true; - } - } - return hasChanges; + private boolean mergeBibEntry(BibEntry entryFromFetcher, BibEntry entryFromLibrary) { + return entryFromFetcher.getFields().stream() + .filter(field -> isNewFieldFromFetcher(entryFromFetcher, entryFromLibrary, field)) + .map(field -> { + entryFromFetcher.getField(field) + .ifPresent(value -> entryFromLibrary.setField(field, value)); + return true; + }) + .findAny() + .orElse(false); + } + + private boolean isNewFieldFromFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, Field field) { + Optional fetcherValue = entryFromFetcher.getField(field); + Optional libraryValue = entryFromLibrary.getField(field); + + return fetcherValue.isPresent() && libraryValue.isEmpty(); } private Optional getFetcherForField(Field field) { @@ -86,7 +100,4 @@ private Optional getFetcherForField(Field field) { default -> Optional.empty(); }; } - - public record FetcherResult(BibEntry entryFromLibrary, BibEntry mergedEntry, boolean hasChanges) { - } } diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 6d0dbfcf546..1198a11d7c5 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1680,11 +1680,12 @@ Entry\ from\ %0=Entry from %0 Merge\ entry\ with\ %0\ information=Merge entry with %0 information Get\ bibliographic\ data\ from\ %0\ (fully\ automated)=Get bibliographic data from %0 (fully automated) Batch\ update\ successful.\ %0\ entries\ updated.=Batch update successful. %0 entries updated. -Fetching\ entry\ %0\ of\ %1=Fetching entry %0 of %1 Error\ while\ fetching\ and\ merging\ entries\:\ %0=Error while fetching and merging entries: %0 +Processing\ entry\ %0\ of\ %1=Processing entry %0 of %1 Fetching\ and\ merging\ entries=Fetching and merging entries -No\ entries\ exist.=No entries exist. No\ updates\ found.=No updates found. +Fetching\ information\ using\ %0=Fetching information using %0 +No\ information\ added=No information added Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 Add\ new\ list=Add new list Open\ existing\ list=Open existing list From 95e9ae85c52e9c9724cfda7d265f0da6ef8ae82e Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 27 Oct 2024 13:44:17 +1100 Subject: [PATCH 19/28] Fixed the minor import format issue. --- src/main/java/org/jabref/gui/frame/MainMenu.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java index cb31d98b863..4944aadc321 100644 --- a/src/main/java/org/jabref/gui/frame/MainMenu.java +++ b/src/main/java/org/jabref/gui/frame/MainMenu.java @@ -51,8 +51,8 @@ import org.jabref.gui.linkedfile.RedownloadMissingFilesAction; import org.jabref.gui.maintable.NewLibraryFromPdfActionOffline; import org.jabref.gui.maintable.NewLibraryFromPdfActionOnline; -import org.jabref.gui.mergeentries.MergeEntriesAction; import org.jabref.gui.mergeentries.BatchEntryMergeWithFetchedDataAction; +import org.jabref.gui.mergeentries.MergeEntriesAction; import org.jabref.gui.plaincitationparser.PlainCitationParserAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preferences.ShowPreferencesAction; From 7305f3c1456d6896fb98594fb0bb7e1a7ec60d7d Mon Sep 17 00:00:00 2001 From: KUMOAWAI <128650848+KUMOAWAI@users.noreply.github.com> Date: Sun, 27 Oct 2024 14:55:33 +1100 Subject: [PATCH 20/28] Apply suggestions from code review Modify the CHANGELOG.md as suggested Co-authored-by: Loay Ghreeb --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f30d016eb1d..163a7c9ba80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) -- Added batch fetching of bibliographic data for multiple entries in the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) +- Added batch fetching of bibliographic data for multiple entries in the "Lookup" menu. [koppor#372](https://github.com/koppor/jabref/issues/372) - We use the menu icon for background tasks as a progress indicator to visualise an import's progress when dragging and dropping several PDF files into the main table. [#12072](https://github.com/JabRef/jabref/pull/12072) ### Changed From 9dce81a524e86fa7cafe7c5b1184fb1e65644555 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 27 Oct 2024 22:01:35 +1100 Subject: [PATCH 21/28] Refactor: Reorder parameters to (fetcher, library) in merge operations - Reorder parameters to follow source->target convention in BatchEntryMergeWithFetchedDataAction and MergeEntriesHelper - Consistently use entryFromFetcher and entryFromLibrary as parameter names --- .../BatchEntryMergeWithFetchedDataAction.java | 25 +++++++++---------- .../gui/mergeentries/MergeEntriesHelper.java | 24 +++++++++--------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java index b02b74fbec9..ab52a819528 100644 --- a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java @@ -1,4 +1,3 @@ - package org.jabref.gui.mergeentries; import java.util.ArrayList; @@ -58,15 +57,15 @@ public void execute() { return; } - List entries = libraryTab.getDatabase().getEntries(); - if (entries.isEmpty()) { + List libraryEntries = libraryTab.getDatabase().getEntries(); + if (libraryEntries.isEmpty()) { notificationService.notify(Localization.lang("empty library")); return; } MergeContext context = new MergeContext( libraryTab, - entries, + libraryEntries, new MergingIdBasedFetcher(preferences.getImportFormatPreferences()), notificationService ); @@ -77,7 +76,7 @@ public void execute() { private static BackgroundTask> createBackgroundTask(MergeContext context) { BackgroundTask> task = new BackgroundTask<>() { @Override - public List call() { + public List call() { return processMergeEntries(context, this); } }; @@ -89,7 +88,7 @@ public List call() { handleSuccess(updatedEntries, context)); task.onFailure(ex -> - handleFailure(ex, context.notificationService)); + handleFailure(ex, context.notificationService())); return task; } @@ -98,23 +97,23 @@ private static List processMergeEntries(MergeContext context, Background int totalEntries = context.entries().size(); for (int i = 0; i < totalEntries && !task.isCancelled(); i++) { - BibEntry entry = context.entries().get(i); + BibEntry libraryEntry = context.entries().get(i); updateProgress(i, totalEntries, task); - fetchAndMergeEntry(context, entry); + fetchAndMergeEntry(context, libraryEntry); } finalizeCompoundEdit(context); return context.updatedEntries(); } - private static void fetchAndMergeEntry(MergeContext context, BibEntry entry) { - LOGGER.debug("Processing entry: {}", entry); + private static void fetchAndMergeEntry(MergeContext context, BibEntry libraryEntry) { + LOGGER.debug("Processing library entry: {}", libraryEntry); - context.fetcher().fetchEntry(entry) + context.fetcher().fetchEntry(libraryEntry) .filter(MergingIdBasedFetcher.FetcherResult::hasChanges) .ifPresent(result -> Platform.runLater(() -> { - MergeEntriesHelper.mergeEntries(entry, result.mergedEntry(), context.compoundEdit()); - entry.getCitationKey().ifPresent(context.updatedEntries()::add); + MergeEntriesHelper.mergeEntries(result.mergedEntry(), libraryEntry, context.compoundEdit()); + libraryEntry.getCitationKey().ifPresent(context.updatedEntries()::add); })); } diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java index 8b565d4c8a0..4f833c8eb43 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -31,22 +31,22 @@ private MergeEntriesHelper() { /** * Merges two BibEntry objects with undo support. * - * @param entryFromLibrary The entry to be updated (target, from the library) * @param entryFromFetcher The entry containing new information (source, from the fetcher) + * @param entryFromLibrary The entry to be updated (target, from the library) * @param undoManager Compound edit to collect undo information */ - public static void mergeEntries(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { - LOGGER.debug("Entry from library: {}", entryFromLibrary); + public static void mergeEntries(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { LOGGER.debug("Entry from fetcher: {}", entryFromFetcher); + LOGGER.debug("Entry from library: {}", entryFromLibrary); - mergeEntryType(entryFromLibrary, entryFromFetcher, undoManager); - mergeFields(entryFromLibrary, entryFromFetcher, undoManager); - removeFieldsNotPresentInFetcher(entryFromLibrary, entryFromFetcher, undoManager); + mergeEntryType(entryFromFetcher, entryFromLibrary, undoManager); + mergeFields(entryFromFetcher, entryFromLibrary, undoManager); + removeFieldsNotPresentInFetcher(entryFromFetcher, entryFromLibrary, undoManager); } - private static void mergeEntryType(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { - EntryType libraryType = entryFromLibrary.getType(); + private static void mergeEntryType(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { EntryType fetcherType = entryFromFetcher.getType(); + EntryType libraryType = entryFromLibrary.getType(); if (!libraryType.equals(fetcherType)) { LOGGER.debug("Updating type {} -> {}", libraryType, fetcherType); @@ -55,7 +55,7 @@ private static void mergeEntryType(BibEntry entryFromLibrary, BibEntry entryFrom } } - private static void mergeFields(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { + private static void mergeFields(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { Set allFields = new LinkedHashSet<>(); allFields.addAll(entryFromFetcher.getFields()); allFields.addAll(entryFromLibrary.getFields()); @@ -65,7 +65,7 @@ private static void mergeFields(BibEntry entryFromLibrary, BibEntry entryFromFet Optional libraryValue = entryFromLibrary.getField(field); fetcherValue.ifPresent(newValue -> { - if (shouldUpdateField(libraryValue, newValue)) { + if (shouldUpdateField(newValue, libraryValue)) { LOGGER.debug("Updating field {}: {} -> {}", field, libraryValue.orElse(null), newValue); entryFromLibrary.setField(field, newValue); undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue.orElse(null), newValue)); @@ -74,7 +74,7 @@ private static void mergeFields(BibEntry entryFromLibrary, BibEntry entryFromFet } } - private static boolean shouldUpdateField(Optional libraryValue, String fetcherValue) { + private static boolean shouldUpdateField(String fetcherValue, Optional libraryValue) { return libraryValue.map(value -> fetcherValue.length() > value.length()) .orElse(true); } @@ -84,7 +84,7 @@ private static boolean shouldUpdateField(Optional libraryValue, String f * This ensures the merged entry only contains fields that are considered current * according to the fetched data. */ - private static void removeFieldsNotPresentInFetcher(BibEntry entryFromLibrary, BibEntry entryFromFetcher, NamedCompound undoManager) { + private static void removeFieldsNotPresentInFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { Set obsoleteFields = new LinkedHashSet<>(entryFromLibrary.getFields()); obsoleteFields.removeAll(entryFromFetcher.getFields()); From 61f5a8f4a15eb875fdb38d66c96bcd30ac9481eb Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Thu, 31 Oct 2024 15:45:29 +1100 Subject: [PATCH 22/28] Refactor: Deleted the empty line before package --- .../java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java | 1 - .../java/org/jabref/gui/mergeentries/MergeEntriesHelper.java | 1 - .../org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java | 1 - 3 files changed, 3 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index 869a3a7e83b..c14e0770c15 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -1,4 +1,3 @@ - package org.jabref.gui.mergeentries; import java.util.Arrays; diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java index 4f833c8eb43..f2c64cefa14 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -1,4 +1,3 @@ - package org.jabref.gui.mergeentries; import java.util.LinkedHashSet; diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index 1d700c10ba3..0872fcef53e 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -1,4 +1,3 @@ - package org.jabref.logic.importer.fetcher; import java.util.List; From 74681e1a77a18fc49b00b78b57909d2c7ec22da6 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 16 Nov 2024 18:47:58 +1100 Subject: [PATCH 23/28] Refactor: Deleted the empty line before package --- .../org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java b/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java index 3e9a4d35e90..743b0aac900 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeWithFetchedEntryAction.java @@ -1,4 +1,3 @@ - package org.jabref.gui.mergeentries; import javax.swing.undo.UndoManager; From 3197ef49564fdd53d30ff0706bfb13840d2adb6e Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 16 Nov 2024 19:23:12 +1100 Subject: [PATCH 24/28] refactor: Extract background processing into BatchEntryMergeTask, optimized MergingIdBasedFetcher - Moved the original single entry update feature button to the lookup menu. MergingIdBasedFetcher: - Deleted redundant getFetcherForField() method - Add StringNormalizer to standardize field value comparisons - Update existing fields when their normalized values differ - Track and report updated fields in FetcherResult - Improve debug logging for merge operations BatchEntryMergeTask (New): - Extract background processing logic from BatchEntryMergeWithFetchedDataAction - Introduce MergeContext record for operation parameters - Improve error handling and progress reporting BatchEntryMergeWithFetchedDataAction: - Split into multiple classes for better separation of concerns - Move single entry update feature button to lookup menu - Simplify to focus on operation orchestration MergeEntriesHelper: - Made methods return boolean to avoid redundant merge actions --- .../java/org/jabref/gui/frame/MainMenu.java | 16 +- .../jabref/gui/maintable/RightClickMenu.java | 9 +- .../gui/mergeentries/BatchEntryMergeTask.java | 156 +++++++++++++++++ .../BatchEntryMergeWithFetchedDataAction.java | 157 +++--------------- .../gui/mergeentries/MergeEntriesHelper.java | 62 +++---- .../fetcher/MergingIdBasedFetcher.java | 151 +++++++++++------ 6 files changed, 324 insertions(+), 227 deletions(-) create mode 100644 src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java index 4944aadc321..26dc4fa2cb8 100644 --- a/src/main/java/org/jabref/gui/frame/MainMenu.java +++ b/src/main/java/org/jabref/gui/frame/MainMenu.java @@ -53,6 +53,7 @@ import org.jabref.gui.maintable.NewLibraryFromPdfActionOnline; import org.jabref.gui.mergeentries.BatchEntryMergeWithFetchedDataAction; import org.jabref.gui.mergeentries.MergeEntriesAction; +import org.jabref.gui.mergeentries.MergeWithFetchedEntryAction; import org.jabref.gui.plaincitationparser.PlainCitationParserAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preferences.ShowPreferencesAction; @@ -278,16 +279,15 @@ private void createMenu() { new SeparatorMenuItem(), + factory.createMenuItem( + StandardActions.MERGE_WITH_FETCHED_ENTRY, + new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)), + + new SeparatorMenuItem(), + factory.createMenuItem( StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRY, - new BatchEntryMergeWithFetchedDataAction( - frame::getCurrentLibraryTab, - preferences, - dialogService, - stateManager, - taskExecutor - ) - ) + new BatchEntryMergeWithFetchedDataAction(stateManager, undoManager, preferences, dialogService, taskExecutor)) ); final MenuItem pushToApplicationMenuItem = factory.createMenuItem(pushToApplicationCommand.getAction(), pushToApplicationCommand); diff --git a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java index 66430e8770e..dbbfc79f8af 100644 --- a/src/main/java/org/jabref/gui/maintable/RightClickMenu.java +++ b/src/main/java/org/jabref/gui/maintable/RightClickMenu.java @@ -21,9 +21,7 @@ import org.jabref.gui.keyboard.KeyBindingRepository; import org.jabref.gui.linkedfile.AttachFileAction; import org.jabref.gui.linkedfile.AttachFileFromURLAction; -import org.jabref.gui.menus.ChangeEntryTypeMenu; import org.jabref.gui.mergeentries.MergeEntriesAction; -import org.jabref.gui.mergeentries.MergeWithFetchedEntryAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.preview.CopyCitationAction; import org.jabref.gui.preview.PreviewPreferences; @@ -87,12 +85,7 @@ public static ContextMenu create(BibEntryTableViewModel entry, extractFileReferencesOffline, factory.createMenuItem(StandardActions.OPEN_URL, new OpenUrlAction(dialogService, stateManager, preferences)), - factory.createMenuItem(StandardActions.SEARCH_SHORTSCIENCE, new SearchShortScienceAction(dialogService, stateManager, preferences)), - - new SeparatorMenuItem(), - - new ChangeEntryTypeMenu(libraryTab.getSelectedEntries(), libraryTab.getBibDatabaseContext(), undoManager, entryTypesManager).asSubMenu(), - factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)) + factory.createMenuItem(StandardActions.SEARCH_SHORTSCIENCE, new SearchShortScienceAction(dialogService, stateManager, preferences)) ); EasyBind.subscribe(preferences.getGrobidPreferences().grobidEnabledProperty(), enabled -> { diff --git a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java new file mode 100644 index 00000000000..8713ba5e739 --- /dev/null +++ b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java @@ -0,0 +1,156 @@ +package org.jabref.gui.mergeentries; + +import java.util.List; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; + +import javax.swing.undo.UndoManager; + +import org.jabref.gui.undo.NamedCompound; +import org.jabref.logic.importer.fetcher.MergingIdBasedFetcher; +import org.jabref.logic.l10n.Localization; +import org.jabref.logic.util.BackgroundTask; +import org.jabref.logic.util.NotificationService; +import org.jabref.model.entry.BibEntry; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A background task that handles fetching and merging of bibliography entries. + * This implementation provides improved concurrency handling, better Optional usage, + * and more robust error handling. + */ +public class BatchEntryMergeTask extends BackgroundTask> { + + private static final Logger LOGGER = LoggerFactory.getLogger(BatchEntryMergeTask.class); + private final MergeContext context; + private final NamedCompound compoundEdit; + private final AtomicInteger processedEntries; + private final AtomicInteger successfulUpdates; + + public BatchEntryMergeTask(MergeContext context) { + this.context = context; + this.compoundEdit = new NamedCompound(Localization.lang("Merge entries")); + this.processedEntries = new AtomicInteger(0); + this.successfulUpdates = new AtomicInteger(0); + + configureTask(); + } + + private void configureTask() { + setTitle(Localization.lang("Fetching and merging entries")); + withInitialMessage(Localization.lang("Starting merge operation...")); + showToUser(true); + } + + @Override + public List call() throws Exception { + try { + List updatedEntries = processMergeEntries(); + finalizeOperation(updatedEntries); + LOGGER.debug("Merge operation completed. Processed: {}, Successfully updated: {}", + processedEntries.get(), successfulUpdates.get()); + notifySuccess(successfulUpdates.get()); + return updatedEntries; + } catch (Exception e) { + LOGGER.error("Critical error during merge operation", e); + notifyError(e); + throw e; + } + } + + private List processMergeEntries() { + return context.entries().stream() + .takeWhile(_ -> !isCancelled()) + .map(this::processSingleEntryWithProgress) + .filter(Optional::isPresent) + .map(Optional::get) + .collect(Collectors.toList()); + } + + private Optional processSingleEntryWithProgress(BibEntry entry) { + updateProgress(processedEntries.incrementAndGet(), context.entries().size()); + updateMessage(Localization.lang("Processing entry %0 of %1", + processedEntries.get(), + context.entries().size())); + return processSingleEntry(entry); + } + + private Optional processSingleEntry(BibEntry entry) { + try { + LOGGER.debug("Processing entry: {}", entry); + return context.fetcher().fetchEntry(entry) + .filter(MergingIdBasedFetcher.FetcherResult::hasChanges) + .flatMap(result -> { + boolean changesApplied = applyMerge(entry, result); + if (changesApplied) { + successfulUpdates.incrementAndGet(); + return entry.getCitationKey(); + } + return Optional.empty(); + }); + } catch (Exception e) { + handleEntryProcessingError(entry, e); + return Optional.empty(); + } + } + + private boolean applyMerge(BibEntry entry, MergingIdBasedFetcher.FetcherResult result) { + synchronized (compoundEdit) { + try { + return MergeEntriesHelper.mergeEntries(result.mergedEntry(), entry, compoundEdit); + } catch (Exception e) { + LOGGER.error("Error during merge operation for entry: {}", entry, e); + return false; + } + } + } + + private void handleEntryProcessingError(BibEntry entry, Exception e) { + String citationKey = entry.getCitationKey().orElse("unknown"); + String errorMessage = String.format("Error processing entry %s: %s", + citationKey, + e.getMessage()); + + LOGGER.error(errorMessage, e); + context.notificationService().notify(Localization.lang(errorMessage)); + } + + private void finalizeOperation(List updatedEntries) { + if (!updatedEntries.isEmpty()) { + synchronized (compoundEdit) { + compoundEdit.end(); + context.undoManager.addEdit(compoundEdit); + } + } + } + + private void notifySuccess(int updateCount) { + String message = updateCount == 0 + ? Localization.lang("No updates found.") + : Localization.lang("Batch update successful. %0 entries updated.", updateCount); + context.notificationService().notify(message); + } + + private void notifyError(Exception e) { + context.notificationService().notify( + Localization.lang("Merge operation failed: %0", e.getMessage())); + } + + /** + * Record containing all the context needed for the merge operation. + * Implements defensive copying to ensure immutability. + */ + public record MergeContext( + List entries, + MergingIdBasedFetcher fetcher, + UndoManager undoManager, + NotificationService notificationService + ) { + public MergeContext { + entries = List.copyOf(entries); // Defensive copy + } + } +} diff --git a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java index ab52a819528..1ddbf5e0048 100644 --- a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java @@ -1,176 +1,67 @@ package org.jabref.gui.mergeentries; -import java.util.ArrayList; -import java.util.Collections; import java.util.List; -import java.util.function.Supplier; -import javafx.application.Platform; +import javax.swing.undo.UndoManager; -import org.jabref.gui.LibraryTab; import org.jabref.gui.StateManager; import org.jabref.gui.actions.ActionHelper; import org.jabref.gui.actions.SimpleCommand; import org.jabref.gui.preferences.GuiPreferences; -import org.jabref.gui.undo.NamedCompound; import org.jabref.logic.importer.fetcher.MergingIdBasedFetcher; import org.jabref.logic.l10n.Localization; -import org.jabref.logic.util.BackgroundTask; import org.jabref.logic.util.NotificationService; import org.jabref.logic.util.TaskExecutor; +import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - /** - * Handles the merging of multiple bibliography entries with fetched data. - * This action performs background fetching and merging of entries while - * providing progress updates to the user. + * Handles batch merging of bibliography entries with fetched data. */ public class BatchEntryMergeWithFetchedDataAction extends SimpleCommand { - private static final Logger LOGGER = LoggerFactory.getLogger(BatchEntryMergeWithFetchedDataAction.class); - - private final Supplier tabSupplier; + private final StateManager stateManager; + private final UndoManager undoManager; private final GuiPreferences preferences; private final NotificationService notificationService; private final TaskExecutor taskExecutor; - public BatchEntryMergeWithFetchedDataAction(Supplier tabSupplier, + public BatchEntryMergeWithFetchedDataAction(StateManager stateManager, + UndoManager undoManager, GuiPreferences preferences, NotificationService notificationService, - StateManager stateManager, TaskExecutor taskExecutor) { - this.tabSupplier = tabSupplier; + this.stateManager = stateManager; + this.undoManager = undoManager; this.preferences = preferences; this.notificationService = notificationService; this.taskExecutor = taskExecutor; + this.executable.bind(ActionHelper.needsDatabase(stateManager)); } @Override public void execute() { - LibraryTab libraryTab = tabSupplier.get(); - if (libraryTab == null) { - LOGGER.error("Cannot perform batch merge: no library is open."); - return; - } - - List libraryEntries = libraryTab.getDatabase().getEntries(); - if (libraryEntries.isEmpty()) { - notificationService.notify(Localization.lang("empty library")); + if (stateManager.getActiveDatabase().isEmpty()) { return; } - MergeContext context = new MergeContext( - libraryTab, - libraryEntries, - new MergingIdBasedFetcher(preferences.getImportFormatPreferences()), - notificationService - ); - - taskExecutor.execute(createBackgroundTask(context)); - } - - private static BackgroundTask> createBackgroundTask(MergeContext context) { - BackgroundTask> task = new BackgroundTask<>() { - @Override - public List call() { - return processMergeEntries(context, this); - } - }; - - task.setTitle(Localization.lang("Fetching and merging entries")); - task.showToUser(true); - - task.onSuccess(updatedEntries -> - handleSuccess(updatedEntries, context)); - - task.onFailure(ex -> - handleFailure(ex, context.notificationService())); + List entries = stateManager.getActiveDatabase() + .map(BibDatabaseContext::getEntries) + .orElse(List.of()); - return task; - } - - private static List processMergeEntries(MergeContext context, BackgroundTask task) { - int totalEntries = context.entries().size(); - - for (int i = 0; i < totalEntries && !task.isCancelled(); i++) { - BibEntry libraryEntry = context.entries().get(i); - updateProgress(i, totalEntries, task); - fetchAndMergeEntry(context, libraryEntry); - } - - finalizeCompoundEdit(context); - return context.updatedEntries(); - } - - private static void fetchAndMergeEntry(MergeContext context, BibEntry libraryEntry) { - LOGGER.debug("Processing library entry: {}", libraryEntry); - - context.fetcher().fetchEntry(libraryEntry) - .filter(MergingIdBasedFetcher.FetcherResult::hasChanges) - .ifPresent(result -> Platform.runLater(() -> { - MergeEntriesHelper.mergeEntries(result.mergedEntry(), libraryEntry, context.compoundEdit()); - libraryEntry.getCitationKey().ifPresent(context.updatedEntries()::add); - })); - } - - private static void finalizeCompoundEdit(MergeContext context) { - if (!context.updatedEntries().isEmpty()) { - Platform.runLater(() -> { - context.compoundEdit().end(); - context.libraryTab().getUndoManager().addEdit(context.compoundEdit()); - }); + if (entries.isEmpty()) { + notificationService.notify(Localization.lang("No entries available for merging")); + return; } - } - private static void updateProgress(int currentIndex, int totalEntries, BackgroundTask task) { - Platform.runLater(() -> { - task.updateMessage(Localization.lang("Processing entry %0 of %1", currentIndex + 1, totalEntries)); - task.updateProgress(currentIndex, totalEntries); - }); - } - - private static void handleSuccess(List updatedEntries, MergeContext context) { - Platform.runLater(() -> { - String message = updatedEntries.isEmpty() - ? Localization.lang("No updates found.") - : Localization.lang("Batch update successful. %0 entries updated.", - updatedEntries.size()); - - LOGGER.debug("Batch update completed. {}", message); - context.notificationService().notify(message); - }); - } + MergingIdBasedFetcher fetcher = new MergingIdBasedFetcher(preferences.getImportFormatPreferences()); + BatchEntryMergeTask mergeTask = new BatchEntryMergeTask( + new BatchEntryMergeTask.MergeContext(entries, + fetcher, + undoManager, + notificationService)); - private static void handleFailure(Exception ex, NotificationService notificationService) { - LOGGER.error("Error during fetch and merge", ex); - Platform.runLater(() -> - notificationService.notify( - Localization.lang("Error while fetching and merging entries: %0", ex.getMessage()) - ) - ); - } - - private record MergeContext( - LibraryTab libraryTab, - List entries, - MergingIdBasedFetcher fetcher, - NamedCompound compoundEdit, - List updatedEntries, - NotificationService notificationService - ) { - MergeContext(LibraryTab libraryTab, List entries, MergingIdBasedFetcher fetcher, NotificationService notificationService) { - this( - libraryTab, - entries, - fetcher, - new NamedCompound(Localization.lang("Merge entries")), - Collections.synchronizedList(new ArrayList<>()), - notificationService - ); - } + mergeTask.executeWith(taskExecutor); } } diff --git a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java index f2c64cefa14..990429fd69e 100644 --- a/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java +++ b/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java @@ -34,16 +34,18 @@ private MergeEntriesHelper() { * @param entryFromLibrary The entry to be updated (target, from the library) * @param undoManager Compound edit to collect undo information */ - public static void mergeEntries(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { + public static boolean mergeEntries(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { LOGGER.debug("Entry from fetcher: {}", entryFromFetcher); LOGGER.debug("Entry from library: {}", entryFromLibrary); - mergeEntryType(entryFromFetcher, entryFromLibrary, undoManager); - mergeFields(entryFromFetcher, entryFromLibrary, undoManager); - removeFieldsNotPresentInFetcher(entryFromFetcher, entryFromLibrary, undoManager); + boolean typeChanged = mergeEntryType(entryFromFetcher, entryFromLibrary, undoManager); + boolean fieldsChanged = mergeFields(entryFromFetcher, entryFromLibrary, undoManager); + boolean fieldsRemoved = removeFieldsNotPresentInFetcher(entryFromFetcher, entryFromLibrary, undoManager); + + return typeChanged || fieldsChanged || fieldsRemoved; } - private static void mergeEntryType(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { + private static boolean mergeEntryType(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { EntryType fetcherType = entryFromFetcher.getType(); EntryType libraryType = entryFromLibrary.getType(); @@ -51,52 +53,56 @@ private static void mergeEntryType(BibEntry entryFromFetcher, BibEntry entryFrom LOGGER.debug("Updating type {} -> {}", libraryType, fetcherType); entryFromLibrary.setType(fetcherType); undoManager.addEdit(new UndoableChangeType(entryFromLibrary, libraryType, fetcherType)); + return true; } + return false; } - private static void mergeFields(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { + private static boolean mergeFields(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { Set allFields = new LinkedHashSet<>(); allFields.addAll(entryFromFetcher.getFields()); allFields.addAll(entryFromLibrary.getFields()); + boolean anyFieldsChanged = false; + for (Field field : allFields) { Optional fetcherValue = entryFromFetcher.getField(field); Optional libraryValue = entryFromLibrary.getField(field); - fetcherValue.ifPresent(newValue -> { - if (shouldUpdateField(newValue, libraryValue)) { - LOGGER.debug("Updating field {}: {} -> {}", field, libraryValue.orElse(null), newValue); - entryFromLibrary.setField(field, newValue); - undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue.orElse(null), newValue)); - } - }); + if (fetcherValue.isPresent() && shouldUpdateField(fetcherValue.get(), libraryValue)) { + LOGGER.debug("Updating field {}: {} -> {}", field, libraryValue.orElse(null), fetcherValue.get()); + entryFromLibrary.setField(field, fetcherValue.get()); + undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue.orElse(null), fetcherValue.get())); + anyFieldsChanged = true; + } } + return anyFieldsChanged; } - private static boolean shouldUpdateField(String fetcherValue, Optional libraryValue) { - return libraryValue.map(value -> fetcherValue.length() > value.length()) - .orElse(true); - } - - /** - * Removes fields from the library entry that are not present in the fetcher entry. - * This ensures the merged entry only contains fields that are considered current - * according to the fetched data. - */ - private static void removeFieldsNotPresentInFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { + private static boolean removeFieldsNotPresentInFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) { Set obsoleteFields = new LinkedHashSet<>(entryFromLibrary.getFields()); obsoleteFields.removeAll(entryFromFetcher.getFields()); + boolean anyFieldsRemoved = false; + for (Field field : obsoleteFields) { if (FieldFactory.isInternalField(field)) { continue; } - entryFromLibrary.getField(field).ifPresent(value -> { - LOGGER.debug("Removing obsolete field {} with value {}", field, value); + Optional value = entryFromLibrary.getField(field); + if (value.isPresent()) { + LOGGER.debug("Removing obsolete field {} with value {}", field, value.get()); entryFromLibrary.clearField(field); - undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, value, null)); - }); + undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, value.get(), null)); + anyFieldsRemoved = true; + } } + return anyFieldsRemoved; + } + + private static boolean shouldUpdateField(String fetcherValue, Optional libraryValue) { + return libraryValue.map(value -> fetcherValue.length() > value.length()) + .orElse(true); } } diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index 0872fcef53e..522a0547ae1 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -2,17 +2,19 @@ import java.util.List; import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import org.jabref.logic.importer.FetcherException; import org.jabref.logic.importer.IdBasedFetcher; import org.jabref.logic.importer.ImportFormatPreferences; -import org.jabref.logic.importer.fetcher.isbntobibtex.IsbnFetcher; +import org.jabref.logic.importer.WebFetchers; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.Field; import org.jabref.model.entry.field.StandardField; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.mariadb.jdbc.internal.logging.Logger; +import org.mariadb.jdbc.internal.logging.LoggerFactory; /** * Fetches and merges bibliographic information from external sources into existing BibEntry objects. @@ -22,81 +24,130 @@ * in the library entry. */ public class MergingIdBasedFetcher { - - public record FetcherResult(BibEntry entryFromLibrary, BibEntry mergedEntry, boolean hasChanges) { - } - + private static final Logger LOGGER = LoggerFactory.getLogger(MergingIdBasedFetcher.class); private static final List SUPPORTED_FIELDS = List.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT); - - private static final Logger LOGGER = LoggerFactory.getLogger(MergingIdBasedFetcher.class); private final ImportFormatPreferences importFormatPreferences; public MergingIdBasedFetcher(ImportFormatPreferences importFormatPreferences) { this.importFormatPreferences = importFormatPreferences; } + public record FetcherResult( + BibEntry entryFromLibrary, + BibEntry mergedEntry, + boolean hasChanges, + Set updatedFields + ) { + public FetcherResult { + updatedFields = Set.copyOf(updatedFields); + } + } + public Optional fetchEntry(BibEntry entryFromLibrary) { - LOGGER.debug("Processing library entry {}", entryFromLibrary); + if (entryFromLibrary == null) { + return Optional.empty(); + } + + logEntryProcessing(entryFromLibrary); + return findFirstValidFetch(entryFromLibrary); + } + private void logEntryProcessing(BibEntry entry) { + LOGGER.debug("Processing library entry: {}", + entry.getCitationKey().orElse("[no key]")); + + SUPPORTED_FIELDS.forEach(field -> + entry.getField(field).ifPresent(value -> + LOGGER.debug("Entry has {} identifier: {}", field, value))); + } + + private Optional findFirstValidFetch(BibEntry entry) { return SUPPORTED_FIELDS.stream() - .map(field -> tryFetch(entryFromLibrary, field)) - .filter(Optional::isPresent) - .map(Optional::get) + .map(field -> fetchWithField(entry, field)) + .flatMap(Optional::stream) .findFirst(); } - private Optional tryFetch(BibEntry entryFromLibrary, Field field) { - return entryFromLibrary.getField(field) - .flatMap(identifier -> getFetcherForField(field) - .flatMap(fetcher -> performFetch(fetcher, field, identifier, entryFromLibrary))); + private Optional fetchWithField(BibEntry entry, Field field) { + return entry.getField(field) + .flatMap(identifier -> fetchWithIdentifier(field, identifier, entry)); } - private Optional performFetch(IdBasedFetcher fetcher, Field field, + private Optional fetchWithIdentifier(Field field, String identifier, + BibEntry entryFromLibrary) { + return WebFetchers.getIdBasedFetcherForField(field, importFormatPreferences) + .flatMap(fetcher -> executeFetch(fetcher, field, identifier, entryFromLibrary)); + } + + private Optional executeFetch(IdBasedFetcher fetcher, Field field, String identifier, BibEntry entryFromLibrary) { try { - return attemptFetch(fetcher, identifier, entryFromLibrary); - } catch (Exception exception) { - LOGGER.error("Error fetching entry with {} identifier: {}", field, identifier, exception); + LOGGER.debug("Fetching with {}: {}", + fetcher.getClass().getSimpleName(), identifier); + return fetcher.performSearchById(identifier) + .map(fetchedEntry -> mergeBibEntries(entryFromLibrary, fetchedEntry)); + } catch (FetcherException e) { + LOGGER.error("Fetch failed for {} with identifier: {}", + field, identifier, e); return Optional.empty(); } } - private Optional attemptFetch(IdBasedFetcher fetcher, String identifier, - BibEntry entryFromLibrary) throws FetcherException { - return fetcher.performSearchById(identifier) - .map(entryFromFetcher -> { - BibEntry mergedEntry = (BibEntry) entryFromLibrary.clone(); - boolean hasChanges = mergeBibEntry(entryFromFetcher, mergedEntry); - return new FetcherResult(entryFromLibrary, mergedEntry, hasChanges); - }); + private FetcherResult mergeBibEntries(BibEntry entryFromLibrary, + BibEntry fetchedEntry) { + BibEntry mergedEntry = new BibEntry(entryFromLibrary.getType()); + + entryFromLibrary.getFields().forEach(field -> + entryFromLibrary.getField(field) + .ifPresent(value -> mergedEntry.setField(field, value))); + + Set updatedFields = updateFieldsFromSource(fetchedEntry, mergedEntry); + + return new FetcherResult(entryFromLibrary, mergedEntry, + !updatedFields.isEmpty(), updatedFields); } - private boolean mergeBibEntry(BibEntry entryFromFetcher, BibEntry entryFromLibrary) { - return entryFromFetcher.getFields().stream() - .filter(field -> isNewFieldFromFetcher(entryFromFetcher, entryFromLibrary, field)) - .map(field -> { - entryFromFetcher.getField(field) - .ifPresent(value -> entryFromLibrary.setField(field, value)); - return true; - }) - .findAny() - .orElse(false); + private Set updateFieldsFromSource(BibEntry sourceEntry, + BibEntry targetEntry) { + return sourceEntry.getFields().stream() + .filter(field -> shouldUpdateField(field, sourceEntry, targetEntry)) + .peek(field -> updateField(field, sourceEntry, targetEntry)) + .collect(Collectors.toSet()); } - private boolean isNewFieldFromFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, Field field) { - Optional fetcherValue = entryFromFetcher.getField(field); - Optional libraryValue = entryFromLibrary.getField(field); + private boolean shouldUpdateField(Field field, BibEntry sourceEntry, + BibEntry targetEntry) { + String sourceValue = sourceEntry.getField(field) + .map(StringNormalizer::normalize) + .orElse(""); + String targetValue = targetEntry.getField(field) + .map(StringNormalizer::normalize) + .orElse(""); + return !sourceValue.equals(targetValue); + } - return fetcherValue.isPresent() && libraryValue.isEmpty(); + private void updateField(Field field, BibEntry sourceEntry, + BibEntry targetEntry) { + sourceEntry.getField(field).ifPresent(value -> { + targetEntry.setField(field, value); + LOGGER.debug("Updated field {}: '{}'", field, value); + }); } +} - private Optional getFetcherForField(Field field) { - return switch (field) { - case StandardField.DOI -> Optional.of(new DoiFetcher(importFormatPreferences)); - case StandardField.ISBN -> Optional.of(new IsbnFetcher(importFormatPreferences)); - case StandardField.EPRINT -> Optional.of(new IacrEprintFetcher(importFormatPreferences)); - default -> Optional.empty(); - }; +class StringNormalizer { + public static String normalize(String value) { + if (value == null) { + return ""; + } + return value.trim() + .replaceAll("\\s+", " ") + .replaceAll("\\r\\n|\\r|\\n", " ") + .replaceAll("\\s*-+\\s*", "-") + .replaceAll("\\s*,\\s*", ", ") + .replaceAll("\\s*;\\s*", "; ") + .replaceAll("\\s*:\\s*", ": "); } } + From c04a2fafdcf8b6fd0936984efb18f2feacadff83 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sat, 16 Nov 2024 22:33:51 +1100 Subject: [PATCH 25/28] Updated JabRef_en.properties --- .../org/jabref/gui/mergeentries/BatchEntryMergeTask.java | 9 +++------ src/main/resources/l10n/JabRef_en.properties | 5 ++++- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java index 8713ba5e739..9c4d5d7075e 100644 --- a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java +++ b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java @@ -110,12 +110,9 @@ private boolean applyMerge(BibEntry entry, MergingIdBasedFetcher.FetcherResult r private void handleEntryProcessingError(BibEntry entry, Exception e) { String citationKey = entry.getCitationKey().orElse("unknown"); - String errorMessage = String.format("Error processing entry %s: %s", - citationKey, - e.getMessage()); - - LOGGER.error(errorMessage, e); - context.notificationService().notify(Localization.lang(errorMessage)); + String message = Localization.lang("Error processing entry", citationKey, e.getMessage()); + LOGGER.error(message, e); + context.notificationService().notify(message); } private void finalizeOperation(List updatedEntries) { diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 4630a1b345c..19653fb2b05 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1680,7 +1680,10 @@ Entry\ from\ %0=Entry from %0 Merge\ entry\ with\ %0\ information=Merge entry with %0 information Get\ bibliographic\ data\ from\ %0\ (fully\ automated)=Get bibliographic data from %0 (fully automated) Batch\ update\ successful.\ %0\ entries\ updated.=Batch update successful. %0 entries updated. -Error\ while\ fetching\ and\ merging\ entries\:\ %0=Error while fetching and merging entries: %0 +Error\ processing\ entry=Error processing entry +Merge\ operation\ failed\:\ %0=Merge operation failed: %0 +No\ entries\ available\ for\ merging=No entries available for merging +Starting\ merge\ operation...=Starting merge operation... Processing\ entry\ %0\ of\ %1=Processing entry %0 of %1 Fetching\ and\ merging\ entries=Fetching and merging entries No\ updates\ found.=No updates found. From 4d1b88d4a0b06b7c76f316b6b148889c26ccd037 Mon Sep 17 00:00:00 2001 From: KUMOAWAI <128650848+KUMOAWAI@users.noreply.github.com> Date: Sun, 17 Nov 2024 14:17:32 +1100 Subject: [PATCH 26/28] Apply suggestions from code review Update JabRef_en.properties as koppor suggested Co-authored-by: Oliver Kopp --- src/main/resources/l10n/JabRef_en.properties | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 19653fb2b05..985f6c7c09c 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1679,13 +1679,13 @@ No\ %0\ found=No %0 found Entry\ from\ %0=Entry from %0 Merge\ entry\ with\ %0\ information=Merge entry with %0 information Get\ bibliographic\ data\ from\ %0\ (fully\ automated)=Get bibliographic data from %0 (fully automated) -Batch\ update\ successful.\ %0\ entries\ updated.=Batch update successful. %0 entries updated. +Batch\ update\ successful.\ %0\ entry(s)\ updated.=Batch update successful. %0 entry(s) updated. Error\ processing\ entry=Error processing entry Merge\ operation\ failed\:\ %0=Merge operation failed: %0 No\ entries\ available\ for\ merging=No entries available for merging Starting\ merge\ operation...=Starting merge operation... Processing\ entry\ %0\ of\ %1=Processing entry %0 of %1 -Fetching\ and\ merging\ entries=Fetching and merging entries +Fetching\ and\ merging\ entry(s)=Fetching and merging entry(s) No\ updates\ found.=No updates found. Fetching\ information\ using\ %0=Fetching information using %0 No\ information\ added=No information added From e8645a29fce70a82559a181bb54651bc54f2666f Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 17 Nov 2024 15:14:17 +1100 Subject: [PATCH 27/28] Modify log messages correspondingly. --- .../java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java index 9c4d5d7075e..8e3007683fc 100644 --- a/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java +++ b/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java @@ -40,7 +40,7 @@ public BatchEntryMergeTask(MergeContext context) { } private void configureTask() { - setTitle(Localization.lang("Fetching and merging entries")); + setTitle(Localization.lang("Fetching and merging entry(s)")); withInitialMessage(Localization.lang("Starting merge operation...")); showToUser(true); } @@ -127,7 +127,7 @@ private void finalizeOperation(List updatedEntries) { private void notifySuccess(int updateCount) { String message = updateCount == 0 ? Localization.lang("No updates found.") - : Localization.lang("Batch update successful. %0 entries updated.", updateCount); + : Localization.lang("Batch update successful. %0 entry(s) updated.", updateCount); context.notificationService().notify(message); } From ac8c59d81e172fea505e86754a9132059a3f9849 Mon Sep 17 00:00:00 2001 From: Boji Zhang Date: Sun, 17 Nov 2024 15:34:43 +1100 Subject: [PATCH 28/28] Refactor: Move string normalization from StringNormalizer to StringUtil - Moved normalize() method to StringUtil - use Pattern.compile for NORMALIZE_PATTERN - Update shouldUpdateField() method in MergingIdBasedFetcher to use StringUtil.normalize() --- .../fetcher/MergingIdBasedFetcher.java | 23 ++-------- .../org/jabref/model/strings/StringUtil.java | 45 +++++++++++++++++++ 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java index 522a0547ae1..fc0e1ca3955 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java @@ -12,6 +12,7 @@ import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.Field; import org.jabref.model.entry.field.StandardField; +import org.jabref.model.strings.StringUtil; import org.mariadb.jdbc.internal.logging.Logger; import org.mariadb.jdbc.internal.logging.LoggerFactory; @@ -116,13 +117,12 @@ private Set updateFieldsFromSource(BibEntry sourceEntry, .collect(Collectors.toSet()); } - private boolean shouldUpdateField(Field field, BibEntry sourceEntry, - BibEntry targetEntry) { + private boolean shouldUpdateField(Field field, BibEntry sourceEntry, BibEntry targetEntry) { String sourceValue = sourceEntry.getField(field) - .map(StringNormalizer::normalize) + .map(StringUtil::normalize) .orElse(""); String targetValue = targetEntry.getField(field) - .map(StringNormalizer::normalize) + .map(StringUtil::normalize) .orElse(""); return !sourceValue.equals(targetValue); } @@ -136,18 +136,3 @@ private void updateField(Field field, BibEntry sourceEntry, } } -class StringNormalizer { - public static String normalize(String value) { - if (value == null) { - return ""; - } - return value.trim() - .replaceAll("\\s+", " ") - .replaceAll("\\r\\n|\\r|\\n", " ") - .replaceAll("\\s*-+\\s*", "-") - .replaceAll("\\s*,\\s*", ", ") - .replaceAll("\\s*;\\s*", "; ") - .replaceAll("\\s*:\\s*", ": "); - } -} - diff --git a/src/main/java/org/jabref/model/strings/StringUtil.java b/src/main/java/org/jabref/model/strings/StringUtil.java index 2237526cd3e..6cec1b9322e 100644 --- a/src/main/java/org/jabref/model/strings/StringUtil.java +++ b/src/main/java/org/jabref/model/strings/StringUtil.java @@ -25,6 +25,12 @@ public class StringUtil { // contains all possible line breaks, not omitting any break such as "\\n" private static final Pattern LINE_BREAKS = Pattern.compile("\\r\\n|\\r|\\n"); private static final Pattern BRACED_TITLE_CAPITAL_PATTERN = Pattern.compile("\\{[A-Z]+\\}"); + private static final Pattern NORMALIZE_PATTERN = Pattern.compile( + "\\s+|" + // multiple whitespace + "\\s*-+\\s*|" + // hyphens with surrounding spaces + "\\s*,\\s*|" + // commas with surrounding spaces + "\\s*;\\s*|" + // semicolons with surrounding spaces + "\\s*:\\s*"); // colons with surrounding spaces private static final UnicodeToReadableCharMap UNICODE_CHAR_MAP = new UnicodeToReadableCharMap(); public static String booleanToBinaryString(boolean expression) { @@ -755,4 +761,43 @@ public static String removeStringAtTheEnd(String string, String stringToBeRemove public static boolean endsWithIgnoreCase(String string, String suffix) { return StringUtils.endsWithIgnoreCase(string, suffix); } + + /** + * Normalizes a string by standardizing whitespace and punctuation. This includes: + * - Trimming outer whitespace + * - Converting multiple whitespace characters to a single space + * - Converting line breaks to spaces + * - Standardizing formatting around punctuation (hyphens, commas, semicolons, colons) + * + * @param value The string to normalize + * @return The normalized string, or empty string if input is null + */ + public static String normalize(String value) { + if (value == null) { + return ""; + } + + String withoutLineBreaks = LINE_BREAKS.matcher(value).replaceAll(" "); + + String trimmed = withoutLineBreaks.trim(); + return NORMALIZE_PATTERN.matcher(trimmed).replaceAll(match -> { + String matchStr = match.group(); + if (matchStr.matches("\\s+")) { + return " "; + } + if (matchStr.matches("\\s*-+\\s*")) { + return "-"; + } + if (matchStr.matches("\\s*,\\s*")) { + return ", "; + } + if (matchStr.matches("\\s*;\\s*")) { + return "; "; + } + if (matchStr.matches("\\s*:\\s*")) { + return ": "; + } + return matchStr; + }); + } }