Fix issue #11189 - Implement a caching solution with local storage for citation relations #11845
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,17 +36,20 @@ | |
| import org.jabref.gui.collab.entrychange.PreviewWithSourceTab; | ||
| import org.jabref.gui.desktop.os.NativeDesktop; | ||
| import org.jabref.gui.entryeditor.EntryEditorTab; | ||
| import org.jabref.gui.icon.IconTheme; | ||
| import org.jabref.gui.preferences.GuiPreferences; | ||
| import org.jabref.gui.util.NoSelectionModel; | ||
| import org.jabref.gui.util.ViewModelListCellFactory; | ||
| import org.jabref.logic.bibtex.BibEntryWriter; | ||
| import org.jabref.logic.bibtex.FieldPreferences; | ||
| import org.jabref.logic.bibtex.FieldWriter; | ||
| import org.jabref.logic.citation.repository.LRUBibEntryRelationsCache; | ||
| import org.jabref.logic.citation.repository.LRUBibEntryRelationsRepository; | ||
| import org.jabref.logic.citation.service.SearchCitationsRelationsService; | ||
| import org.jabref.logic.database.DuplicateCheck; | ||
| import org.jabref.logic.exporter.BibWriter; | ||
| import org.jabref.logic.importer.fetcher.CitationFetcher; | ||
| import org.jabref.logic.importer.fetcher.SemanticScholarCitationFetcher; | ||
| import org.jabref.logic.l10n.Localization; | ||
| import org.jabref.logic.os.OS; | ||
| import org.jabref.logic.util.BackgroundTask; | ||
|
|
@@ -85,7 +88,7 @@ public class CitationRelationsTab extends EntryEditorTab { | |
| private final GuiPreferences preferences; | ||
| private final LibraryTab libraryTab; | ||
| private final TaskExecutor taskExecutor; | ||
| private final SearchCitationsRelationsService searchCitationsRelationsService; | ||
| private final CitationsRelationsTabViewModel citationsRelationsTabViewModel; | ||
| private final DuplicateCheck duplicateCheck; | ||
| private final BibEntryTypesManager entryTypesManager; | ||
|
|
@@ -109,9 +112,22 @@ public CitationRelationsTab(DialogService dialogService, | |
|
|
||
| this.entryTypesManager = bibEntryTypesManager; | ||
| this.duplicateCheck = new DuplicateCheck(entryTypesManager); | ||
| var bibEntryRelationsRepository = new LRUBibEntryRelationsRepository( | ||
| new LRUBibEntryRelationsCache() | ||
| ); | ||
| this.searchCitationsRelationsService = new SearchCitationsRelationsService( | ||
| new SemanticScholarCitationFetcher(preferences.getImporterPreferences()), | ||
| bibEntryRelationsRepository | ||
| ); | ||
| citationsRelationsTabViewModel = new CitationsRelationsTabViewModel( | ||
| databaseContext, | ||
| preferences, | ||
| undoManager, | ||
| stateManager, | ||
| dialogService, | ||
| fileUpdateMonitor, | ||
| taskExecutor | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -405,41 +421,54 @@ private void searchForRelations(BibEntry entry, CheckListView<CitationRelationIt | |
| citedByTask.cancel(); | ||
| } | ||
|
|
||
| this.createBackGroundTask(entry, searchType, shouldRefresh) | ||
| .consumeOnRunning(task -> prepareToSearchForRelations( | ||
alexandre-cremieux marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| abortButton, refreshButton, importButton, progress, task | ||
| )) | ||
| .onSuccess(fetchedList -> onSearchForRelationsSucceed( | ||
| entry, | ||
| listView, | ||
| abortButton, | ||
| refreshButton, | ||
| searchType, | ||
| importButton, | ||
| progress, | ||
| fetchedList, | ||
| observableList | ||
| )) | ||
| .onFailure(exception -> { | ||
| LOGGER.error("Error while fetching citing Articles", exception); | ||
InAnYan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| hideNodes(abortButton, progress, importButton); | ||
| listView.setPlaceholder( | ||
| new Label(Localization.lang( | ||
| "Error while fetching citing entries: %0", exception.getMessage()) | ||
| ) | ||
alexandre-cremieux marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ); | ||
| refreshButton.setVisible(true); | ||
| dialogService.notify(exception.getMessage()); | ||
| }) | ||
| .executeWith(taskExecutor); | ||
| } | ||
|
|
||
| private BackgroundTask<List<BibEntry>> createBackGroundTask( | ||
|
There was a problem hiding this comment. Typo: Background (without uppercase G) There was a problem hiding this comment. The highlighted TODO: Which benefts are this - and why should it implemetned lateR? There was a problem hiding this comment. This TODO follows a discussion we had earlier that you can read here: #11845 (comment) There was a problem hiding this comment. But what stops you from returning a runnable? If it's used in many places and that makes you rewrite a lot of code to include There was a problem hiding this comment. Just in theory, this could be needed to keep the level of abstration. Should be decided on where the method is actually used. There was a problem hiding this comment. The
As mentioned, this improvement could indeed imply several code changes (in this class):
At least, I think we agree that this should not be part of this PR. As I saw that the current implementation could be improved (stopping a task that maybe shouldn't be stopped, avoid same task but different executors for each instances of the tab, etc). I proposed to take a look to this after merging this PR. Reason why the TODO is there (after Oliver's accepted that at that time). But I can remove completely the TODO also if it bother you (and I understand that cause I do not like TODO also). However, I already have another to issue opened for which I volunteered related to same tab... There was a problem hiding this comment. Maybe just add a link your reasoning in your comment in the to-do? |
||
| BibEntry entry, CitationFetcher.SearchType searchType, boolean shouldRefresh | ||
| ) { | ||
| return switch (searchType) { | ||
| case CitationFetcher.SearchType.CITES -> { | ||
| citingTask = BackgroundTask.wrap( | ||
alexandre-cremieux marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| () -> this.searchCitationsRelationsService.searchReferences(entry, shouldRefresh) | ||
| ); | ||
| yield citingTask; | ||
| } | ||
| case CitationFetcher.SearchType.CITED_BY -> { | ||
| citedByTask = BackgroundTask.wrap( | ||
| () -> this.searchCitationsRelationsService.searchCitations(entry, shouldRefresh) | ||
| ); | ||
| yield citedByTask; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private void onSearchForRelationsSucceed(BibEntry entry, CheckListView<CitationRelationItem> listView, | ||
| Button abortButton, Button refreshButton, | ||
| CitationFetcher.SearchType searchType, Button importButton, | ||
|
|
@@ -456,7 +485,7 @@ private void onSearchForRelationsSucceed(BibEntry entry, CheckListView<CitationR | |
| .map(localEntry -> new CitationRelationItem(entr, localEntry, true)) | ||
| .orElseGet(() -> new CitationRelationItem(entr, false))) | ||
| .toList() | ||
| ); | ||
|
|
||
| if (!observableList.isEmpty()) { | ||
| listView.refresh(); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package org.jabref.logic.citation.repository; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import org.jabref.model.entry.BibEntry; | ||
|
|
||
| public interface BibEntryRelationsRepository { | ||
alexandre-cremieux marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| void insertCitations(BibEntry entry, List<BibEntry> citations); | ||
|
|
||
| List<BibEntry> readCitations(BibEntry entry); | ||
|
|
||
| boolean containsCitations(BibEntry entry); | ||
|
|
||
| void insertReferences(BibEntry entry, List<BibEntry> citations); | ||
|
|
||
| List<BibEntry> readReferences(BibEntry entry); | ||
|
|
||
| boolean containsReferences(BibEntry entry); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package org.jabref.logic.citation.repository; | ||
|
|
||
| import java.util.LinkedHashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import org.jabref.model.entry.BibEntry; | ||
| import org.jabref.model.entry.identifier.DOI; | ||
|
|
||
| import org.eclipse.jgit.util.LRUMap; | ||
|
|
||
| public class LRUBibEntryRelationsCache { | ||
| private static final Integer MAX_CACHED_ENTRIES = 100; | ||
| private static final Map<DOI, Set<BibEntry>> CITATIONS_MAP = new LRUMap<>(MAX_CACHED_ENTRIES, MAX_CACHED_ENTRIES); | ||
| private static final Map<DOI, Set<BibEntry>> REFERENCES_MAP = new LRUMap<>(MAX_CACHED_ENTRIES, MAX_CACHED_ENTRIES); | ||
|
|
||
| public List<BibEntry> getCitations(BibEntry entry) { | ||
| return entry | ||
alexandre-cremieux marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .getDOI() | ||
| .stream() | ||
| .flatMap(doi -> CITATIONS_MAP.getOrDefault(doi, Set.of()).stream()) | ||
| .toList(); | ||
| } | ||
|
|
||
| public List<BibEntry> getReferences(BibEntry entry) { | ||
| return entry | ||
| .getDOI() | ||
| .stream() | ||
| .flatMap(doi -> REFERENCES_MAP.getOrDefault(doi, Set.of()).stream()) | ||
| .toList(); | ||
| } | ||
|
|
||
| public void cacheOrMergeCitations(BibEntry entry, List<BibEntry> citations) { | ||
alexandre-cremieux marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| entry.getDOI().ifPresent(doi -> { | ||
| var cachedRelations = CITATIONS_MAP.getOrDefault(doi, new LinkedHashSet<>()); | ||
| cachedRelations.addAll(citations); | ||
| CITATIONS_MAP.put(doi, cachedRelations); | ||
| }); | ||
| } | ||
|
|
||
| public void cacheOrMergeReferences(BibEntry entry, List<BibEntry> references) { | ||
| entry.getDOI().ifPresent(doi -> { | ||
| var cachedRelations = REFERENCES_MAP.getOrDefault(doi, new LinkedHashSet<>()); | ||
| cachedRelations.addAll(references); | ||
| REFERENCES_MAP.put(doi, cachedRelations); | ||
| }); | ||
| } | ||
|
|
||
| public boolean citationsCached(BibEntry entry) { | ||
| return entry.getDOI().map(CITATIONS_MAP::containsKey).orElse(false); | ||
| } | ||
|
|
||
| public boolean referencesCached(BibEntry entry) { | ||
| return entry.getDOI().map(REFERENCES_MAP::containsKey).orElse(false); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.