Minor code improvements to ImportHandler (JabRef#10562)

* Minor code improvements

* Introduce interface ImportCleanup (better inheritance)

* Refine ImportHandler

src/main/java/org/jabref/gui/JabRefFrame.java

@@ -790,7 +790,7 @@ public void addTab(ParserResult parserResult, boolean raisePanel) {
      */
     private void addImportedEntries(final LibraryTab panel, final ParserResult parserResult) {
         BackgroundTask<ParserResult> task = BackgroundTask.wrap(() -> parserResult);
-        ImportCleanup cleanup = new ImportCleanup(panel.getBibDatabaseContext().getMode());
+        ImportCleanup cleanup = ImportCleanup.targeting(panel.getBibDatabaseContext().getMode());
         cleanup.doPostCleanup(parserResult.getDatabase().getEntries());
         ImportEntriesDialog dialog = new ImportEntriesDialog(panel.getBibDatabaseContext(), task);
         dialog.setTitle(Localization.lang("Import"));

src/main/java/org/jabref/gui/entryeditor/RelatedArticlesTab.java

@@ -78,7 +78,7 @@ private StackPane getRelatedArticlesPane(BibEntry entry) {
                 .wrap(() -> fetcher.performSearch(entry))
                 .onRunning(() -> progress.setVisible(true))
                 .onSuccess(relatedArticles -> {
-                    ImportCleanup cleanup = new ImportCleanup(BibDatabaseModeDetection.inferMode(new BibDatabase(List.of(entry))));
+                    ImportCleanup cleanup = ImportCleanup.targeting(BibDatabaseModeDetection.inferMode(new BibDatabase(List.of(entry))));
                     cleanup.doPostCleanup(relatedArticles);
                     progress.setVisible(false);
                     root.getChildren().add(getRelatedArticleInfo(relatedArticles, fetcher));

src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

@@ -52,6 +52,7 @@
 import org.jabref.model.util.OptionalUtil;
 import org.jabref.preferences.PreferencesService;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -174,50 +175,40 @@ private BibEntry createEmptyEntryWithLink(Path file) {
         return entry;
     }
 
+    /**
+     * Cleans up the given entries and adds them to the library.
+     * There is no automatic download done.
+     */
     public void importEntries(List<BibEntry> entries) {
-        ImportCleanup cleanup = new ImportCleanup(bibDatabaseContext.getMode());
+        ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode());
         cleanup.doPostCleanup(entries);
-        bibDatabaseContext.getDatabase().insertEntries(entries);
-
-        // Set owner/timestamp
-        UpdateField.setAutomaticFields(entries,
-                preferencesService.getOwnerPreferences(),
-                preferencesService.getTimestampPreferences());
-
-        // Generate citation keys
-        if (preferencesService.getImporterPreferences().isGenerateNewKeyOnImport()) {
-            generateKeys(entries);
-        }
+        importCleanedEntries(entries);
+    }
 
-        // Add to group
+    public void importCleanedEntries(List<BibEntry> entries) {
+        bibDatabaseContext.getDatabase().insertEntries(entries);
+        generateKeys(entries);
+        setAutomaticFields(entries);
         addToGroups(entries, stateManager.getSelectedGroup(bibDatabaseContext));
     }
 
     public void importEntryWithDuplicateCheck(BibDatabaseContext bibDatabaseContext, BibEntry entry) {
-        BibEntry cleanedEntry = cleanUpEntry(bibDatabaseContext, entry);
-        Optional<BibEntry> existingDuplicateInLibrary = findDuplicate(bibDatabaseContext, cleanedEntry);
-
-        BibEntry entryToInsert = cleanedEntry;
-
+        BibEntry entryToInsert = cleanUpEntry(bibDatabaseContext, entry);
+        Optional<BibEntry> existingDuplicateInLibrary = findDuplicate(bibDatabaseContext, entryToInsert);
         if (existingDuplicateInLibrary.isPresent()) {
-            Optional<BibEntry> duplicateHandledEntry = handleDuplicates(bibDatabaseContext, cleanedEntry, existingDuplicateInLibrary.get());
+            Optional<BibEntry> duplicateHandledEntry = handleDuplicates(bibDatabaseContext, entryToInsert, existingDuplicateInLibrary.get());
             if (duplicateHandledEntry.isEmpty()) {
                 return;
             }
             entryToInsert = duplicateHandledEntry.get();
         }
-        regenerateCiteKey(entryToInsert);
-        bibDatabaseContext.getDatabase().insertEntry(entryToInsert);
-
-        setAutomaticFieldsForEntry(entryToInsert);
-
-        addEntryToGroups(entryToInsert);
-
+        importCleanedEntries(List.of(entryToInsert));
         downloadLinkedFiles(entryToInsert);
     }
 
-    public BibEntry cleanUpEntry(BibDatabaseContext bibDatabaseContext, BibEntry entry) {
-        ImportCleanup cleanup = new ImportCleanup(bibDatabaseContext.getMode());
+    @VisibleForTesting
+    BibEntry cleanUpEntry(BibDatabaseContext bibDatabaseContext, BibEntry entry) {
+        ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode());
         return cleanup.doPostCleanup(entry);
     }
 
@@ -251,24 +242,14 @@ public DuplicateDecisionResult getDuplicateDecision(BibEntry originalEntry, BibE
         return new DuplicateDecisionResult(decision, dialog.getMergedEntry());
     }
 
-    public void regenerateCiteKey(BibEntry entry) {
-        if (preferencesService.getImporterPreferences().isGenerateNewKeyOnImport()) {
-            generateKeys(List.of(entry));
-        }
-    }
-
-    public void setAutomaticFieldsForEntry(BibEntry entry) {
+    public void setAutomaticFields(List<BibEntry> entries) {
         UpdateField.setAutomaticFields(
-                List.of(entry),
+                entries,
                 preferencesService.getOwnerPreferences(),
                 preferencesService.getTimestampPreferences()
         );
     }
 
-    public void addEntryToGroups(BibEntry entry) {
-        addToGroups(List.of(entry), stateManager.getSelectedGroup(this.bibDatabaseContext));
-    }
-
     public void downloadLinkedFiles(BibEntry entry) {
         if (preferencesService.getFilePreferences().shouldDownloadLinkedFiles()) {
             entry.getFiles().stream()
@@ -305,15 +286,15 @@ private void addToGroups(List<BibEntry> entries, Collection<GroupTreeNode> group
      * @param entries entries to generate keys for
      */
     private void generateKeys(List<BibEntry> entries) {
+        if (!preferencesService.getImporterPreferences().isGenerateNewKeyOnImport()) {
+            return;
+        }
         CitationKeyGenerator keyGenerator = new CitationKeyGenerator(
                 bibDatabaseContext.getMetaData().getCiteKeyPattern(preferencesService.getCitationKeyPatternPreferences()
                                                                                      .getKeyPattern()),
                 bibDatabaseContext.getDatabase(),
                 preferencesService.getCitationKeyPatternPreferences());
-
-        for (BibEntry entry : entries) {
-            keyGenerator.generateAndSetKey(entry);
-        }
+        entries.forEach(keyGenerator::generateAndSetKey);
     }
 
     public List<BibEntry> handleBibTeXData(String entries) {

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

@@ -77,7 +77,7 @@ public void fetchAndMerge(BibEntry entry, List<Field> fields) {
                 if (fetcher.isPresent()) {
                     BackgroundTask.wrap(() -> fetcher.get().performSearchById(fieldContent.get()))
                                   .onSuccess(fetchedEntry -> {
-                                      ImportCleanup cleanup = new ImportCleanup(bibDatabaseContext.getMode());
+                                      ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode());
                                       String type = field.getDisplayName();
                                       if (fetchedEntry.isPresent()) {
                                           cleanup.doPostCleanup(fetchedEntry.get());
@@ -169,7 +169,7 @@ public void fetchAndMerge(BibEntry entry, EntryBasedFetcher fetcher) {
         BackgroundTask.wrap(() -> fetcher.performSearch(entry).stream().findFirst())
                       .onSuccess(fetchedEntry -> {
                           if (fetchedEntry.isPresent()) {
-                              ImportCleanup cleanup = new ImportCleanup(bibDatabaseContext.getMode());
+                              ImportCleanup cleanup = ImportCleanup.targeting(bibDatabaseContext.getMode());
                               cleanup.doPostCleanup(fetchedEntry.get());
                               showMergeDialog(entry, fetchedEntry.get(), fetcher);
                           } else {

src/main/java/org/jabref/logic/importer/ImportCleanup.java

@@ -2,37 +2,24 @@
 
 import java.util.Collection;
 
-import org.jabref.logic.cleanup.ConvertToBiblatexCleanup;
-import org.jabref.logic.cleanup.ConvertToBibtexCleanup;
 import org.jabref.model.database.BibDatabaseMode;
 import org.jabref.model.entry.BibEntry;
 
-public class ImportCleanup {
+public interface ImportCleanup {
 
-    private final BibDatabaseMode targetBibEntryFormat;
-
-    public ImportCleanup(BibDatabaseMode targetBibEntryFormat) {
-        this.targetBibEntryFormat = targetBibEntryFormat;
+    static ImportCleanup targeting(BibDatabaseMode mode) {
+        return switch (mode) {
+            case BIBTEX -> new ImportCleanupBibtex();
+            case BIBLATEX -> new ImportCleanupBiblatex();
+        };
     }
 
-    /**
-     * Performs a format conversion of the given entry into the targeted format.
-     *
-     * @return Returns the cleaned up bibentry to enable usage of doPostCleanup in streams.
-     */
-    public BibEntry doPostCleanup(BibEntry entry) {
-        if (targetBibEntryFormat == BibDatabaseMode.BIBTEX) {
-            new ConvertToBibtexCleanup().cleanup(entry);
-        } else if (targetBibEntryFormat == BibDatabaseMode.BIBLATEX) {
-            new ConvertToBiblatexCleanup().cleanup(entry);
-        }
-        return entry;
-    }
+    BibEntry doPostCleanup(BibEntry entry);
 
     /**
      * Performs a format conversion of the given entry collection into the targeted format.
      */
-    public void doPostCleanup(Collection<BibEntry> entries) {
+    default void doPostCleanup(Collection<BibEntry> entries) {
         entries.forEach(this::doPostCleanup);
     }
 }

src/main/java/org/jabref/logic/importer/ImportCleanupBiblatex.java

@@ -0,0 +1,21 @@
+package org.jabref.logic.importer;
+
+import org.jabref.logic.cleanup.ConvertToBiblatexCleanup;
+import org.jabref.model.entry.BibEntry;
+
+public class ImportCleanupBiblatex implements ImportCleanup {
+
+    private final ConvertToBiblatexCleanup convertToBiblatexCleanup = new ConvertToBiblatexCleanup();
+
+    /**
+     * Performs a format conversion of the given entry into the targeted format.
+     * Modifies the given entry and also returns it to enable usage of doPostCleanup in streams.
+     *
+     * @return Cleaned up BibEntry
+     */
+    @Override
+    public BibEntry doPostCleanup(BibEntry entry) {
+         convertToBiblatexCleanup.cleanup(entry);
+         return entry;
+    }
+}

src/main/java/org/jabref/logic/importer/ImportCleanupBibtex.java

@@ -0,0 +1,21 @@
+package org.jabref.logic.importer;
+
+import org.jabref.logic.cleanup.ConvertToBibtexCleanup;
+import org.jabref.model.entry.BibEntry;
+
+public class ImportCleanupBibtex implements ImportCleanup {
+
+    private final ConvertToBibtexCleanup convertToBibtexCleanup = new ConvertToBibtexCleanup();
+
+    /**
+     * Performs a format conversion of the given entry into the targeted format.
+     * Modifies the given entry and also returns it to enable usage of doPostCleanup in streams.
+     *
+     * @return Cleaned up BibEntry
+     */
+    @Override
+    public BibEntry doPostCleanup(BibEntry entry) {
+        convertToBibtexCleanup.cleanup(entry);
+        return entry;
+    }
+}

src/test/java/org/jabref/gui/externalfiles/ImportHandlerTest.java

@@ -1,7 +1,6 @@
 package org.jabref.gui.externalfiles;
 
 import java.util.List;
-import java.util.Optional;
 
 import javax.swing.undo.UndoManager;
 
@@ -13,6 +12,7 @@
 import org.jabref.logic.importer.ImportFormatPreferences;
 import org.jabref.model.database.BibDatabase;
 import org.jabref.model.database.BibDatabaseContext;
+import org.jabref.model.database.BibDatabaseMode;
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.entry.field.StandardField;
 import org.jabref.model.entry.types.StandardEntryType;
@@ -56,6 +56,7 @@ void setUp() {
 
         bibDatabaseContext = mock(BibDatabaseContext.class);
         BibDatabase bibDatabase = new BibDatabase();
+        when(bibDatabaseContext.getMode()).thenReturn(BibDatabaseMode.BIBTEX);
         when(bibDatabaseContext.getDatabase()).thenReturn(bibDatabase);
         when(duplicateCheck.isDuplicate(any(), any(), any())).thenReturn(false);
         importHandler = new ImportHandler(
@@ -107,16 +108,13 @@ void handleBibTeXData() {
     void cleanUpEntryTest() {
         BibEntry entry = new BibEntry().withField(StandardField.AUTHOR, "Clear Author");
         BibEntry cleanedEntry = importHandler.cleanUpEntry(bibDatabaseContext, entry);
-
-        Optional<String> authorOptional = cleanedEntry.getField(StandardField.AUTHOR);
-
-        assertEquals(Optional.of("Clear Author"), authorOptional);
+        assertEquals(new BibEntry().withField(StandardField.AUTHOR, "Clear Author"), cleanedEntry);
     }
 
     @Test
     void findDuplicateTest() {
-        // Assume there's no duplicate initially
-        assertFalse(importHandler.findDuplicate(bibDatabaseContext, testEntry).isPresent());
+        // Assume there is no duplicate initially
+        assertTrue(importHandler.findDuplicate(bibDatabaseContext, testEntry).isEmpty());
     }
 
     @Test

src/test/java/org/jabref/logic/importer/fetcher/ArXivFetcherTest.java

@@ -182,7 +182,7 @@ public void supportsAuthorSearch() throws FetcherException {
         getInputTestAuthors().forEach(queryBuilder::add);
 
         List<BibEntry> result = getFetcher().performSearch(queryBuilder.toString());
-        new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result);
+        ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result);
 
         assertFalse(result.isEmpty());
         result.forEach(bibEntry -> {
@@ -199,9 +199,9 @@ public void noSupportsAuthorSearchWithLastFirstName() throws FetcherException {
         getTestAuthors().forEach(queryBuilder::add);
 
         List<BibEntry> result = getFetcher().performSearch(queryBuilder.toString());
-        new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result);
+        ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result);
 
-        assertTrue(result.isEmpty());
+        assertEquals(List.of(), result);
     }
 
     @Test

src/test/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcherTest.java

@@ -68,7 +68,7 @@ public void performSearchOnEmptyQuery(Set<SearchBasedFetcher> fetchers) throws E
     @MethodSource("performSearchParameters")
     public void performSearchOnNonEmptyQuery(Set<SearchBasedFetcher> fetchers) throws Exception {
         CompositeSearchBasedFetcher compositeFetcher = new CompositeSearchBasedFetcher(fetchers, Integer.MAX_VALUE);
-        ImportCleanup cleanup = new ImportCleanup(BibDatabaseMode.BIBTEX);
+        ImportCleanup cleanup = ImportCleanup.targeting(BibDatabaseMode.BIBTEX);
 
         List<BibEntry> compositeResult = compositeFetcher.performSearch("quantum");
         for (SearchBasedFetcher fetcher : fetchers) {

src/test/java/org/jabref/logic/importer/fetcher/SearchBasedFetcherCapabilityTest.java

@@ -36,7 +36,7 @@ default void supportsAuthorSearch() throws Exception {
         getTestAuthors().forEach(queryBuilder::add);
 
         List<BibEntry> result = getFetcher().performSearch(queryBuilder.toString());
-        new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result);
+        ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result);
 
         assertFalse(result.isEmpty());
         result.forEach(bibEntry -> {
@@ -53,7 +53,7 @@ default void supportsAuthorSearch() throws Exception {
     @Test
     default void supportsYearSearch() throws Exception {
         List<BibEntry> result = getFetcher().performSearch("year:" + getTestYear());
-        new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result);
+        ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result);
         List<String> differentYearsInResult = result.stream()
                                                     .map(bibEntry -> bibEntry.getField(StandardField.YEAR))
                                                     .filter(Optional::isPresent)
@@ -72,7 +72,7 @@ default void supportsYearRangeSearch() throws Exception {
         List<String> yearsInYearRange = List.of("2018", "2019", "2020");
 
         List<BibEntry> result = getFetcher().performSearch("year-range:2018-2020");
-        new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result);
+        ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result);
         List<String> differentYearsInResult = result.stream()
                                                     .map(bibEntry -> bibEntry.getField(StandardField.YEAR))
                                                     .filter(Optional::isPresent)
@@ -92,7 +92,7 @@ default void supportsYearRangeSearch() throws Exception {
     @Test
     default void supportsJournalSearch() throws Exception {
         List<BibEntry> result = getFetcher().performSearch("journal:\"" + getTestJournal() + "\"");
-        new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result);
+        ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result);
 
         assertFalse(result.isEmpty());
         result.forEach(bibEntry -> {