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

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

Open
wants to merge 25 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 16 commits
Commits
Show all changes
25 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
6a8b21b
Introduce the DAO layer for relations (#11189):
alexandre-cremieux Nov 17, 2024
01f6da4
MVStoreDAO implementation for citations relations (#11189):
alexandre-cremieux Nov 17, 2024
337780d
Implement a search lock mechanism for citations relations (#11189):
alexandre-cremieux Nov 24, 2024
7c2e32d
Avoid user to force update citation relations even the fetcher return…
alexandre-cremieux Nov 27, 2024
187b5d4
Make the citation relations update automatic after the guard delay is…
alexandre-cremieux Dec 8, 2024
b71d9fc
SearchCitationsRelationsService as singleton
Jan 11, 2025
dda21ed
Merge branch 'main' into fix-issue-11189-part-00-refactor-citation-re…
Jan 20, 2025
077e9ca
Update failing tests after merge
Jan 20, 2025
0bff913
Clean code style according to checkstyle
Jan 20, 2025
ea36f1a
Clean code style according to checkstyle
Jan 20, 2025
d7f9c2d
Fix pending PR comments
Jan 20, 2025
97b38c6
Fix null pointer exception in CitationsRelationsTab
Jan 22, 2025
54e7e21
Add settings for Citations relations store TTL
Jan 25, 2025
6415b1b
Update MVStoreBibEntryRelationDAO serializer/deserializer
Jan 25, 2025
e18d557
Update CHANGELOG-MD for Citations relations tab caching logic (#11189)
Jan 26, 2025
57b5c02
Merge branch 'main' into fix-issue-11189-part-00-refactor-citation-re…
Jan 26, 2025
16cfd2c
Return an empty BibEntry from MVStoreBibEntryRelationDAO in case of p…
Jan 26, 2025
33f5cdf
Add a test to ensure that an empty list is returned from in case of s…
Jan 27, 2025
2ebf62e
Merge branch 'main' into fix-issue-11189-part-00-refactor-citation-re…
Jan 28, 2025
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
6 changes: 6 additions & 0 deletions src/main/java/org/jabref/gui/JabRefGUI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.jabref.gui.util.UiTaskExecutor;
import org.jabref.logic.UiCommand;
import org.jabref.logic.ai.AiService;
import org.jabref.logic.citation.SearchCitationsRelationsService;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.net.ProxyRegisterer;
import org.jabref.logic.os.OS;
Expand Down Expand Up @@ -175,6 +176,11 @@ public void initialize() {
dialogService,
taskExecutor);
Injector.setModelOrService(AiService.class, aiService);

Injector.setModelOrService(
SearchCitationsRelationsService.class,
new SearchCitationsRelationsService(preferences.getImporterPreferences())
);
}

private void setupProxy() {
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/org/jabref/gui/entryeditor/EntryEditor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import org.jabref.gui.util.UiTaskExecutor;
import org.jabref.logic.ai.AiService;
import org.jabref.logic.bibtex.TypedBibEntry;
import org.jabref.logic.citation.SearchCitationsRelationsService;
import org.jabref.logic.help.HelpFile;
import org.jabref.logic.importer.EntryBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
Expand Down Expand Up @@ -123,6 +124,7 @@ public class EntryEditor extends BorderPane {
@Inject private KeyBindingRepository keyBindingRepository;
@Inject private JournalAbbreviationRepository journalAbbreviationRepository;
@Inject private AiService aiService;
@Inject private SearchCitationsRelationsService searchCitationsRelationsService;

private final List<EntryEditorTab> allPossibleTabs;

Expand Down Expand Up @@ -305,8 +307,13 @@ private List<EntryEditorTab> createTabs() {
tabs.add(new MathSciNetTab());
tabs.add(new FileAnnotationTab(libraryTab.getAnnotationCache()));
tabs.add(new SciteTab(preferences, taskExecutor, dialogService));
tabs.add(new CitationRelationsTab(dialogService, databaseContext,
undoManager, stateManager, fileMonitor, preferences, libraryTab, taskExecutor, bibEntryTypesManager));
tabs.add(new CitationRelationsTab(
dialogService, databaseContext,
undoManager, stateManager,
fileMonitor, preferences,
libraryTab, taskExecutor,
bibEntryTypesManager, searchCitationsRelationsService
));
tabs.add(new RelatedArticlesTab(buildInfo, databaseContext, preferences, dialogService, taskExecutor));
sourceTab = new SourceTab(
databaseContext,
Expand Down
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.

97 changes: 60 additions & 37 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 @@ -37,8 +37,6 @@
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.mergeentries.EntriesMergeResult;
import org.jabref.gui.mergeentries.MergeEntriesDialog;
Expand All @@ -51,8 +49,10 @@
import org.jabref.logic.bibtex.BibEntryWriter;
import org.jabref.logic.bibtex.FieldPreferences;
import org.jabref.logic.bibtex.FieldWriter;
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.l10n.Localization;
import org.jabref.logic.os.OS;
import org.jabref.logic.util.BackgroundTask;
Expand Down Expand Up @@ -92,7 +92,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;
alexandre-cremieux marked this conversation as resolved.
Show resolved Hide resolved
private final CitationsRelationsTabViewModel citationsRelationsTabViewModel;
private final DuplicateCheck duplicateCheck;
private final BibEntryTypesManager entryTypesManager;
Expand All @@ -107,7 +107,8 @@ public CitationRelationsTab(DialogService dialogService,
GuiPreferences preferences,
LibraryTab libraryTab,
TaskExecutor taskExecutor,
BibEntryTypesManager bibEntryTypesManager) {
BibEntryTypesManager bibEntryTypesManager,
SearchCitationsRelationsService searchCitationsRelationsService) {
this.dialogService = dialogService;
this.databaseContext = databaseContext;
this.preferences = preferences;
Expand All @@ -121,9 +122,17 @@ 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);
this.searchCitationsRelationsService = searchCitationsRelationsService;

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

/**
Expand Down Expand Up @@ -199,22 +208,22 @@ private SplitPane getPaneAndStartSearch(BibEntry entry) {

refreshCitingButton.setOnMouseClicked(event -> {
searchForRelations(entry, citingListView, abortCitingButton,
refreshCitingButton, CitationFetcher.SearchType.CITES, importCitingButton, citingProgress, true);
refreshCitingButton, CitationFetcher.SearchType.CITES, importCitingButton, citingProgress);
});

refreshCitedByButton.setOnMouseClicked(event -> searchForRelations(entry, citedByListView, abortCitedButton,
refreshCitedByButton, CitationFetcher.SearchType.CITED_BY, importCitedByButton, citedByProgress, true));
refreshCitedByButton, CitationFetcher.SearchType.CITED_BY, importCitedByButton, citedByProgress));

// Create SplitPane to hold all nodes above
SplitPane container = new SplitPane(citingVBox, citedByVBox);
styleFetchedListView(citedByListView);
styleFetchedListView(citingListView);

searchForRelations(entry, citingListView, abortCitingButton, refreshCitingButton,
CitationFetcher.SearchType.CITES, importCitingButton, citingProgress, false);
CitationFetcher.SearchType.CITES, importCitingButton, citingProgress);

searchForRelations(entry, citedByListView, abortCitedButton, refreshCitedByButton,
CitationFetcher.SearchType.CITED_BY, importCitedByButton, citedByProgress, false);
CitationFetcher.SearchType.CITED_BY, importCitedByButton, citedByProgress);

return container;
}
Expand Down Expand Up @@ -410,7 +419,7 @@ protected void bindToEntry(BibEntry entry) {
*/
private void searchForRelations(BibEntry entry, CheckListView<CitationRelationItem> listView, Button abortButton,
Button refreshButton, CitationFetcher.SearchType searchType, Button importButton,
ProgressIndicator progress, boolean shouldRefresh) {
ProgressIndicator progress) {
if (entry.getDOI().isEmpty()) {
hideNodes(abortButton, progress);
showNodes(refreshButton);
Expand All @@ -424,47 +433,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)
.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);
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 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@InAnYan This was here before I touched this code. I am leaving this for now in this PR as this out of the scope.

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? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@InAnYan This was here before I touched this code. I am leaving this for now in this PR as this out of the scope.

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
) {
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)
);
yield citingTask;
}
case CitationFetcher.SearchType.CITED_BY -> {
citedByTask = BackgroundTask.wrap(
() -> this.searchCitationsRelationsService.searchCitations(entry)
);
yield citedByTask;
}
};
}

private void onSearchForRelationsSucceed(BibEntry entry, CheckListView<CitationRelationItem> listView,
Button abortButton, Button refreshButton,
CitationFetcher.SearchType searchType, Button importButton,
Expand All @@ -481,7 +504,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 @@ -7,10 +7,10 @@

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.importer.fetcher.CitationFetcher;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.util.TaskExecutor;
import org.jabref.model.database.BibDatabaseContext;
Expand Down
Loading
Loading