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.

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

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

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

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package org.jabref.gui.entryeditor.citationrelationtab;

import java.io.File;
import java.io.IOException;
import java.io.StringWriter;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;

Expand Down Expand Up @@ -36,17 +40,19 @@
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.ChainBibEntryRelationsRepository;
import org.jabref.logic.citation.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 +91,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 +115,29 @@ 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);

try {
var jabRefPath = Paths.get("/home/sacha/Documents/projects/JabRef");
var citationsPath = Path.of(jabRefPath.toAbsolutePath() + File.separator + "citations");
var relationsPath = Path.of(jabRefPath.toAbsolutePath() + File.separator + "references");
var bibEntryRelationsRepository = new ChainBibEntryRelationsRepository(citationsPath, relationsPath);
this.searchCitationsRelationsService = new SearchCitationsRelationsService(
new SemanticScholarCitationFetcher(preferences.getImporterPreferences()), bibEntryRelationsRepository
);
} catch (Exception e) {
e.printStackTrace();
throw new RuntimeException(e);
}

citationsRelationsTabViewModel = new CitationsRelationsTabViewModel(
databaseContext,
preferences,
undoManager,
stateManager,
dialogService,
fileUpdateMonitor,
taskExecutor
);
}

/**
Expand Down Expand Up @@ -399,47 +425,61 @@ private void searchForRelations(BibEntry entry, CheckListView<CitationRelationIt

listView.setItems(observableList);

// TODO: It should not be possible to cancel a search task that is already running for same tab
if (citingTask != null && !citingTask.isCancelled() && searchType == CitationFetcher.SearchType.CITES) {
citingTask.cancel();
} else if (citedByTask != null && !citedByTask.isCancelled() && searchType == CitationFetcher.SearchType.CITED_BY) {
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())));

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

/**
* TODO: Make the method return a callable and let the calling method create the background task.
*/
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 +496,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
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package org.jabref.logic.citation;

import java.util.List;

import org.jabref.logic.citation.repository.BibEntryRelationsRepository;
import org.jabref.logic.importer.FetcherException;
import org.jabref.logic.importer.fetcher.CitationFetcher;
import org.jabref.model.entry.BibEntry;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class SearchCitationsRelationsService {

private static final Logger LOGGER = LoggerFactory
.getLogger(SearchCitationsRelationsService.class);

private final CitationFetcher citationFetcher;
private final BibEntryRelationsRepository relationsRepository;

public SearchCitationsRelationsService(
CitationFetcher citationFetcher, BibEntryRelationsRepository repository
) {
this.citationFetcher = citationFetcher;
this.relationsRepository = repository;
}

public List<BibEntry> searchReferences(BibEntry referencer, boolean forceUpdate) {
if (forceUpdate || !this.relationsRepository.containsReferences(referencer)) {
try {
var references = this.citationFetcher.searchCiting(referencer);
if (!references.isEmpty()) {
this.relationsRepository.insertReferences(referencer, references);
}
} catch (FetcherException e) {
LOGGER.error("Error while fetching references for entry {}", referencer.getTitle(), e);
}
}
return this.relationsRepository.readReferences(referencer);
}

public List<BibEntry> searchCitations(BibEntry cited, boolean forceUpdate) {
if (forceUpdate || !this.relationsRepository.containsCitations(cited)) {
try {
var citations = this.citationFetcher.searchCitedBy(cited);
if (!citations.isEmpty()) {
this.relationsRepository.insertCitations(cited, citations);
}
} catch (FetcherException e) {
LOGGER.error("Error while fetching citations for entry {}", cited.getTitle(), e);
}
}
return this.relationsRepository.readCitations(cited);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.jabref.logic.citation.repository;

import java.util.List;

import org.jabref.model.entry.BibEntry;

public interface BibEntryRelationDAO {

List<BibEntry> getRelations(BibEntry entry);

void cacheOrMergeRelations(BibEntry entry, List<BibEntry> relations);

boolean containsKey(BibEntry entry);
}
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);
}
Loading