From 260ea31cfc1986502e18227bcae57ef2624dd3fe Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 23 Oct 2023 22:50:50 +0200 Subject: [PATCH] Minor code improvements to ImportHandler (#10562) * Minor code improvements * Introduce interface ImportCleanup (better inheritance) * Refine ImportHandler --- src/main/java/org/jabref/gui/JabRefFrame.java | 2 +- .../gui/entryeditor/RelatedArticlesTab.java | 2 +- .../gui/externalfiles/ImportHandler.java | 69 +++++++------------ .../gui/mergeentries/FetchAndMergeEntry.java | 4 +- .../jabref/logic/importer/ImportCleanup.java | 29 +++----- .../logic/importer/ImportCleanupBiblatex.java | 21 ++++++ .../logic/importer/ImportCleanupBibtex.java | 21 ++++++ .../gui/externalfiles/ImportHandlerTest.java | 12 ++-- .../importer/fetcher/ArXivFetcherTest.java | 6 +- .../CompositeSearchBasedFetcherTest.java | 2 +- .../SearchBasedFetcherCapabilityTest.java | 8 +-- 11 files changed, 92 insertions(+), 84 deletions(-) create mode 100644 src/main/java/org/jabref/logic/importer/ImportCleanupBiblatex.java create mode 100644 src/main/java/org/jabref/logic/importer/ImportCleanupBibtex.java diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java index 3dff38bafbb..f8fe7a35937 100644 --- a/src/main/java/org/jabref/gui/JabRefFrame.java +++ b/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 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")); diff --git a/src/main/java/org/jabref/gui/entryeditor/RelatedArticlesTab.java b/src/main/java/org/jabref/gui/entryeditor/RelatedArticlesTab.java index baf22123c35..1cf3ad74e5b 100644 --- a/src/main/java/org/jabref/gui/entryeditor/RelatedArticlesTab.java +++ b/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)); diff --git a/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java b/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java index 430099becdc..a9fbbe28eb8 100644 --- a/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java +++ b/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 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 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 existingDuplicateInLibrary = findDuplicate(bibDatabaseContext, cleanedEntry); - - BibEntry entryToInsert = cleanedEntry; - + BibEntry entryToInsert = cleanUpEntry(bibDatabaseContext, entry); + Optional existingDuplicateInLibrary = findDuplicate(bibDatabaseContext, entryToInsert); if (existingDuplicateInLibrary.isPresent()) { - Optional duplicateHandledEntry = handleDuplicates(bibDatabaseContext, cleanedEntry, existingDuplicateInLibrary.get()); + Optional 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 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 entries, Collection group * @param entries entries to generate keys for */ private void generateKeys(List 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 handleBibTeXData(String entries) { diff --git a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java index 0aa765d81ba..c5c65b1cc5d 100644 --- a/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java +++ b/src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java @@ -77,7 +77,7 @@ public void fetchAndMerge(BibEntry entry, List 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 { diff --git a/src/main/java/org/jabref/logic/importer/ImportCleanup.java b/src/main/java/org/jabref/logic/importer/ImportCleanup.java index 6aae40cd0d5..c6af4d0a2cd 100644 --- a/src/main/java/org/jabref/logic/importer/ImportCleanup.java +++ b/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 entries) { + default void doPostCleanup(Collection entries) { entries.forEach(this::doPostCleanup); } } diff --git a/src/main/java/org/jabref/logic/importer/ImportCleanupBiblatex.java b/src/main/java/org/jabref/logic/importer/ImportCleanupBiblatex.java new file mode 100644 index 00000000000..99919a2d3f0 --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/org/jabref/logic/importer/ImportCleanupBibtex.java b/src/main/java/org/jabref/logic/importer/ImportCleanupBibtex.java new file mode 100644 index 00000000000..ddda1f73e73 --- /dev/null +++ b/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; + } +} diff --git a/src/test/java/org/jabref/gui/externalfiles/ImportHandlerTest.java b/src/test/java/org/jabref/gui/externalfiles/ImportHandlerTest.java index 0c17b00335d..2e973ac5a23 100644 --- a/src/test/java/org/jabref/gui/externalfiles/ImportHandlerTest.java +++ b/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 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 diff --git a/src/test/java/org/jabref/logic/importer/fetcher/ArXivFetcherTest.java b/src/test/java/org/jabref/logic/importer/fetcher/ArXivFetcherTest.java index 5ab1bf7032f..65d3c2bcfc9 100644 --- a/src/test/java/org/jabref/logic/importer/fetcher/ArXivFetcherTest.java +++ b/src/test/java/org/jabref/logic/importer/fetcher/ArXivFetcherTest.java @@ -182,7 +182,7 @@ public void supportsAuthorSearch() throws FetcherException { getInputTestAuthors().forEach(queryBuilder::add); List 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 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 diff --git a/src/test/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcherTest.java b/src/test/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcherTest.java index 27b781014fb..8667a600427 100644 --- a/src/test/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcherTest.java +++ b/src/test/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcherTest.java @@ -68,7 +68,7 @@ public void performSearchOnEmptyQuery(Set fetchers) throws E @MethodSource("performSearchParameters") public void performSearchOnNonEmptyQuery(Set fetchers) throws Exception { CompositeSearchBasedFetcher compositeFetcher = new CompositeSearchBasedFetcher(fetchers, Integer.MAX_VALUE); - ImportCleanup cleanup = new ImportCleanup(BibDatabaseMode.BIBTEX); + ImportCleanup cleanup = ImportCleanup.targeting(BibDatabaseMode.BIBTEX); List compositeResult = compositeFetcher.performSearch("quantum"); for (SearchBasedFetcher fetcher : fetchers) { diff --git a/src/test/java/org/jabref/logic/importer/fetcher/SearchBasedFetcherCapabilityTest.java b/src/test/java/org/jabref/logic/importer/fetcher/SearchBasedFetcherCapabilityTest.java index 3de0af37fd4..a93f73bed17 100644 --- a/src/test/java/org/jabref/logic/importer/fetcher/SearchBasedFetcherCapabilityTest.java +++ b/src/test/java/org/jabref/logic/importer/fetcher/SearchBasedFetcherCapabilityTest.java @@ -36,7 +36,7 @@ default void supportsAuthorSearch() throws Exception { getTestAuthors().forEach(queryBuilder::add); List 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 result = getFetcher().performSearch("year:" + getTestYear()); - new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result); + ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result); List differentYearsInResult = result.stream() .map(bibEntry -> bibEntry.getField(StandardField.YEAR)) .filter(Optional::isPresent) @@ -72,7 +72,7 @@ default void supportsYearRangeSearch() throws Exception { List yearsInYearRange = List.of("2018", "2019", "2020"); List result = getFetcher().performSearch("year-range:2018-2020"); - new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result); + ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result); List 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 result = getFetcher().performSearch("journal:\"" + getTestJournal() + "\""); - new ImportCleanup(BibDatabaseMode.BIBTEX).doPostCleanup(result); + ImportCleanup.targeting(BibDatabaseMode.BIBTEX).doPostCleanup(result); assertFalse(result.isEmpty()); result.forEach(bibEntry -> {