Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue #11189 part 00 refactor citation relation tab logic #11845

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix issue #11189 part 00 refactor citation relation tab logic #11845

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3785cb1
Refactor Citations Relations Tab (#11189)
alexandre-cremieux Sep 19, 2024
8048da8
Refactor Citations Relations Tab (#11189)
alexandre-cremieux Sep 24, 2024
18db75e
Refactor Citations Relations Service Layer (#11189)
alexandre-cremieux Sep 28, 2024
0e9de3c
Merge branch 'main' into fix-issue-11189-part-00-refactor-citation-re…
koppor Oct 10, 2024
9a31735
Address PR comments (#11901)
alexandre-cremieux Nov 6, 2024
b1133d0
MVStore implementation for citations: first approach (wip for open di…
alexandre-cremieux Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 0 additions & 40 deletions src/main/java/org/jabref/gui/entryeditor/citationrelationtab/BibEntryRelationsCache.java
View file
Open in desktop

This file was deleted.

73 changes: 0 additions & 73 deletions ...main/java/org/jabref/gui/entryeditor/citationrelationtab/BibEntryRelationsRepository.java
View file
Open in desktop

This file was deleted.

95 changes: 62 additions & 33 deletions src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.entryeditor.citationrelationtab.semanticscholar.CitationFetcher;
import org.jabref.gui.entryeditor.citationrelationtab.semanticscholar.SemanticScholarFetcher;
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;
Expand Down Expand Up @@ -85,7 +88,7 @@ public class CitationRelationsTab extends EntryEditorTab {
private final GuiPreferences preferences;
private final LibraryTab libraryTab;
private final TaskExecutor taskExecutor;
private final BibEntryRelationsRepository bibEntryRelationsRepository;
private final SearchCitationsRelationsService searchCitationsRelationsService;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduces a service layer that segregates the fetching and repository logic definitions.

private final CitationsRelationsTabViewModel citationsRelationsTabViewModel;
private final DuplicateCheck duplicateCheck;
private final BibEntryTypesManager entryTypesManager;
Expand All @@ -109,9 +112,22 @@ public CitationRelationsTab(DialogService dialogService,

this.entryTypesManager = bibEntryTypesManager;
this.duplicateCheck = new DuplicateCheck(entryTypesManager);
this.bibEntryRelationsRepository = new BibEntryRelationsRepository(new SemanticScholarFetcher(preferences.getImporterPreferences()),
new BibEntryRelationsCache());
citationsRelationsTabViewModel = new CitationsRelationsTabViewModel(databaseContext, preferences, undoManager, stateManager, dialogService, fileUpdateMonitor, taskExecutor);
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
);
}

/**
Expand Down Expand Up @@ -405,41 +421,54 @@ private void searchForRelations(BibEntry entry, CheckListView<CitationRelationIt
citedByTask.cancel();
}

BackgroundTask<List<BibEntry>> task;

if (searchType == CitationFetcher.SearchType.CITES) {
task = BackgroundTask.wrap(() -> {
if (shouldRefresh) {
bibEntryRelationsRepository.forceRefreshReferences(entry);
}
return bibEntryRelationsRepository.getReferences(entry);
});
citingTask = task;
} else {
task = BackgroundTask.wrap(() -> {
if (shouldRefresh) {
bibEntryRelationsRepository.forceRefreshCitations(entry);
}
return bibEntryRelationsRepository.getCitations(entry);
});
citedByTask = task;
}

task.onRunning(() -> prepareToSearchForRelations(abortButton, refreshButton, importButton, progress, task))
.onSuccess(fetchedList -> onSearchForRelationsSucceed(entry, listView, abortButton, refreshButton,
searchType, importButton, progress, fetchedList, observableList))
this.createBackGroundTask(entry, searchType, shouldRefresh)
.consumeOnRunning(task -> prepareToSearchForRelations(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably be renamed applyOnRunning(Consumer<Task> consumer).

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);
hideNodes(abortButton, progress, importButton);
listView.setPlaceholder(new Label(Localization.lang("Error while fetching citing entries: %0",
exception.getMessage())));

listView.setPlaceholder(
new Label(Localization.lang(
"Error while fetching citing entries: %0", exception.getMessage())
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not reformat. Our tooling cannot deal with that - see https://devdocs.jabref.org/code-howtos/localization.html for some hints.

);
refreshButton.setVisible(true);
dialogService.notify(exception.getMessage());
})
.executeWith(taskExecutor);
}

private BackgroundTask<List<BibEntry>> createBackGroundTask(
BibEntry entry, CitationFetcher.SearchType searchType, boolean shouldRefresh
) {
return switch (searchType) {
case CitationFetcher.SearchType.CITES -> {
citingTask = BackgroundTask.wrap(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really appreciate this solution. The method should return a Callable instead of BackGroundTask and it should not be possible to restart a search if one is already running for same tab. I propose to refactor this in a next PR but lets focus on the cache refactoring first. For now logic is same as before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me.

You can add a TODO comment if you want.

() -> 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,
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion ...n/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@

import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.entryeditor.citationrelationtab.semanticscholar.CitationFetcher;
import org.jabref.gui.externalfiles.ImportHandler;
import org.jabref.gui.preferences.GuiPreferences;
import org.jabref.logic.citationkeypattern.CitationKeyGenerator;
import org.jabref.logic.citationkeypattern.CitationKeyPatternPreferences;
import org.jabref.logic.importer.fetcher.CitationFetcher;
import org.jabref.logic.util.TaskExecutor;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
Expand Down
20 changes: 20 additions & 0 deletions src/main/java/org/jabref/logic/citation/repository/BibEntryRelationsRepository.java
View file
Open in desktop
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one will be re-implemented in next PR.


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);
}
57 changes: 57 additions & 0 deletions src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java
View file
Open in desktop
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns a copy now.

.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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method used to rewrite data, now it does merge inputs according the method name.

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);
}
}
Loading
Loading