Simplify

Co-authored-by: Oliver Kopp <[email protected]>

Author: calixtus

jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java

@@ -3,11 +3,11 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
-import java.util.concurrent.atomic.AtomicInteger;
 
 import javax.swing.undo.UndoManager;
 
 import org.jabref.gui.undo.NamedCompound;
+import org.jabref.gui.util.UiTaskExecutor;
 import org.jabref.logic.importer.fetcher.MergingIdBasedFetcher;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.logic.util.BackgroundTask;
@@ -17,23 +17,22 @@
 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<List<String>> {
+/// 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<Void> {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(BatchEntryMergeTask.class);
 
     private final NamedCompound compoundEdit;
-    private final AtomicInteger processedEntries;
-    private final AtomicInteger successfulUpdates;
     private final List<BibEntry> entries;
     private final MergingIdBasedFetcher fetcher;
     private final UndoManager undoManager;
     private final NotificationService notificationService;
 
+    private int processedEntries;
+    private int successfulUpdates;
+
     public BatchEntryMergeTask(List<BibEntry> entries,
                                MergingIdBasedFetcher fetcher,
                                UndoManager undoManager,
@@ -44,56 +43,50 @@ public BatchEntryMergeTask(List<BibEntry> entries,
         this.notificationService = notificationService;
 
         this.compoundEdit = new NamedCompound(Localization.lang("Merge entries"));
-        this.processedEntries = new AtomicInteger(0);
-        this.successfulUpdates = new AtomicInteger(0);
-
-        configureTask();
-    }
+        this.processedEntries = 0;
+        this.successfulUpdates = 0;
 
-    private void configureTask() {
         setTitle(Localization.lang("Fetching and merging entry(s)"));
         withInitialMessage(Localization.lang("Starting merge operation..."));
         showToUser(true);
     }
 
     @Override
-    public List<String> call() {
+    public Void call() {
             if (isCancelled()) {
                 notifyCancellation();
-                return List.of();
+                return null;
             }
 
             List<String> updatedEntries = processMergeEntries();
 
             if (isCancelled()) {
                 notifyCancellation();
-                finalizeOperation(updatedEntries);
-                return updatedEntries;
+                updateUndoManager(updatedEntries);
+                return null;
             }
 
-            finalizeOperation(updatedEntries);
+            updateUndoManager(updatedEntries);
             LOGGER.debug("Merge operation completed. Processed: {}, Successfully updated: {}",
-                    processedEntries.get(), successfulUpdates.get());
-            notifySuccess(successfulUpdates.get());
-            return updatedEntries;
+                    processedEntries, successfulUpdates);
+            notifySuccess(successfulUpdates);
+            return null;
     }
 
     private void notifyCancellation() {
         LOGGER.debug("Merge operation was cancelled. Processed: {}, Successfully updated: {}",
-                processedEntries.get(), successfulUpdates.get());
+                processedEntries, successfulUpdates);
         notificationService.notify(
-                Localization.lang("Merge operation cancelled after updating %0 entry(s)", successfulUpdates.get()));
+                Localization.lang("Merge operation cancelled after updating %0 entry(s)", successfulUpdates));
     }
 
     private List<String> processMergeEntries() {
-        List<String> updatedEntries = new ArrayList<>();
+        List<String> updatedEntries = new ArrayList<>(entries.size());
 
         for (BibEntry entry : entries) {
-            Optional<String> result = processSingleEntryWithProgress(entry);
-            result.ifPresent(updatedEntries::add);
-
+            processSingleEntryWithProgress(entry).ifPresent(updatedEntries::add);
             if (isCancelled()) {
-                LOGGER.debug("Cancellation requested after processing entry {}", processedEntries.get());
+                LOGGER.debug("Cancellation requested after processing entry {}", processedEntries);
                 break;
             }
         }
@@ -102,56 +95,31 @@ private List<String> processMergeEntries() {
     }
 
     private Optional<String> processSingleEntryWithProgress(BibEntry entry) {
-        updateProgress(processedEntries.incrementAndGet(), entries.size());
+        updateProgress(++processedEntries, entries.size());
         updateMessage(Localization.lang("Processing entry %0 of %1",
-                processedEntries.get(),
+                processedEntries,
                 entries.size()));
         return processSingleEntry(entry);
     }
 
     private Optional<String> processSingleEntry(BibEntry entry) {
-        try {
-            LOGGER.debug("Processing entry: {}", entry);
-            return 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();
-        }
+        LOGGER.debug("Processing entry: {}", entry);
+        return fetcher.fetchEntry(entry)
+                      .filter(MergingIdBasedFetcher.FetcherResult::hasChanges)
+                      .flatMap(result -> {
+                          boolean changesApplied = MergeEntriesHelper.mergeEntries(result.mergedEntry(), entry, compoundEdit);
+                          if (changesApplied) {
+                              successfulUpdates++;
+                              return entry.getCitationKey();
+                          }
+                          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 message = Localization.lang("Error processing entry", citationKey, e.getMessage());
-        LOGGER.error(message, e);
-        notificationService.notify(message);
-    }
-
-    private void finalizeOperation(List<String> updatedEntries) {
+    private void updateUndoManager(List<String> updatedEntries) {
         if (!updatedEntries.isEmpty()) {
-            synchronized (compoundEdit) {
-                compoundEdit.end();
-                undoManager.addEdit(compoundEdit);
-            }
+            compoundEdit.end();
+            UiTaskExecutor.runInJavaFXThread(() -> undoManager.addEdit(compoundEdit));
         }
     }
 

jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java

@@ -15,9 +15,7 @@
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntry;
 
-/**
- * Handles batch merging of bibliography entries with fetched data.
- */
+/// Handles batch merging of bibliography entries with fetched data.
 public class BatchEntryMergeWithFetchedDataAction extends SimpleCommand {
 
     private final StateManager stateManager;

jabgui/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java

@@ -27,38 +27,36 @@ public final class MergeEntriesHelper {
     private MergeEntriesHelper() {
     }
 
-    /**
-     * Merges two BibEntry objects with undo support.
-     *
-     * @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 boolean mergeEntries(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) {
+    /// Merges two BibEntry objects with undo support.
+    ///
+    /// @param entryFromFetcher The entry containing new information (source, from the fetcher)
+    /// @param entryFromLibrary The entry to be updated (target, from the library)
+    /// @param namedCompound Compound edit to collect undo information
+    public static boolean mergeEntries(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound namedCompound) {
         LOGGER.debug("Entry from fetcher: {}", entryFromFetcher);
         LOGGER.debug("Entry from library: {}", entryFromLibrary);
 
-        boolean typeChanged = mergeEntryType(entryFromFetcher, entryFromLibrary, undoManager);
-        boolean fieldsChanged = mergeFields(entryFromFetcher, entryFromLibrary, undoManager);
-        boolean fieldsRemoved = removeFieldsNotPresentInFetcher(entryFromFetcher, entryFromLibrary, undoManager);
+        boolean typeChanged = mergeEntryType(entryFromFetcher, entryFromLibrary, namedCompound);
+        boolean fieldsChanged = mergeFields(entryFromFetcher, entryFromLibrary, namedCompound);
+        boolean fieldsRemoved = removeFieldsNotPresentInFetcher(entryFromFetcher, entryFromLibrary, namedCompound);
 
         return typeChanged || fieldsChanged || fieldsRemoved;
     }
 
-    private static boolean mergeEntryType(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) {
+    private static boolean mergeEntryType(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound namedCompound) {
         EntryType fetcherType = entryFromFetcher.getType();
         EntryType libraryType = entryFromLibrary.getType();
 
         if (!libraryType.equals(fetcherType)) {
             LOGGER.debug("Updating type {} -> {}", libraryType, fetcherType);
             entryFromLibrary.setType(fetcherType);
-            undoManager.addEdit(new UndoableChangeType(entryFromLibrary, libraryType, fetcherType));
+            namedCompound.addEdit(new UndoableChangeType(entryFromLibrary, libraryType, fetcherType));
             return true;
         }
         return false;
     }
 
-    private static boolean mergeFields(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) {
+    private static boolean mergeFields(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound namedCompound) {
         Set<Field> allFields = new LinkedHashSet<>();
         allFields.addAll(entryFromFetcher.getFields());
         allFields.addAll(entryFromLibrary.getFields());
@@ -72,14 +70,14 @@ private static boolean mergeFields(BibEntry entryFromFetcher, BibEntry entryFrom
             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()));
+                namedCompound.addEdit(new UndoableFieldChange(entryFromLibrary, field, libraryValue.orElse(null), fetcherValue.get()));
                 anyFieldsChanged = true;
             }
         }
         return anyFieldsChanged;
     }
 
-    private static boolean removeFieldsNotPresentInFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound undoManager) {
+    private static boolean removeFieldsNotPresentInFetcher(BibEntry entryFromFetcher, BibEntry entryFromLibrary, NamedCompound namedCompound) {
         Set<Field> obsoleteFields = new LinkedHashSet<>(entryFromLibrary.getFields());
         obsoleteFields.removeAll(entryFromFetcher.getFields());
 
@@ -94,15 +92,18 @@ private static boolean removeFieldsNotPresentInFetcher(BibEntry entryFromFetcher
             if (value.isPresent()) {
                 LOGGER.debug("Removing obsolete field {} with value {}", field, value.get());
                 entryFromLibrary.clearField(field);
-                undoManager.addEdit(new UndoableFieldChange(entryFromLibrary, field, value.get(), null));
+                namedCompound.addEdit(new UndoableFieldChange(entryFromLibrary, field, value.get(), null));
                 anyFieldsRemoved = true;
             }
         }
         return anyFieldsRemoved;
     }
 
     private static boolean shouldUpdateField(String fetcherValue, Optional<String> libraryValue) {
-        return libraryValue.map(value -> fetcherValue.length() > value.length())
-                           .orElse(true);
+        // TODO: Think of a better heuristics - better "quality" is the ultimate goal (e.g., more sensible year, better page ranges, longer abstract ...)
+        //       This is difficult to get 100% right
+        //       Read more at https://github.com/JabRef/jabref/issues/12549
+        // Currently: Only overwrite if there is nothing in the library
+        return libraryValue.isEmpty();
     }
 }

jablib/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

@@ -17,13 +17,11 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-/**
- * 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.
- */
+/// 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 {
     private static final Logger LOGGER = LoggerFactory.getLogger(MergingIdBasedFetcher.class);
     private static final List<StandardField> SUPPORTED_FIELDS =

jablib/src/main/java/org/jabref/logic/util/BackgroundTask.java

@@ -20,19 +20,21 @@
 import org.slf4j.LoggerFactory;
 
 /// This class is essentially a wrapper around [javafx.concurrent.Task].
-/// We cannot use [javafx.concurrent.Task] directly since it runs certain update notifications on the JavaFX thread,
-/// and so makes testing harder.
-/// We take the opportunity and implement a fluid interface.
 ///
 /// A task created here is to be submitted to `#execute(BackgroundTask)` to submit.
-/// This class is injected at `@Inject TaskExecutor`
+/// BackgroundTask class is mostly injected by `@Inject TaskExecutor` or provided during class initialization.
+///
+/// We take the opportunity and implement a fluid interface.
 ///
 /// Example (for using the fluent interface)
 ///
 ///     BackgroundTask.wrap(() -> ...).showToUser(true).onRunning(() -> ...).onSuccess(() -> ...).onFailure(() -> ...).executeWith(taskExecutor);
 ///
 /// Background: The task executor one takes care to show it in the UI. See `org.jabref.gui.StateManager#addBackgroundTask(BackgroundTask, Task)` for details.
 ///
+/// We cannot use [javafx.concurrent.Task] directly since it runs certain update notifications on the JavaFX thread,
+/// and so makes testing harder.
+///
 /// TODO: Think of migrating to <a href="https://github.com/ReactiveX/RxJava#simple-background-computation">RxJava</a>;
 ///       <a href="https://www.baeldung.com/java-completablefuture">CompletableFuture</a> do not seem to support everything.
 ///       If this is not possible, add an @implNote why.