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 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
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

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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you discussed this with Oliver?

It's your local directory, but I think you probably understand that you should change it.

var citationsPath = Path.of(jabRefPath.toAbsolutePath() + File.separator + "citations");
var relationsPath = Path.of(jabRefPath.toAbsolutePath() + File.separator + "references");
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about Path.resolve()? I think it's more idiomatic. What if toAbsolutePath() will return a trailing slash? That is why there is a dedicated resolve().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also make constants for "citations" and "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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For nullable things we typically use Option. (Even though IDEA will warn that oh no optional is used as a field.

But still, Optional is better

Copy link
Collaborator

Choose a reason for hiding this comment

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

citedByTask != null && !citedByTask.isCancelled()

Could be rewritten like citedByTask.map(BackgroundTask::isCancelled).orElse(false) (could you please double-check if any parameters are needed in BackgroundTask::).

@koppor, using Optionals seems to be a little bit more verbose. Should we still use Optional there instead of null?

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

citing Articles -> citing articles

just a typo 😃

hideNodes(abortButton, progress, importButton);
listView.setPlaceholder(new Label(Localization.lang("Error while fetching citing entries: %0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

articles or entries? 😅

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,53 @@
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.isReferencesUpdatable(referencer))
|| !this.relationsRepository.containsReferences(referencer)) {
try {
var references = this.citationFetcher.searchCiting(referencer);
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.isCitationsUpdatable(cited))
|| !this.relationsRepository.containsCitations(cited)) {
try {
var citations = this.citationFetcher.searchCitedBy(cited);
this.relationsRepository.insertCitations(cited, citations);
} catch (FetcherException e) {
LOGGER.error("Error while fetching citations for entry {}", cited.getTitle(), e);
}
}
return this.relationsRepository.readCitations(cited);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will return an empty list if no citation were inserted, right?

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
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);

default boolean isUpdatable(BibEntry entry) {
return true;
}
}
Loading